Skip to content

Conversation

@tvernum
Copy link
Contributor

@tvernum tvernum commented Dec 12, 2024

This changes AuthenticationServiceTests.testInvalidToken to configure the security (tokens) index correctly.

Previously defensiveCopy() would return null which would cause getTokenDocById to throw an exception, which decodeToken would catch and ignore.

But that is not a realistic scenario, and is testing by side-effect.

This changes `AuthenticationServiceTests.testInvalidToken` to
configure the security (tokens) index correctly.

Previously `defensiveCopy()` would return `null` which would cause
`getTokenDocById` to throw an exception, which `decodeToken` would
catch and ignore.

But that is not a realistic scenario, and is testing by side-effect.
@tvernum tvernum requested a review from jfreden December 12, 2024 05:47
@elasticsearchmachine elasticsearchmachine added v9.0.0 needs:triage Requires assignment of a team area label labels Dec 12, 2024
@tvernum tvernum added >test Issues or PRs that are addressing/adding tests :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) and removed needs:triage Requires assignment of a team area label labels Dec 12, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Dec 12, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@tvernum
Copy link
Contributor Author

tvernum commented Dec 12, 2024

I have a refactor of the security index manager on another branch, and on that branch the test actually fails because the incorrect usage triggers an assert instead of throwing a NPE.

Copy link
Contributor

@jfreden jfreden left a comment

Choose a reason for hiding this comment

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

LGTM!

@tvernum tvernum enabled auto-merge (squash) December 16, 2024 07:40
@tvernum tvernum merged commit 55ae512 into elastic:main Dec 16, 2024
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team >test Issues or PRs that are addressing/adding tests v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants