[Test] Remove test usage of Authentication constructor #86054
Conversation
This PR is a follow-up of elastic#86020 and removes all remaining usages of Authentication constructor in test code. Relates: elastic#86020
ywangd
added
>test
|
Pinging @elastic/es-security (Team:Security) |
| final int tokenVariant = ESTestCase.randomIntBetween(0, 9); | ||
| if (tokenVariant == 0 && user == null && realmRef == null) { | ||
| // service account | ||
| prepareServiceAccountMetadata(); |
There was a problem hiding this comment.
A bug fix for the test helper itself
| return randomValueOtherThanMany( | ||
| authc -> authc.getAuthenticationType() == AuthenticationType.INTERNAL, | ||
| () -> AuthenticationTestHelper.builder().build() | ||
| ); |
There was a problem hiding this comment.
Internal users are filtered out by default for audit logging.
| final User finalUser; | ||
| final boolean isRunAs = randomBoolean(); | ||
| // If testing run-as, randomize whether the service account actually has the run-as permission | ||
| // This makes a difference in the auditing logs (runAsDenied vs accessDenied) | ||
| final boolean canRunAs = isRunAs && randomBoolean(); | ||
| if (isRunAs) { | ||
| finalUser = new User(new User(randomAlphaOfLengthBetween(3, 8)), serviceUser); | ||
| } else { | ||
| finalUser = serviceUser; | ||
| } |
There was a problem hiding this comment.
Service account cannot run-as. So no point to test.
| // Service account run as | ||
| final User authenticatedUser2 = new User("elastic/some-service"); |
There was a problem hiding this comment.
Service account cannot run-as.
| verify(apiKeyService).parseRoleDescriptorsBytes("key-id-3", anotherRoleBytes, RoleReference.ApiKeyRoleType.ASSIGNED); | ||
| } | ||
|
|
||
| private Authentication createAuthentication() { |
There was a problem hiding this comment.
This method has only 2 usages and are both replaced by inline code.
| // User associated to API key authentication has empty roles | ||
| user = stripRoles(user); |
There was a problem hiding this comment.
Another bug for the test helper
There was a problem hiding this comment.
I'm Okay with the strip but I would've preferred an assert for the empty role name list, wdyt?
There was a problem hiding this comment.
I chose to strip the roles for usability reasons. It is challenging to randomize authentication freely if we assert the role list must be empty and the Authentication happens to be an API key (it can either API key or a token created by an API key or run-as by an API key). Since this is after all test code, I took the easier approach to strip roles.
That said, stripping roles is in fact more akin to what happens in production code:
which essentially also strips the roles.
| TimeValue.parseTimeValue(randomTimeValue(), getClass().getSimpleName() + ".expiresIn"), | ||
| createAuthentication() | ||
| AuthenticationTestHelper.builder() | ||
| .realm() |
| assert authResult.isAuthenticated() : "API Key authn result must be successful"; | ||
| final User apiKeyUser = authResult.getValue(); | ||
| assert false == apiKeyUser.isRunAs(); | ||
| assert apiKeyUser.roles().length == 0 : "The user associated to an API key authentication must have no role"; |
There was a problem hiding this comment.
This looks like something ripe for a refactoring 😁
There was a problem hiding this comment.
Agreed. We can iterate how newApiKeyAuthentication should work in future PRs. The roles are better stripped inside this method for a central control.
|
@elasticmachine run elasticsearch-ci/part-2 |
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
3 participants
This PR is a follow-up of #86020 and removes all remaining usages of
Authentication constructor in test code.
Relates: #86020