Locking down Authentication constructors #86206
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR removes one constructor from Authentication and change the visibility of the other one to private. This means Authentication object must now be created using dedicated convenient methods, e.g. newRealmAuthentication. This approach helps maintain the internal logic of Authentication object more easily and correctly. It also allows further refactoring for Authentication internals easier. Technically, the constructor with StreamInput argument is still public. But this one is special enough that we can leave it for now and come back to it later if necessary. Relates: elastic#85905 Relates: elastic#86020 Relates: elastic#86054
|
Pinging @elastic/es-security (Team:Security) |
tvernum
approved these changes
May 2, 2022
ywangd
added a commit
to ywangd/elasticsearch
that referenced
this pull request
May 3, 2022
In elastic#86206, we closed down Authentication constructors to favour dedicated convenient methods for instantiation. The constructor usages in the example project were however left out (another refactor fallout). Relates: elastic#86206 Resolves: elastic#86378
ywangd
added a commit
that referenced
this pull request
May 4, 2022
Today the run-as information is spread between User (authenticatedUser) and Authentication (lookup realm). They have to be configured consistently to work correctly. Previously there was also no inherent logic to ensure the consistency. Recent refactoring of Authentication class has made the situation better by favour Authentication creation with dedicated convenient methods over free-for-all constructors. #86206 is the ongoing PR that will finally remove public access of Authentication constructors. Now that the Authentication class is being tightly controlled, it makes possible to clean-up the User class. Specifically, the run-as information is already provided by the Authentication class, there is no need for the User class to also keep track of it. In fact, the way User class tracking the authenticating user with an inner user is less straightforward and not friendly to serialisation. Also, conceptually run-as is an information at Authentication level instead of User level. This PR refactors the User class so that it no longer keeps track of run-as information so that there is a single consistent way to check whether an Authentication object is run-as. The essential changes are: * Removes User.isRunAs() method * Removes User.authenticatedUser() method and its backing instance variable * Removes all User constructors that take authenticatedUser as an argument * Adds a new private RunAsUser class inside Authentication Note that this RunAsUser class is not really necessary in long term, the plan is to remove it in the later refactoring of Authentication class (where Subject variable and methods will become primary over the current User). It is added mostly to make the refactoring easier and reduce the change to how things work conceptually. This class is not exposed and there should be no need to use this class outside of Authentication itself. This tight scope should make it relatively easy to remove it later. Relates: #86206 Relates: #80117
ywangd
added a commit
to ywangd/elasticsearch
that referenced
this pull request
May 9, 2022
This PR marks the Authentication class to be final which is the last step of locking down the Authentication class. The Authentication class is basically a record class and it has internal logics on what values can or cannot be used together. We don't expect it to be extended by any subclasses and a concrete object should always be created for tests instead of mocking. Relates: elastic#86424 Relates: elastic#86206
ywangd
added a commit
that referenced
this pull request
May 17, 2022
This PR marks the Authentication class to be final which is the last step of locking down the Authentication class. The Authentication class is basically a record class and it has internal logics on what values can or cannot be used together. We don't expect it to be extended by any subclasses and a concrete object should always be created for tests instead of mocking. Relates: #86424 Relates: #86206
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes one constructor from Authentication and change
the visibility of the other one to private. This means Authentication
object must now be created using dedicated convenient methods, e.g.
newRealmAuthentication. This approach helps maintain the internal logic
of Authentication object more easily and correctly. It also allows
further refactoring for Authentication internals easier.
Technically, the constructor with StreamInput argument is still public.
But this one is special enough that we can leave it for now and come
back to it later if necessary.
Relates: #85905
Relates: #86020
Relates: #86054