Updates for #6911 and #6918 #1131
Conversation
Spring Security uses an anonymous principal of "anonymousUser" by default which does not match the rest of Spinnaker where the anonymous user is expected to have the username "anonymous". This ensures that fiat-api code consistently uses the same anonymous principal. Related to spinnaker/spinnaker#6918
| "anonymous", | ||
| "anonymous", | ||
| AuthorityUtils.createAuthorityList("ROLE_ANONYMOUS"))); | ||
| SpinnakerUsers.ANONYMOUS, |
There was a problem hiding this comment.
notes for reviewers -- the new code looks like it does exactly the same thing as the old code, so this is "just" a refactor.
| @@ -143,6 +144,8 @@ protected void configure(HttpSecurity http) throws Exception { | |||
| .exceptionHandling() | |||
| .and() | |||
| .anonymous() | |||
| // match the same anonymous userid as expected elsewhere | |||
There was a problem hiding this comment.
How is this different from .anonymous() above? Can you write a test that demonstrates this?
There was a problem hiding this comment.
The default settings in this DSL set the principal to the string anonymousUser. The DSL is tricky to test since it's mostly a DSL for setting up beans.
There was a problem hiding this comment.
So using the updated Spring Security DSL, this whole method would look more like the following:
http.servletApi(Customizer.withDefaults())
.exceptionHandling(Customizer.withDefaults())
.anonymous(anonymous -> anonymous.principal(SpinnakerUsers.ANONYMOUS))
.addFilterBefore(
new FiatAuthenticationFilter(fiatStatus, authenticationConverter),
AnonymousAuthenticationFilter.class);There was a problem hiding this comment.
I didn't see before. .principal(SpinnakerUsers.ANONYMOUS) modifies how .anonymous() works...it's not another mechanism in addition to it.
There was a problem hiding this comment.
I'll figure out some sort of test.
There was a problem hiding this comment.
Turns out testing this is futile. I've extracted some constants to make this more consistent.
This extracts some constants for use in configuring anonymous authentication.
|
@Mergifyio update |
✅ Branch has been successfully updated |
Contains two small commits separated by the two relevant issues:
anonymoususername spinnaker#6918 (small change which required similar dependency updates)