[Test] Add a dedicate helper class for randomizing Authentication #85590
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
The Authentication class has many internal logics about what values can or cannot be combined together. It is not straightforward to always get the logic right when trying to create such an object for tests. This could lead to spurious failures or incomplete test coverage. This PR adds a helper class for creating such an object with necessary configurabililty. The relevant methods in AuthenticationTests now delegate to the new class to avoid having to touch too many files in one PR. The ultimate goal is to have it used in every place where an Authentication object is needed for test to replace any calls to constructors or mocking.
|
Pinging @elastic/es-security (Team:Security) |
albertzaharovits
approved these changes
Apr 13, 2022
ywangd
added a commit
to ywangd/elasticsearch
that referenced
this pull request
Apr 19, 2022
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
ywangd
added a commit
that referenced
this pull request
Apr 21, 2022
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
ywangd
added a commit
to ywangd/elasticsearch
that referenced
this pull request
Apr 26, 2022
In elastic#85255, some mocking of Authentication class got replaced by randomly creating actual Authentication objects. This is a general direction we want to head towards because Authentication object has plenty internal logics which makes it hard to mock correctly (and also undesirable). The recent change in elastic#85590 adds a test helper which makes randomising Authentication object easier for tests. For ApiKeyServiceTests.testGetApiKeyMetadata, the randomisation is however too broad (broader then what the mocking provided) and can sometimes creates authentication object that does not pass the assertion. The assertion expects no API key authentication. But the randomisation can generate such one because it randomises whether the authentication has run-as even when the effective user is from a realm. Since API keys can run-as, the resulted Authentication object can be an overall API key authentication object. This PR reduces the randomness by not allow run-as so that the resulted Authentication cannot be API keys. Relates: elastic#85255 Resolves: elastic#86179
ywangd
added a commit
that referenced
this pull request
Apr 26, 2022
In #85255, some mocking of Authentication class got replaced by randomly creating actual Authentication objects. This is a general direction we want to head towards because Authentication object has plenty internal logics which makes it hard to mock correctly (and also undesirable). The recent change in #85590 adds a test helper which makes randomising Authentication object easier for tests. For ApiKeyServiceTests.testGetApiKeyMetadata, the randomisation is however too broad (broader then what the mocking provided) and can sometimes creates authentication object that does not pass the assertion. The assertion expects no API key authentication. But the randomisation can generate such one because it randomises whether the authentication has run-as even when the effective user is from a realm. Since API keys can run-as, the resulted Authentication object can be an overall API key authentication object. This PR reduces the randomness by not allow run-as so that the resulted Authentication cannot be API keys. Relates: #85255 Resolves: #86179
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.
The Authentication class has many internal logics about what values can
or cannot be combined together. It is not straightforward to always get
the logic right when trying to create such an object for tests. This
could lead to spurious failures or incomplete test coverage.
This PR adds a helper class for creating such an object with necessary
configurabililty. The relevant methods in AuthenticationTests now
delegate to the new class to avoid having to touch too many files in one
PR. The ultimate goal is to have it used in every place where an
Authentication object is needed for test to replace any calls to
constructors or mocking.