Implement Token Revocation Endpoint by babuv2 · Pull Request #84 · spring-projects/spring-authorization-server

babuv2 · 2020-06-05T11:20:26Z

Fixes gh-83

pivotal-issuemaster · 2020-06-05T11:20:28Z

@babuv2 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2020-06-05T11:22:46Z

@babuv2 Thank you for signing the Contributor License Agreement!

jgrandja

Thanks for the PR @babuv2! Overall, this looks very good.

Please see review comments for requested changes.

jgrandja · 2020-06-09T01:44:25Z

...gframework/security/oauth2/server/authorization/web/OAuth2TokenRevocationEndpointFilter.java

+	 * The default endpoint {@code URI} for token revocation request.
+	 */
+	public static final String DEFAULT_TOKEN_REVOCATION_ENDPOINT_URI = "/oauth2/revoke";
+	public static final String TOKEN_TYPE_HINT = "token_type_hint";


Change modifier to private for TOKEN_TYPE_HINT and TOKEN

jgrandja · 2020-06-09T01:50:55Z

...gframework/security/oauth2/server/authorization/web/OAuth2TokenRevocationEndpointFilter.java

+			MultiValueMap<String, String> parameters = OAuth2EndpointUtils.getParameters(request);
+
+			// client_id (REQUIRED)
+			String clientId = parameters.getFirst(OAuth2ParameterNames.CLIENT_ID);


Please remove client_id as it's not a valid parameter for a revocation request.

jgrandja · 2020-06-09T01:54:25Z

...ity/oauth2/server/authorization/authentication/OAuth2TokenRevocationAuthenticationToken.java

