Provide support for OAuth 2.0 #4238

jgrandja · 2017-03-07T22:17:45Z

Provide support for an OAuth 2.0 Client.

* Spring IO Athens-BUILD-SNAPSHOT -> Cairo-BUILD-SNAPSHOT * CGLib 3.1 -> 3.2.5 latest release Issue related to ASM cglib/cglib#20 * AssertJ 2.2.0 -> 3.6.2 latest release * PowerMock 1.6.2 -> 1.6.5 latest release is 1.6.6 but has regression Issue powermock/powermock#717

rwinch · 2017-03-16T17:36:58Z

General

Do all assertions at the top of the constructor rather than assert, assign, assert, assign, ...
I'm confused why AuthorizationRequestAttributes is in oauth2-core and AuthorizationRequestRepository is in oauth2-client

spring-security-oauth2-core

I think that AbstractToken should probably have constructs on it for expiration. If not, we need to have expiration added to RefreshToken
the openid package should be a child of oauth2

spring-security-oauth2-client

Enhance the default log in page to account for oauth2Login similar to how it works with formLogin
We should rethink our URLs. Perhaps something like
- http://localhost:8080/oauth2/clients should be http://localhost:8080/login to follow conventions
- http://localhost:8080/oauth2/authorize/github should be http://localhost:8080/oauth2/authorization/code/github (we requesting an authorization code)
- http://localhost:8080/oauth2/client/github should be http://localhost:8080/oauth2/authorize/code/github (we are trying to authorize a code)
Why does the authentication package contain AuthorizationCodeGrantProcessingFilter vs being in authorization
AuthorizationCodeGrantProcessingFilter use constructor injection and avoid afterPropertiesSet
AuthorizationCodeGrantProcessingFilter try and remove some of the static methods

oauth2login

Please add a README with directions on setup oauth2login Where to go to get client-id, client-secret, etc for each provider.
email is displayed as empty for github client
Running in STS with Buildship gives an error

java.lang.NoSuchMethodError: ch.qos.logback.core.util.Loader.getResourceOccurrenceCount(Ljava/lang/String;Ljava/lang/ClassLoader;)Ljava/util/Set;
	at ch.qos.logback.classic.util.ContextInitializer.multiplicityWarning(ContextInitializer.java:160)
	at ch.qos.logback.classic.util.ContextInitializer.statusOnResourceSearch(ContextInitializer.java:183)
	at ch.qos.logback.classic.util.ContextInitializer.getResource(ContextInitializer.java:141)
	at ch.qos.logback.classic.util.ContextInitializer.findURLOfDefaultConfigurationFile(ContextInitializer.java:130)
	at ch.qos.logback.classic.util.ContextInitializer.autoConfig(ContextInitializer.java:148)
	at org.slf4j.impl.StaticLoggerBinder.init(StaticLoggerBinder.java:85)
	at org.slf4j.impl.StaticLoggerBinder.<clinit>(StaticLoggerBinder.java:55)
	at org.slf4j.LoggerFactory.bind(LoggerFactory.java:150)
	at org.slf4j.LoggerFactory.performInitialization(LoggerFactory.java:124)
	at org.slf4j.LoggerFactory.getILoggerFactory(LoggerFactory.java:412)
	at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:357)
	at org.apache.commons.logging.impl.SLF4JLogFactory.getInstance(SLF4JLogFactory.java:156)
	at org.apache.commons.logging.impl.SLF4JLogFactory.getInstance(SLF4JLogFactory.java:132)
	at org.apache.commons.logging.LogFactory.getLog(LogFactory.java:274)
	at org.springframework.boot.SpringApplication.<clinit>(SpringApplication.java:190)
	at org.springframework.security.samples.OAuth2LoginApplication.main(OAuth2LoginApplication.java:31)

rwinch · 2017-03-16T17:39:41Z

core/src/test/java/org/springframework/security/core/JavaVersionTests.java

