Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove try auth mode #94287

Merged
merged 4 commits into from
Mar 10, 2021
Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Mar 10, 2021

Summary

Follow-up of #92784 given #92784 (comment)

  • Remove the authRequired: try auth mode
  • Use HAPI's try mode instead of optional when authRequired is optional

Checklist

@pgayvallet pgayvallet added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.13.0 labels Mar 10, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet marked this pull request as ready for review March 10, 2021 15:28
@pgayvallet pgayvallet requested a review from a team as a code owner March 10, 2021 15:28
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@pgayvallet pgayvallet requested a review from mshustov March 10, 2021 15:28
@joshdover
Copy link
Contributor

joshdover commented Mar 10, 2021

Just to be sure I followed along here correctly:

  • We used to have required and optional where optional would return an error for invalid creds
  • We added try which would not return an error for invalid creds
  • We're now replacing optional with try's behavior and removing try. So now invalid creds are treated the same as no creds on optional routes.

Sound right?

@pgayvallet
Copy link
Contributor Author

@joshdover that's right.

We confirmed with the security team that they were ok using (HAPI's) try instead of optional for their routes.

Note that the only reason why we need to use HAPI's try (treat invalid creds as no cred) is the way the FTR tests are deleting users before accessing the logout page (which cause failures because the /bootstrap.js endpoint throws a 403 in that case when accessing the page), but fixing that in FTR would be so impactful that we decided to just switch to try instead.

@pgayvallet pgayvallet merged commit 7d2c4d4 into elastic:master Mar 10, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Mar 10, 2021
* remove `try` auth mode

* update generated doc

* update generated doc

* adapt integration test
pgayvallet added a commit that referenced this pull request Mar 10, 2021
* remove `try` auth mode

* update generated doc

* update generated doc

* adapt integration test
jloleysens added a commit that referenced this pull request Mar 11, 2021
…-action

* 'master' of github.com:elastic/kibana: (43 commits)
  [Console] Update copy when showing warnings in response headers (#94270)
  [TSVB] Type public code. Step 1 (#93231)
  [ML] Functional tests - stabilize slider value selection (#94313)
  skip another suite blocking es promotion (#94367)
  [Security Solution] Eliminates a redundant external link icon (#94194)
  skip another suite blocking es promotion (#94367)
  [App Search] Role mappings migration part 1 (#94346)
  [Security Solution][Detections] Fix flaky indicator enrichment tests (#94241)
  [Workplace Search] Deduplicate icons (#94359)
  [ML] Add latest transform to intro text (#94039)
  skip test failing es promotion (#94367)
  [Maps] convert elasticsearch_utils to TS (#93984)
  [Security_Solution][Telemetry] - Update endpoint usage to use agentService (#93829)
  [Security Solution][Exceptions] Fixes OS adding method for exception enrichment (#94343)
  [ILM] Add support for frozen phase (#93068)
  [App Search] Fixed 2 relevance tuning bugs (#94312)
  remove `try` auth mode (#94287)
  Removing resolver functional tests (#94331)
  migrate warning mixin to core (#94273)
  [App Search] Add routes for Role Mappings (#94221)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/phase/phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/configuration_issues_context.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants