[Test] Remove calls to Authentication constructor#86020
[Test] Remove calls to Authentication constructor#86020ywangd merged 6 commits intoelastic:masterfrom
Conversation
This PR is another step towards locking down how Authentication object can be instantiated: It should be created using dedicated convenient methods instead of constructors. Production usage of constructors are mostly removed. But lots of test code still uses them. This PR replaces one form of the usage with the newly introduced test helper and removes the corresponding constructor. Relates: elastic#85590 Relates: elastic#85905
|
Pinging @elastic/es-security (Team:Security) |
| public Authentication(User user, RealmRef authenticatedBy, RealmRef lookedUpBy) { | ||
| this(user, authenticatedBy, lookedUpBy, Version.CURRENT, AuthenticationType.REALM, Collections.emptyMap()); | ||
| } |
There was a problem hiding this comment.
This constructor is now removed. The next PR will make the other constructor private.
There was a problem hiding this comment.
After a second thought, I restored this constructor for this PR because it is tagged as >test and this is technically production code. I will raise a separate PR after all test usages are addressed to remove this constructor and change the other to private.
| } | ||
| case TOKEN -> { | ||
| if (isServiceAccount) { | ||
| if (isServiceAccount != null && isServiceAccount) { |
There was a problem hiding this comment.
A bug fix for the test helper itself.
| final RealmRef authenticatedBy = new RealmRef("auth_by", "auth_by_type", "node"); | ||
| final RealmRef lookedUpBy = new RealmRef("lookup_by", "lookup_by_type", "node"); | ||
| final Authentication authentication = new Authentication(new User("user"), authenticatedBy, lookedUpBy); | ||
| final Authentication authentication = AuthenticationTestHelper.builder() | ||
| .user(new User("not-user")) | ||
| .realmRef(authenticatedBy) | ||
| .runAs() | ||
| .user(new User("user")) | ||
| .realmRef(lookedUpBy) | ||
| .build(); |
There was a problem hiding this comment.
The authentication object was created incorrectly: The user does not have a run-as but the authentication has a non-null lookup realm. So the new code fixed it by adding an authenticating user as I believe the intention here was to have run-as.
The similar issue existed in a few other places and they have been fixed similarly.
| @@ -209,38 +218,22 @@ public void testRunAsAccessResourceOf() { | |||
| } | |||
|
|
|||
| public void testIsServiceAccount() { | |||
There was a problem hiding this comment.
Service account does not support run-as nor run-by another user. So the old test is not really correct. It is fixed accordingly.
| Authentication original = AuthenticationTestHelper.builder() | ||
| .user(new User("test", "role")) | ||
| .realmRef(new Authentication.RealmRef("realm", "file", "node")) | ||
| .build(false); |
There was a problem hiding this comment.
The false argument here (and in many other places) is to make sure the randomized Authentication is not run-as because that is how the Authentication is created in the current code (null for lookup realm). I suspect most of the tests would work with a run-as Authentication. But I chose to be conservative to reduce the complexity of this PR.
|
@elasticmachine run elasticsearch-ci/part-2 |
This PR is a follow-up of elastic#86020 and removes all remaining usages of Authentication constructor in test code. Relates: elastic#86020
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
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
This PR is another step towards locking down how Authentication object
can be instantiated: It should be created using dedicated convenient
methods instead of constructors.
Production usage of constructors are mostly removed. But lots of test
code still uses them. This PR replaces one form of the usage with the
newly introduced test helper and removes the corresponding constructor.
Relates: #85590
Relates: #85905