@@ -29,7 +29,7 @@
 */
 public class JavaVersionTests {

-	private static final int JDK6_CLASS_VERSION = 50;


All infrastructure changes (i.e. updating to Java 8) should be in a distinct commit

rwinch · 2017-03-16T17:55:17Z

samples/boot/oauth2login/build.gradle

@@ -0,0 +1,19 @@
+apply from: BOOT_SAMPLE_GRADLE


This should only use tabs

rwinch · 2017-03-16T18:38:28Z

...ot/oauth2login/src/main/java/org/springframework/security/samples/config/SecurityConfig.java

+ */
+@Configuration
+@EnableWebSecurity
+public class SecurityConfig extends WebSecurityConfigurerAdapter {


Configuration should be simplified

The customization of ClientRegistration for github needs to go away some how. Perhaps rather than customizing it in the configure any customization should be done in the bean definition itself.

I don't like the boilerplate code here (copy paste, modify for each provider we are leveraging, profiles, etc). Some of this might be necessary in a standard application, but it should not be required in a Boot application. AutoConfiguraiton can live in this sample (within a separate package) until we can get it into boot. In the end we should only have the yml file (the entire SecurityConfig should be removed).

Now that this is getting merged we should add the oauthLogin to HttpSecurity

rwinch · 2017-03-16T19:58:20Z

...-core/src/main/java/org/springframework/security/oauth2/core/ClientAuthenticationMethod.java

@@ -0,0 +1,34 @@
+/*


This probably belongs in oauth2-client rather than oauth2-core

rwinch · 2017-03-16T20:03:49Z

oauth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2Exception.java

+import org.springframework.security.core.Authentication;
+
+/**
+ * Root exception for all OAuth2-related general errors.


This is not true because as the NOTE mentions, OAuth2AuthenticationException is also used. I think we should try to rename this to be more exact. Perhaps OAuth2AuthorizationException (does that work for the use cases it is intended for)?

rwinch · 2017-03-16T21:08:36Z

...springframework/security/oauth2/client/authorization/AuthorizationRequestRedirectFilter.java

+		filterChain.doFilter(request, response);
+	}
+
+	private void obtainAuthorization(HttpServletRequest request, HttpServletResponse response)


Perhaps rename to authorizationRequestRedirect

rwinch · 2017-03-16T21:12:04Z

...springframework/security/oauth2/client/authorization/AuthorizationRequestRedirectFilter.java

+		response.sendError(HttpServletResponse.SC_BAD_REQUEST, failed.getMessage());
+	}
+
+	private String normalizeUri(String uri) {


Rather than implement this yourself, use UriComponentsBuilder

rwinch · 2017-03-16T21:15:45Z

...springframework/security/oauth2/client/authorization/AuthorizationRequestRedirectFilter.java

+
+	@Override
+	public final void afterPropertiesSet() {
+		Assert.notEmpty(this.clientRegistrationRepository.getRegistrations(), "clientRegistrationRepository cannot be empty");


Validate not empty at time of creating the ClientRegistrationRepository

rwinch · 2017-03-16T21:17:48Z

...org/springframework/security/oauth2/client/authorization/AuthorizationRequestRepository.java

+/**
+ * @author Joe Grandja
+ */
+public interface AuthorizationRequestRepository {


Pass in the HttpServletResponse so that cookies could be used

Use a data object rather than having lots of params

rwinch · 2017-03-17T03:32:50Z

.../springframework/security/oauth2/client/authentication/AuthorizationGrantTokenExchanger.java

+/**
+ * @author Joe Grandja
+ */
+public interface AuthorizationGrantTokenExchanger<T extends AuthorizationGrantAuthenticationToken>  {


Why does this need to take a generic argument? Can it just always use AuthorizationCodeGrantAuthenticationToken

jgrandja · 2017-03-20T18:40:41Z

Closing with new PR to follow

jgrandja added this to the 5.0.0.M1 milestone Mar 7, 2017

jgrandja assigned rwinch Mar 7, 2017

jgrandja added the in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) label Mar 7, 2017

jgrandja force-pushed the master branch from fb2f647 to a57b185 Compare March 8, 2017 16:33

jgrandja added 8 commits March 9, 2017 11:39

Add oauth2-core and oauth2-client modules

d2e7d5d

Add order for OAuth2 client filters in FilterComparator

efabc5e

Update maven-compiler-plugin source/target to 1.8

d3da882

Add OAuth2 login sample

31dc3c8

Provide default resolution for OAuth2User.getUserName()

2b57fe2

Catch any URI related exception in ClientRegistration

bcc2f5f

Update poms to 5.0.0.BUILD-SNAPSHOT

67df5c5

jgrandja force-pushed the master branch from a57b185 to 67df5c5 Compare March 9, 2017 17:23

rwinch self-requested a review March 16, 2017 17:37

rwinch reviewed Mar 20, 2017

View reviewed changes

jgrandja closed this Mar 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Provide support for OAuth 2.0 #4238

Provide support for OAuth 2.0 #4238

Uh oh!

jgrandja commented Mar 7, 2017

Uh oh!

rwinch commented Mar 16, 2017 •

edited

Loading

Uh oh!

rwinch Mar 16, 2017

Uh oh!

rwinch Mar 16, 2017

Uh oh!

rwinch Mar 16, 2017

Uh oh!

rwinch Mar 16, 2017

Uh oh!

rwinch Mar 16, 2017

Uh oh!

rwinch Mar 16, 2017

Uh oh!

rwinch Mar 16, 2017

Uh oh!

rwinch Mar 16, 2017

Uh oh!

rwinch Mar 16, 2017

Uh oh!

rwinch Mar 16, 2017

Uh oh!

rwinch Mar 17, 2017

Uh oh!

jgrandja commented Mar 20, 2017

Uh oh!

Uh oh!

Provide support for OAuth 2.0 #4238

Provide support for OAuth 2.0 #4238

Uh oh!

Conversation

jgrandja commented Mar 7, 2017

Uh oh!

rwinch commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General

spring-security-oauth2-core

spring-security-oauth2-client

oauth2login

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja commented Mar 20, 2017

Uh oh!

Uh oh!

rwinch commented Mar 16, 2017 •

edited

Loading