+		this.credentials = "";
+	}
+
+	public OAuth2TokenRevocationAuthenticationToken(String token,


Remove this constructor since client_id is not a valid parameter.

jgrandja · 2020-06-09T01:55:09Z

...ity/oauth2/server/authorization/authentication/OAuth2TokenRevocationAuthenticationToken.java

+	private RegisteredClient registeredClient;
+
+	public OAuth2TokenRevocationAuthenticationToken(String token,
+			Authentication clientPrincipal, String tokenTypeHint) {


Add @Nullable for tokenTypeHint arg

jgrandja · 2020-06-09T01:57:39Z

...ity/oauth2/server/authorization/authentication/OAuth2TokenRevocationAuthenticationToken.java

+public class OAuth2TokenRevocationAuthenticationToken extends AbstractAuthenticationToken {
+	private static final long serialVersionUID = Version.SERIAL_VERSION_UID;
+	private final String tokenTypeHint;
+	private Authentication authentication;


Rename to clientPrincipal

jgrandja · 2020-06-09T02:11:44Z

.../oauth2/server/authorization/authentication/OAuth2TokenRevocationAuthenticationProvider.java

+		OAuth2TokenRevocationAuthenticationToken authenticationToken = (OAuth2TokenRevocationAuthenticationToken)
+				authentication;
+
+		String clientId = authenticationToken.getPrincipal().toString();


We should assert on OAuth2ClientAuthenticationToken. Take a look at OAuth2AuthorizationCodeAuthenticationProvider and apply the same check for client auth.

jgrandja · 2020-06-09T02:16:43Z

.../oauth2/server/authorization/authentication/OAuth2TokenRevocationAuthenticationProvider.java

+			throw new OAuth2AuthenticationException(new OAuth2Error(OAuth2ErrorCodes.INVALID_GRANT));
+		}
+
+		if (token.equals(authorization.getAttribute(OAuth2AuthorizationAttributeNames.CODE))) {


It's not clear to me what this check is for?

This is not needed Joe. I have removed it. As of now we ignore the hint and only look for access token

jgrandja · 2020-06-09T02:19:05Z

.../oauth2/server/authorization/authentication/OAuth2TokenRevocationAuthenticationProvider.java

+		OAuth2Authorization authorization = null;
+		TokenType tokenType = new TokenType(tokenTypeHint);
+
+		if (TokenType.REFRESH_TOKEN.equals(tokenType) || TokenType.ACCESS_TOKEN.equals(tokenType)) {


Apologies as I did mention to allow the capability to revoke a refresh token but at the moment we don't have refresh token grant feature. Therefore, please remove all references to refresh token. We will add this in after we have refresh token feature implemented.

For now, we can assume the revocation request is for access tokens only. Any other token type hint should return 400 for now.

Joe the spec says that hint can be any value. if it is of value access_token or refresh_token the authorization server CAN use it to speed up its lookup. If any other value of token_type_hint is give it should not lead to any error

"An invalid token type hint value is ignored by the authorization
server and does not influence the revocation response."

jgrandja · 2020-06-09T02:25:53Z

...ava/org/springframework/security/oauth2/server/authorization/OAuth2AuthorizationService.java

+	 * @param token the token credential
+	 * @return the {@link OAuth2Authorization} if found, otherwise {@code null}
+	 */
+	OAuth2Authorization findByToken(String token);


I'm wondering if we need to add this operation? Instead we could consider changing findByTokenAndTokenType to String token, @Nullable TokenType tokenType? I think the name would change to. Thoughts?

Just looking up for access_token for now

jgrandja · 2020-06-09T02:28:36Z

...ava/org/springframework/security/oauth2/server/authorization/OAuth2AuthorizationService.java

+	 *
+	 * @param authorization {@link OAuth2Authorization} to be invalidated
+	 */
+	void invalidate(OAuth2Authorization authorization);


I'm not sure we should add this operation here. We will need the ability to revoke access tokens, refresh tokens and possibly authorization codes. This operation doesn't allow us to specify which token to revoke. I wonder if we should have a revoke on OAuth2Authorization instead. Thoughts?

@jgrandja Joe this is the only place where I have a doubt, you mean to say that have a revoke method on the authorization and then we should do
authorization.revoke() and then authorizationService.save(authorization)? Because in that case with our current inmemory implementation one more record will get inserted with revoked status. Instead I thought we can have an invalidate method on the service and let the implementation decide how to handle the revoke. In this default inmemory implementation

we remove the authorization and then

add it in revoked status
Other implementations may remove it from DB and handle other revocations etc.
We can discuss this further Joe

babuv2 · 2020-06-09T16:06:57Z

@jgrandja Joe, Thanks for all the inputs
I am working on all the changes that is requested. I will try to get them done by tomorrow.

jgrandja · 2020-06-15T20:34:05Z

@babuv2 Regarding our discussion around renaming OAuth2AuthorizationService to OAuth2AuthorizationRepository, I'll hold off on this for now until we merge this PR. I'd like to see how things look after we merge and then determine if the rename makes sense or not.

babuv2 · 2020-06-17T04:58:17Z

@babuv2 Regarding our discussion around renaming OAuth2AuthorizationService to OAuth2AuthorizationRepository, I'll hold off on this for now until we merge this PR. I'd like to see how things look after we merge and then determine if the rename makes sense or not.

@jgrandja Joe should I make any more changes to this PR?

jgrandja · 2020-06-17T12:24:34Z

@babuv2 Yes. Do you recall our discussion around OAuth2AuthorizationService.invalidate()? As per comment, this operation does not specify which token to revoke. An authorization code, access token and refresh token may be revoked and the logic will differ depending on which token type it is.

We also agreed that we would extract OAuth2AuthorizationService.invalidate() to a new interface, named OAuth2TokenRevocationService.revoke(...). The OAuth2AuthorizationService will likely be renamed (later) to OAuth2AuthorizationRepository as it will serve mainly for CRUD operations. The OAuth2TokenRevocationService implementation would use the OAuth2AuthorizationService to lookup the token to revoke and then update the "status" of the token before it saves it back.

Fixes spring-projectsgh-83

babuv2 · 2020-06-23T06:22:07Z

@jgrandja Joe, I have added the revocation service implementation

jgrandja · 2020-06-23T13:05:39Z

Thanks @babuv2. I will try to review today if not first thing tomorrow.

jgrandja

Thanks for the updates @babuv2. Please see review comments.

jgrandja · 2020-06-24T14:43:51Z

...pringframework/security/oauth2/server/authorization/DefaultOAuth2TokenRevocationService.java

+		final OAuth2Authorization authorization = this.authorizationService.findByTokenAndTokenType(token, tokenType);
+		if (authorization != null) {
+			final OAuth2Authorization revokedAuthorization = OAuth2Authorization.from(authorization)
+					.revoked(true).build();


OAuth2Authorization.revoked does not indicate which token is revoked so this member will not work.

Let me draw out a scenario:

Client receives code in the Authorization Response

the code is leaked to a malicious client

the malicious client attempts to obtain an access token using the code and the Authorization Server must detect this and revoke the code

any further attempts of using the code should be rejected. As well, the "real" client will not be able to obtain the access token since the code was revoked. Instead the client will have to restart the flow.

So we do need some kind of construct in OAuth2Authorization that maps a token (code, access token or refresh token) to some extra metadata. One attribute would be a revoked flag. We'll also likely want to know the time it was revoked. We may store additional attributes related to the token, eg. the code can only be used once.

Give this some further thought and the type of construct we'll need so it can support storing metadata/attributes related to a specific token.

@babuv2 I've been giving this some further thought and I'd like to propose the following class as a holder of token metadata. The name maps nicely as referenced in the Token Introspection RFC:

This specification defines a protocol that allows authorized
protected resources to query the authorization server to determine
the set of metadata for a given token that was presented to them by
an OAuth 2.0 client. This metadata includes whether or not the token
is currently active (or if it has expired or otherwise been revoked)...

public class OAuth2TokenMetadata<T extends AbstractOAuth2Token> implements Serializable { private static final long serialVersionUID = Version.SERIAL_VERSION_UID; private final T token; private boolean revoked; private Instant revokedAt; public OAuth2TokenMetadata(T token) { } public T getToken() { } public boolean isExpired() { } public boolean isRevoked() { } public Instant getRevokedAt() { } public boolean isActive() { } }

How about we start with this and see how it shapes up. Sounds good?

jgrandja · 2020-06-24T14:56:21Z

...gframework/security/oauth2/server/authorization/web/OAuth2TokenRevocationEndpointFilter.java

+	private final AntPathRequestMatcher revocationEndpointMatcher;
+
+	private final Converter<HttpServletRequest, Authentication> tokenRevocationAuthenticationConverter =
+			new OAuth2TokenRevocationEndpointFilter.TokenRevocationAuthenticationConverter();


Outer class reference OAuth2TokenRevocationEndpointFilter can be removed.

jgrandja · 2020-06-24T14:58:48Z

...gframework/security/oauth2/server/authorization/web/OAuth2TokenRevocationEndpointFilter.java

+	 * The default endpoint {@code URI} for token revocation request.
+	 */
+	public static final String DEFAULT_TOKEN_REVOCATION_ENDPOINT_URI = "/oauth2/revoke";
+	private static final String TOKEN_TYPE_HINT = "token_type_hint";


Please move TOKEN and TOKEN_TYPE_HINT to TokenRevocationAuthenticationConverter

jgrandja · 2020-06-24T15:00:07Z

...gframework/security/oauth2/server/authorization/web/OAuth2TokenRevocationEndpointFilter.java

+		this.errorHttpResponseConverter.write(error, null, httpResponse);
+	}
+
+	private static OAuth2AuthenticationException throwError(String errorCode, String parameterName) {


Should return void

jgrandja · 2020-06-24T15:01:16Z

...gframework/security/oauth2/server/authorization/web/OAuth2TokenRevocationEndpointFilter.java

+			}
+
+			// token_type_hint (OPTIONAL)
+			String tokenTypeHint = parameters.getFirst(TOKEN_TYPE_HINT);


We should validate that TOKEN_TYPE_HINT was not supplied with multiple values

jgrandja · 2020-06-24T15:04:03Z

...gframework/security/oauth2/server/authorization/web/OAuth2TokenRevocationEndpointFilter.java

+		try {
+			Authentication tokenRevocationRequestAuthentication =
+					this.tokenRevocationAuthenticationConverter.convert(request);
+			this.authenticationManager.authenticate(tokenRevocationRequestAuthentication);


We should explicitly set status to 200 - even though this will default. However, it will ensure the response is written and therefore will commit.

jgrandja · 2020-06-24T15:05:16Z

...ity/oauth2/server/authorization/authentication/OAuth2TokenRevocationAuthenticationToken.java

+import java.util.Collections;
+
+/**
+ * An {@link Authentication} implementation used for OAuth 2.0 Client Authentication.


Typo in javadoc

jgrandja · 2020-10-28T21:32:19Z

Thanks for the PR @babuv2 ! This is now merged along with a follow-up polish commit to complete the feature.

jgrandja self-assigned this Jun 9, 2020

jgrandja added the type: enhancement A general enhancement label Jun 9, 2020

jgrandja added this to the 0.0.1 milestone Jun 9, 2020

jgrandja requested changes Jun 9, 2020

View reviewed changes

babuv2 force-pushed the topic/babuv/tokenRevocationEndpoint branch from 4836bdb to 686115c Compare June 11, 2020 11:35

babuv2 requested a review from jgrandja June 11, 2020 11:45

Implement Token Revocation Endpoint

dcd399e

Fixes spring-projectsgh-83

babuv2 force-pushed the topic/babuv/tokenRevocationEndpoint branch from 686115c to dcd399e Compare June 23, 2020 06:15

jgrandja requested changes Jun 24, 2020

View reviewed changes

jgrandja modified the milestones: 0.0.1, 0.0.2 Aug 19, 2020

jgrandja modified the milestones: 0.0.2, 0.0.3 Sep 2, 2020

jgrandja added a commit that referenced this pull request Oct 28, 2020

Polish gh-84

b7ddb83

jgrandja closed this Oct 28, 2020

jgrandja added the status: duplicate A duplicate of another issue label Oct 28, 2020

jgrandja added a commit that referenced this pull request Nov 3, 2020

Polish tests gh-84

df8793c

doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023

Polish spring-projectsgh-84

771308f

doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023

Polish tests spring-projectsgh-84

a7c4c81

Conversation

babuv2 commented Jun 5, 2020

Uh oh!

pivotal-issuemaster commented Jun 5, 2020

Uh oh!

pivotal-issuemaster commented Jun 5, 2020

Uh oh!

jgrandja left a comment

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!

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!

babuv2 Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

babuv2 commented Jun 9, 2020

Uh oh!

jgrandja commented Jun 15, 2020

Uh oh!

babuv2 commented Jun 17, 2020

Uh oh!

jgrandja commented Jun 17, 2020

Uh oh!

babuv2 commented Jun 23, 2020

Uh oh!

jgrandja commented Jun 23, 2020

Uh oh!

jgrandja left a comment

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

babuv2 Jun 11, 2020 •

edited

Loading