-
Notifications
You must be signed in to change notification settings - Fork 742
feat(OAuth2): Remove deprecated OAuth2 annotation. Instead Use Java DSL way of OAuth2 #1887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat(OAuth2): Remove deprecated OAuth2 annotation. Instead Use Java DSL way of OAuth2 #1887
Conversation
d670b8d
to
14ae43e
Compare
Looks like there are some config property changes here, so I'd say this is a feature, not a refactor. Please also describe old and new params in the commit message + PR description. |
…a DSL way of OAuth2 Oauth configuration properties are changed for new java way of dsl. Old Config Properties: security: authn: oauth2: enabled: true client: clientId: <client-id> clientSecret: <client-secret> accessTokenUri: https://www.googleapis.com/oauth2/v4/token userAuthorizationUri: https://accounts.google.com/o/oauth2/v2/auth scope: profile email userInfoRequirements: hd: <domain> resource: userInfoUri: https://www.googleapis.com/oauth2/v3/userinfo userInfoMapping: email: email firstName: given_name lastName: family_name provider: GOOGLE New Config Properties: Google: spring: security: oauth2: client: registration: google: client-id: <client-id> client-secret: <client-secret> authorization-grant-type: authorization_code redirect-uri: "https://<your-domain>/login/oauth2/code/google" scope: profile,email,openid client-name: google provider: google: authorization-uri: https://accounts.google.com/o/oauth2/auth token-uri: https://oauth2.googleapis.com/token user-info-uri: https://www.googleapis.com/oauth2/v3/userinfo user-name-attribute: sub Github: spring: security: oauth2: client: registration: userInfoMapping: email: email firstName: '' lastName: name username: login github: client-id: <client-id> client-secret: <client-secret> authorization-grant-type: authorization_code redirect-uri: "https://<your-domain>/login/oauth2/code/github" scope: user,email client-name: github provider: github: authorization-uri: https://github.com/login/oauth/authorize token-uri: https://github.com/login/oauth/access_token user-info-uri: https://api.github.com/user user-name-attribute: login
14ae43e
to
fe51eda
Compare
@dbyron-sf updated the description and commit message with config properties. Please take a look :) |
gate-oauth2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthConfigEnabled.java
Show resolved
Hide resolved
gate-oauth2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthConfigEnabled.java
Show resolved
Hide resolved
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Outdated
Show resolved
Hide resolved
fiatClientConfigurationProperties, | ||
registry); | ||
} | ||
@Autowired private ClientRegistrationRepository clientRegistrationRepository; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this get used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed here
...src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOAuth2UserInfoService.java
Outdated
Show resolved
Hide resolved
gate-oauth2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOAuth2User.java
Outdated
Show resolved
Hide resolved
gate-oauth2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOIDCUser.java
Outdated
Show resolved
Hide resolved
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Outdated
Show resolved
Hide resolved
...2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOIDCUserInfoService.java
Outdated
Show resolved
Hide resolved
"security.oauth2.client.clientId=Spinnaker-Client", | ||
"security.oauth2.resource.userInfoUri=http://localhost/userinfo" | ||
"spring.security.oauth2.client.registration.github.client-id=ec415f229e8f06f6ddb", | ||
"spring.security.oauth2.client.registration.github.client-secret=53dc2b2125d356c652dfb83fbc0d209de4a9f60" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain this section a bit? How does the presence of these two values allow authentication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Spring Security 5, the above properties provided and bean configuration defined in OAuth2SsoConfig.java will configure OAuth2 authentication by defining a OAuth2 client(in this case its github as the client name is github) for login. Here’s how they work:
OAuth2 Client Configuration:
The spring.security.oauth2.client.registration.github.client-id and spring.security.oauth2.client.registration.github.client-secret properties register a GitHub OAuth2 client in Spring Boot. This allows users to authenticate using their GitHub credentials.
Spring Boot Auto-Configuration:
Spring Boot automatically detects these properties and configures an OAuth2LoginConfigurer under the hood. This enables OAuth2 login.
Security Filter Chain Integration:
Spring Security 5 automatically adds the necessary filters (OAuth2LoginAuthenticationFilter) to handle the OAuth2 authentication process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I really meant is: previously the test worked with these fields:
security.oauth2.client.clientId
security.oauth2.resource.userInfoUri
I'd assume that after changing to the new configuration properties, we'd include these fields to remain consistent:spring.security.oauth2.client.registration.github.client-id
spring.security.oauth2.client.provider.github.user-info-uri
But actually, we removed the userInfoUri field and added the client-secret field which wasn't there before. So what is this test actually doing behind the scenes? And what properties (if any) are actually used by this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
userInfoUri doesn't play any special role in the tests, but it was put in the old config to avoid bean creation issues like the ones below:
Caused by: org.springframework.validation.BindException: org.springframework.validation.BeanPropertyBindingResult: 1 errors
Field error in object 'resourceServerProperties' on field 'tokenInfoUri': rejected value [null]; codes [missing.tokenInfoUri.resourceServerProperties.tokenInfoUri,missing.tokenInfoUri.tokenInfoUri,missing.tokenInfoUri.java.lang.String,missing.tokenInfoUri]; arguments []; default message [Missing tokenInfoUri and userInfoUri and there is no JWT verifier key]
at app//org.springframework.boot.autoconfigure.security.oauth2.resource.ResourceServerProperties.doValidate(ResourceServerProperties.java:230)
at app//org.springframework.boot.autoconfigure.security.oauth2.resource.ResourceServerProperties.validate(ResourceServerProperties.java:198)
We don't need to pass the userInfoUri for tests with the new OAuth config.
@ConditionalOnProperty(name = "security.oauth2.client.clientId") | ||
@Slf4j | ||
@SpinnakerAuthConfig | ||
@Conditional(OAuthConfigEnabled.class) | ||
public class OAuth2SsoConfig extends WebSecurityConfigurerAdapter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WebSecurityConfigurerAdapter is deprecated. Are there plans to move to something that's not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We plan to delete the Adapter from the gate as part of the springboot3 upgrade.
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Outdated
Show resolved
Hide resolved
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Outdated
Show resolved
Hide resolved
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Outdated
Show resolved
Hide resolved
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Outdated
Show resolved
Hide resolved
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Outdated
Show resolved
Hide resolved
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Outdated
Show resolved
Hide resolved
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Outdated
Show resolved
Hide resolved
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Show resolved
Hide resolved
gate-oauth2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOAuth2User.java
Show resolved
Hide resolved
...src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOAuth2UserInfoService.java
Show resolved
Hide resolved
gate-oauth2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOIDCUser.java
Show resolved
Hide resolved
gate-oauth2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOIDCUser.java
Outdated
Show resolved
Hide resolved
...2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOIDCUserInfoService.java
Show resolved
Hide resolved
gate-oauth2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOAuth2User.java
Outdated
Show resolved
Hide resolved
…enServices, Made authorities and attributes immutable
...th2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelper.java
Outdated
Show resolved
Hide resolved
...in/java/com/netflix/spinnaker/gate/security/oauth2/provider/GithubProviderTokenServices.java
Outdated
Show resolved
Hide resolved
gate-oauth2/src/main/java/com/netflix/spinnaker/gate/security/oauth2/SpinnakerOIDCUser.java
Show resolved
Hide resolved
...in/java/com/netflix/spinnaker/gate/security/oauth2/provider/GithubProviderTokenServices.java
Show resolved
Hide resolved
… javadoc to getClaims method. Add test coverage to GithubProviderTokenServices
...src/test/java/com/netflix/spinnaker/gate/security/oauth2/OAuthUserInfoServiceHelperTest.java
Outdated
Show resolved
Hide resolved
…penMocks(this) with @ExtendWith(MockitoExtension.class)
* chore(dependencies): Autobump korkVersion * fix(retrofit2/test): deal with the introduction of Retrofit2EncodeCorrectionInterceptor adding beans as necessary. --------- Co-authored-by: root <root@7006d21ea161> Co-authored-by: David Byron <[email protected]>
Co-authored-by: root <root@3d3c5f1ddb64>
Co-authored-by: root <root@37d4e0bf47bb>
Co-authored-by: root <root@37d26b4fc68c>
Co-authored-by: root <root@07aba17c0fd0>
Co-authored-by: root <root@23070a6ef06a>
Oauth configuration properties are changed for new java way of dsl.
Old Config Properties:
New Config Properties:
Google:
Github: