Don't allow null User.principal #51988
Conversation
Some parts of the User class (e.g. equals/hashCode) assumed that principal could never be null, but the constructor didn't enforce that. This adds a null check into the constructor and fixes a few tests that relied on being able to pass in null usernames.
tvernum
added
>non-issue
:Security/Authentication
|
Pinging @elastic/es-security (:Security/Authentication) |
|
This is some yak shaving for multiple authentication headers. That PR got big, so I'm working on splitting out some standalone pieces. |
|
@elasticmachine update branch |
| threadContext.putTransient(AuthenticationField.AUTHENTICATION_KEY, new Authentication(user, realmRef, null)); | ||
| ingestDocument = new IngestDocument(new HashMap<>(), new HashMap<>()); | ||
| processor = new SetSecurityUserProcessor("_tag", threadContext, "_field", EnumSet.allOf(Property.class)); | ||
| public void testProcessorWithEmptyUserData() throws Exception { |
There was a problem hiding this comment.
(need a mock, because a real user cannot have a null username)
Could we just remove this test case instead?
There was a problem hiding this comment.
Only if we also removed the code from the security processor that deals with nulls.
Which I considered, but I was already down a rabbit hole.
What the processor is doing makes sense - it doesn't make any assumptions about whether a User can have a null principal (although it would also be fine for it to rely on it not being null), so I decided not to bite off even more right now, and just leave it.
| @@ -49,7 +50,7 @@ public User(String username, String[] roles, String fullName, String email, Map< | |||
|
|
|||
| private User(String username, String[] roles, String fullName, String email, Map<String, Object> metadata, boolean enabled, | |||
| User authenticatedUser) { | |||
| this.username = username; | |||
| this.username = Objects.requireNonNull(username); | |||
There was a problem hiding this comment.
nit: I hate when I get NPE in a line that has many calls and it's not quite obvious what threw it so I prefer to add a message to requireNotNull. Not sure if this is worth rerunning CI here though, just a suggestion
There was a problem hiding this comment.
But this line doesn't have many calls. As long as you can link the stacktrace to the code it's clear why this line throughs NPE.
That said, I'd be happy for the whole codebase to have a forbidden API on requireNonNull without a message, but we don't.
Some parts of the User class (e.g. equals/hashCode) assumed that principal could never be null, but the constructor didn't enforce that. This adds a null check into the constructor and fixes a few tests that relied on being able to pass in null usernames. Backport of: elastic#51988
Some parts of the User class (e.g. equals/hashCode) assumed that principal could never be null, but the constructor didn't enforce that. This adds a null check into the constructor and fixes a few tests that relied on being able to pass in null usernames. Backport of: #51988
Some parts of the User class (e.g. equals/hashCode) assumed that
principal could never be null, but the constructor didn't enforce
that.
This adds a null check into the constructor and fixes a few tests that
relied on being able to pass in null usernames.