Provide support for OAuth 2.0 Login #4257

jgrandja · 2017-03-21T17:02:15Z

This PR provides support for authenticating using an external OAuth 2.0 or OpenID Connect 1.0 Provider. For example, login using Google, Facebook, GitHub, etc.

This PR addresses #3907

rwinch · 2017-03-22T02:02:07Z

General

Rebase and resolve build.gradle conflict
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
rather than final methods make class final

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

I'm submitting this for now and will continue the review later. This way you can iterate on it.

rwinch · 2017-03-22T01:11:50Z

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

+												ClientRegistrationRepository clientRegistrationRepository,
+												AuthorizationRequestUriBuilder authorizationUriBuilder) {
+
+		Assert.notNull(filterProcessingUri, "filterProcessingUri cannot be null");


For consistency with the Spring code base...Move all assertions to the top and then assign. Do not do assert, assign, assert, assign

rwinch · 2017-03-22T01:13:17Z

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

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


Generally, try to avoid afterProperties set unless you have to because validation only happens if people remember to invoke this method. Have the clientRegistrationRepository constructor validate that it has registrations and remove this method.

rwinch · 2017-03-22T01:13:58Z

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

+ *
+ * @author Joe Grandja
+ */
+public class AuthorizationRequestRedirectFilter extends OncePerRequestFilter {


Rather than all the final methods make the whole class final

rwinch · 2017-03-22T01:22:55Z

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.


As explained in the note this isn't really true. The name of the class needs refined too

rwinch · 2017-03-22T01:25:57Z

...g/springframework/security/oauth2/client/authorization/InvalidClientIdentifierException.java

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


The only place this is used is within a private method. I think this should be removed

rwinch · 2017-03-22T01:45:15Z

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

+		try {
+			redirectUri = this.authorizationUriBuilder.build(authorizationRequestAttributes);
+		} catch (URISyntaxException ex) {
+			logger.error("An error occurred building the Authorization Request: " + ex.getMessage(), ex);


Do not swallow exceptions. If this code is kept, the user would just get an IllegalArgumentException on 134 but wouldn't know it was due to the URISyntaxException. Instead we should let a RuntimeException be thrown. See comment on the builder api too

rwinch · 2017-03-22T01:46:22Z

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

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


Can you use UriComponentsBuilder instead of writing the logic yourself?

rwinch · 2017-03-22T02:11:03Z

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

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


Is there a reason this is generic vs just always using AuthorizationCodeGrantAuthenticationToken

There will be different implementations of AuthorizationGrantTokenExchanger for each of the grant types authorization_code, client_credentials and resource_owner_password grant types.

For example, the client_credentials grant type would be ClientCredentialsTokenExchanger which would be associated with it's specific type of AuthorizationGrantAuthenticationToken, for example, ClientCredentialsAuthenticationToken

rwinch · 2017-03-22T02:15:42Z

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

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


This API be combined with AuthorizationGrantTokenExchanger?

I don't feel we should combine these 2 API's.
AuthorizationRequestUriBuilder is used for authorization_code and implicit grant types whereas AuthorizationGrantTokenExchanger is used for authorization_code, client_credentials and resource_owner_password grant types.

We also have only one default implementation for AuthorizationRequestUriBuilder that will serve for authorization_code and implicit grant types. We will need different implementations of AuthorizationGrantTokenExchanger for each of authorization_code, client_credentials and resource_owner_password grant types.

rwinch · 2017-03-22T02:17:14Z

...ngframework/security/oauth2/client/authentication/AuthorizationGrantAuthenticationToken.java

+/**
+ * @author Joe Grandja
+ */
+public abstract class AuthorizationGrantAuthenticationToken extends AbstractAuthenticationToken {


We don't need this abstract class. We can extract it later if necessary. For now move everything into AuthorizationCodeGrantAuthenticationToken

See comment

kakawait · 2017-03-27T11:56:39Z

@jgrandja @rwinch Stupid question but is not actually the goal of Spring Social?

I mean with generic ApiAdapter that could work with any providers (like Oauth2ApiAdapter) and SpringSocialConfigurer.

jgrandja · 2017-03-27T13:33:11Z

Hi @kakawait . As mentioned in #3907, OAuth-specific code is spread out in a few projects so we are looking to provide a uniform lower-level base that all projects can leverage. The goal is to have the other projects, for example Spring Social, leverage the lower-level protocol flow logic that will be provided within Spring Security.

kakawait · 2017-03-27T14:25:09Z

@jgrandja Great news, because I had the same issue when I wanted to add social login on my own UAA implementation, I found multiple way to achieve:

Spring Social (http://docs.spring.io/spring-social/docs/1.1.4.RELEASE/reference/htmlsingle/#section_signin)
Spring Security Oauth2 (https://spring.io/guides/tutorials/spring-boot-oauth2/)
And other custom dirty way

I personally choose Spring Social because it supports OAuth 1 in addition to OAuth 2 (not because I'm fan of OAuth 1 but unfortunately Twitter still on OAuth 1) .

Thus I think I will not plan to support an uniform lower-level base for OAuth 1? I'm ok with that since OAuth 1 is dying.

jgrandja · 2017-03-28T15:20:40Z

@rwinch All requested changes have been completed except for the following.

When I login using my GitHub client, I am able to see the email in the User Info page. I'm not sure why it's not showing up when you login with your client.

I still need to look at the STS/Buildship issue.

I'll work on the README for the sample next.

jgrandja · 2017-03-28T15:21:13Z

@rwinch I think the URI naming convention you suggested makes sense.
Let's go with that for now.
The URI's have been changed as follows::

/oauth2/authorization/code/github for initiating the Authorization Code Request

/oauth2/authorize/code/github for handling the Authorization Code Response (which is essentially the registered redirect_uri)

rwinch · 2017-03-28T18:46:08Z

OAuth is now first class so we shouldn't need to use apply(..). Add HttpSecurity.oauth2Login()
change URI to String across the board so that we can do things like variables in the URIs and client side load balancing

rwinch · 2017-03-28T18:37:08Z

samples/boot/oauth2login/src/main/resources/application.yml

+      google:
+        client-id: your-app-client-id
+        client-secret: your-app-client-secret
+        redirect-uri: http://localhost:8080/oauth2/authorize/code/google


We should be able to default the redirect URI.

We know the host at the time the request comes in. We also know the client alias at this time (it is used to look up the details).

Resolved via 4d94134

rwinch · 2017-03-28T18:41:13Z

...framework/boot/autoconfigure/security/oauth2/client/ClientRegistrationAutoConfiguration.java

+		}
+	}
+
+	private static class OAuth2ClientCondition extends SpringBootCondition implements ConfigurationCondition {


I think you can just remove this and it's subclasses and use @ConditionalOnProperty directly on the configuration.

Resolved via 68f4891

rwinch · 2017-03-28T18:48:35Z

...java/org/springframework/boot/autoconfigure/security/oauth2/client/OktaClientProperties.java

+public class OktaClientProperties extends ClientRegistrationProperties {
+	public static final String CLIENT_NAME = "Okta";
+	public static final String CLIENT_ALIAS = "okta";
+	public static final String AUTHORIZATION_URI = "https://your-subdomain.oktapreview.com/oauth2/v1/authorize";


Only default values that reduce the amount of configuration the user needs to provide. Defaults that are incorrect should be removed.

Resolved via 68f4891

rwinch · 2017-03-28T18:51:59Z

...framework/boot/autoconfigure/security/oauth2/client/ClientRegistrationAutoConfiguration.java

+@Configuration
+@ConditionalOnWebApplication
+@ConditionalOnClass(ClientRegistration.class)
+public class ClientRegistrationAutoConfiguration {


I'd like to see this support creating a ClientRegistration from arbitrary client aliases rather than just specific aliases. The client aliases with a convention (i.e. google) simply provide defaults. The client aliases that are unknown the user should still only need to provide environment entries (no need to create the bean)

Resolved via 68f4891

rwinch · 2017-03-28T18:57:03Z

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

+ * @author Joe Grandja
+ */
+@EnableWebSecurity
+public class SecurityConfig extends WebSecurityConfigurerAdapter {


SecurityConfig should not be required. Instead we should allow the user to provide the java class and URL to map the user info in the environment.

Resolved via 8244855

jgrandja · 2017-04-03T13:56:20Z

@rwinch In reference to your comment...

email is displayed as empty for github client

It looks like your email address is set as private.
The GitHub documentation for the Get Single User endpoint states the following:

Note: The returned email is the user's publicly visible email address (or null if the user has not specified a public email address in their profile).

jgrandja · 2017-04-04T14:42:57Z

@rwinch I changed the member types from URI to String for most cases with the exception of OAuth2LoginConfigurer.UserInfoEndpointConfig.userInfoTypeMapping and MappableUserInfoType.
I think it makes sense to keep the type as URI in these cases.

rwinch · 2017-04-13T16:34:31Z

...main/java/org/springframework/security/oauth2/core/protocol/ResponseAttributesExtractor.java

+/**
+ * @author Joe Grandja
+ */
+public final class ResponseAttributesExtractor {


I don't think this should be static classes. Instead perhaps consider implementing the Converter (or similar) API.

Resolved via b876eda

rwinch · 2017-04-13T16:39:19Z

...main/java/org/springframework/security/oauth2/core/protocol/ResponseAttributesExtractor.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.security.oauth2.core.protocol;


I don't think this belongs in the core protocol package. There will be a reactive implementation of the OAuth support that does not support HttpServletRequest. Rather I think we should create a new package that contains converters used for the web

Resolved via b876eda

rwinch · 2017-04-13T19:21:23Z

It would be good to also place the oauth2 stuff in a folder so all oauth2 stuff is within it.

rwinch · 2017-04-13T19:28:18Z

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

+	private final String uri;
+	private final HttpStatus statusCode;
+
+	public enum ErrorCode {


I struggle with this being an Enum because I don't think we will ever be able to guarantee all the errors are enumerated (which is what an Enum is). We could potentially allow OAuth2Error to accept an ErrorCode interface and then create some constants for well known errors

Resolved via 55f15f6

rwinch · 2017-04-13T19:34:46Z

...k/security/oauth2/client/authentication/AuthorizationCodeAuthenticationProcessingFilter.java

+	public Authentication attemptAuthentication(HttpServletRequest request, HttpServletResponse response)
+			throws AuthenticationException, IOException, ServletException {
+
+		if (this.isAuthorizationCodeErrorResponse(request)) {


Extract to leverage a converter API and if it converts to OAuth2Error then throw exception

Resolved via b876eda

Fixes spring-projectsgh-3907

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

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

jgrandja assigned rwinch Mar 21, 2017

jgrandja requested a review from rwinch March 21, 2017 17:02

rwinch requested changes Mar 22, 2017

View reviewed changes

jgrandja force-pushed the oauth2-support branch from 367f7e9 to dfca383 Compare March 23, 2017 02:23

rwinch reviewed Mar 28, 2017

View reviewed changes

jgrandja force-pushed the oauth2-support branch from a6cd297 to 27d0c45 Compare April 11, 2017 16:45

rwinch reviewed Apr 13, 2017

View reviewed changes

jgrandja force-pushed the oauth2-support branch from 7105b99 to 3785a2f Compare April 18, 2017 20:54

jgrandja mentioned this pull request Apr 20, 2017

What is the policy for pull requests for this project? spring-attic/spring-security-oauth#1044

Closed

jgrandja force-pushed the oauth2-support branch 2 times, most recently from 5e47a7a to 1cba5eb Compare April 27, 2017 20:18

Add support for OAuth 2.0 Login

829c386

Fixes spring-projectsgh-3907

jgrandja force-pushed the oauth2-support branch from 1cba5eb to 829c386 Compare April 28, 2017 15:31

jgrandja merged commit 829c386 into spring-projects:master Apr 28, 2017

jgrandja deleted the oauth2-support branch July 17, 2018 18:10

Provide support for OAuth 2.0 Login #4257

Provide support for OAuth 2.0 Login #4257

Conversation

jgrandja commented Mar 21, 2017

rwinch commented Mar 22, 2017 • edited by jgrandja

General

spring-security-oauth2-core

spring-security-oauth2-client

oauth2login

rwinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja Mar 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakawait commented Mar 27, 2017 • edited

jgrandja commented Mar 27, 2017

kakawait commented Mar 27, 2017

jgrandja commented Mar 28, 2017

jgrandja commented Mar 28, 2017

rwinch commented Mar 28, 2017 • edited by jgrandja

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja commented Apr 3, 2017

jgrandja commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwinch commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwinch commented Mar 22, 2017 •

edited by jgrandja

jgrandja Mar 29, 2017 •

edited

kakawait commented Mar 27, 2017 •

edited

rwinch commented Mar 28, 2017 •

edited by jgrandja