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 deprecated Authentication#getUser #91069

Merged
merged 3 commits into from
Oct 25, 2022
Merged

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Oct 24, 2022

This PR removes the deprecated Authentication#getUser method and replaces its usages with #getEffectiveSubject#getUser.

Relates: #88494

This PR removes the deprecated Authentication#getUser method and
replaces its usages with #getEffectiveSubject#getUser.

Relates: elastic#88494
@ywangd ywangd added >refactoring :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.6.0 labels Oct 24, 2022
@ywangd ywangd requested a review from n1v0lg October 24, 2022 02:50
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Oct 24, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@ywangd
Copy link
Member Author

ywangd commented Oct 24, 2022

Failure is unrelated. It's reproducible on the main branch and I raised #91070

@ywangd
Copy link
Member Author

ywangd commented Oct 24, 2022

@elasticmachine run elasticsearch-ci/part-2

Copy link
Contributor

@n1v0lg n1v0lg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

logger.trace(
"Looking up run-as user [{}] for authenticated user [{}]",
runAsUsername,
authentication.getEffectiveSubject().getUser().principal()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use getAuthenticatingSubject here instead? I know at this stage in the flow that will be the same as getEffectiveSubject but it's slightly more accurate. No need to do it as part of this PR since it may have cascading changes to tests where we mock

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I'll update this and below to use getAuthenticatingSubject. Will fix the tests if there are any failure. Thanks!

@ywangd ywangd merged commit d3a781c into elastic:main Oct 25, 2022
jakelandis added a commit that referenced this pull request Oct 25, 2022
#91069 removed this method. This commit removes the usage from the examples.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>refactoring :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants