diff --git a/Source/Plugins/Core/com.equella.core/resources/lang/i18n-resource-centre.properties b/Source/Plugins/Core/com.equella.core/resources/lang/i18n-resource-centre.properties index 51fbfc6be4..3aec13df66 100644 --- a/Source/Plugins/Core/com.equella.core/resources/lang/i18n-resource-centre.properties +++ b/Source/Plugins/Core/com.equella.core/resources/lang/i18n-resource-centre.properties @@ -2981,6 +2981,7 @@ oauth.error.noaccess=You do not have the required permissions to view this page oauth.error.parammandatory=Parameter ''{0}'' must be supplied oauth.error.tokennotfound=Unrecognised token value oauth.error.usernotfound=Impersonated user ''{0}'' does not exist +oauth.error.validationfailed=Client credential validation failed oauth.filter.error.mustbehttps=This request must be made over HTTPS oauth.logon.title=Client requesting access oauth.lti.error.client.disabled=Internal error\: LTI consumer is disabled. Please check LTI consumer configuration diff --git a/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/OAuthWebConstants.java b/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/OAuthWebConstants.java index 6aae5645fd..fcbd9cd254 100644 --- a/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/OAuthWebConstants.java +++ b/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/OAuthWebConstants.java @@ -53,6 +53,8 @@ public final class OAuthWebConstants { public static final String OAUTH_TOKEN_URL = "oauth/access_token"; public static final String HEADER_AUTHORIZATION = "Authorization"; + + public static final String BASIC_AUTHORIZATION_PREFIX = "basic "; public static final String HEADER_X_AUTHORIZATION = "X-Authorization"; public static final String AUTHORIZATION_ACCESS_TOKEN = "access_token"; public static final String AUTHORIZATION_BEARER = "Bearer"; diff --git a/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/service/OAuthWebService.java b/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/service/OAuthWebService.java index 344f295f42..c71673e125 100644 --- a/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/service/OAuthWebService.java +++ b/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/service/OAuthWebService.java @@ -103,11 +103,12 @@ List> getOauthSignatureParams( IOAuthClient getByClientIdAndRedirectUrl(String clientId, String redirectUrl); /** - * Revoke a token from the current Institution. + * Revoke a token if the token was generated by the provided client. * * @param token Token to be revoked - may be invalid already + * @param clientId ID of the Client where the token was generated */ - void revokeToken(String token); + void revokeTokenForClient(String token, String clientId); class AuthorisationDetails { private String userId; diff --git a/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/service/OAuthWebServiceImpl.java b/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/service/OAuthWebServiceImpl.java index 3ac4ec4531..d966b5c158 100644 --- a/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/service/OAuthWebServiceImpl.java +++ b/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/service/OAuthWebServiceImpl.java @@ -26,6 +26,7 @@ import com.tle.common.ExpiringValue; import com.tle.common.institution.CurrentInstitution; import com.tle.common.oauth.beans.OAuthClient; +import com.tle.common.oauth.beans.OAuthToken; import com.tle.common.usermanagement.user.UserState; import com.tle.common.usermanagement.user.valuebean.UserBean; import com.tle.core.encryption.EncryptionService; @@ -255,9 +256,15 @@ public IOAuthClient getByClientIdAndRedirectUrl(String clientId, String redirect } @Override - public void revokeToken(String token) { - LOGGER.info("OAUTH token revoked: " + token); - oauthService.deleteToken(token); + public void revokeTokenForClient(String token, String clientId) { + Optional.ofNullable(oauthService.getToken(token)) + .map(OAuthToken::getClient) + .filter(c -> c.getClientId().equals(clientId)) + .ifPresent( + (client) -> { + oauthService.deleteToken(token); + LOGGER.info("OAUTH token revoked: " + token); + }); } /** Throw an exception if any SINGLE_PARAMETERS occur repeatedly. */ diff --git a/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/servlet/OAuthTokenServlet.java b/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/servlet/OAuthTokenServlet.java index 94e6c46e6b..0ed2fae856 100644 --- a/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/servlet/OAuthTokenServlet.java +++ b/Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/servlet/OAuthTokenServlet.java @@ -20,6 +20,7 @@ import com.dytech.edge.exceptions.WebException; import com.tle.common.Check; +import com.tle.core.encryption.EncryptionService; import com.tle.core.guice.Bind; import com.tle.core.oauth.OAuthConstants; import com.tle.web.oauth.OAuthException; @@ -32,6 +33,8 @@ import com.tle.web.oauth.service.OAuthWebService.AuthorisationDetails; import java.io.IOException; import java.time.Instant; +import java.util.Base64; +import java.util.Optional; import javax.inject.Inject; import javax.inject.Singleton; import javax.servlet.http.HttpServletRequest; @@ -50,6 +53,8 @@ public class OAuthTokenServlet extends AbstractOAuthServlet { @Inject private OAuthWebService oauthWebService; + @Inject private EncryptionService encryptionService; + @Override protected void doService(HttpServletRequest request, HttpServletResponse response) throws WebException { @@ -165,14 +170,49 @@ else if (isClientCredentials) { } } + private Optional validateCredentials(String auth) { + if (auth.toLowerCase().startsWith(OAuthWebConstants.BASIC_AUTHORIZATION_PREFIX)) { + // Remove the prefix `Basic `. + String decoded = new String(Base64.getDecoder().decode(auth.substring(6))); + // Use index of the last colon because a Client ID may have colons. + int delimiterIndex = decoded.lastIndexOf(":"); + if (delimiterIndex > 0) { + String clientId = decoded.substring(0, delimiterIndex); + String clientSecret = decoded.substring(delimiterIndex + 1); + + IOAuthClient client = oauthWebService.getByClientIdOnly(clientId); + + return Optional.ofNullable(client) + .filter(c -> encryptionService.decrypt(c.getClientSecret()).equals(clientSecret)) + .map(IOAuthClient::getClientId); + } + } + + return Optional.empty(); + } + /** - * According to the spec for OAuth 2.0 - * Token Revocation >, the response code should be 200 regardless whether the token is valid - * or not. Hence, token validation is not needed. However, if a token is not present in the + * Revoke an Oauth token. Client credentials must be validated first. If the validation fails, + * return a 401 error. + * + *

According to the spec for OAuth + * 2.0 Token Revocation >, the response code should be 200 regardless whether the token is + * valid or not. Hence, token validation is not needed. However, if a token is not present in the * request payload, return a 400 error. */ private void revokeToken(HttpServletRequest request) { - String token = getParameter(request, TOKEN_PARAM, true); - oauthWebService.revokeToken(token); + Optional.ofNullable(request.getHeader(OAuthWebConstants.HEADER_AUTHORIZATION)) + .flatMap(this::validateCredentials) + .ifPresentOrElse( + (client) -> { + String token = getParameter(request, TOKEN_PARAM, true); + oauthWebService.revokeTokenForClient(token, client); + }, + () -> { + throw new OAuthException( + HttpServletResponse.SC_UNAUTHORIZED, + OAuthConstants.ERROR_INVALID_REQUEST, + text("oauth.error.validationfailed")); + }); } } diff --git a/autotest/OldTests/src/test/java/com/tle/webtests/test/webservices/rest/OAuthTest.java b/autotest/OldTests/src/test/java/com/tle/webtests/test/webservices/rest/OAuthTest.java index 9c00d670fb..3f6bd566f3 100644 --- a/autotest/OldTests/src/test/java/com/tle/webtests/test/webservices/rest/OAuthTest.java +++ b/autotest/OldTests/src/test/java/com/tle/webtests/test/webservices/rest/OAuthTest.java @@ -14,6 +14,7 @@ import com.tle.webtests.test.users.TokenSecurity; import java.io.IOException; import java.text.ParseException; +import java.util.Base64; import java.util.Collections; import java.util.List; import java.util.UUID; @@ -34,6 +35,7 @@ public class OAuthTest extends AbstractRestApiTest { private static final String CLIENT_ID_SERVER_FLOW = "testOAuthServerSideFlowClient"; private static final String CLIENT_ID = "testOAuthTokenLoginClient"; private static final String CLIENT_ID_VALIDITY = "testOAuthTokenValidityClient"; + private String clientSecretValidity; private static final String REDIRECT_URI = "default"; private static final String TOKEN_REVOCATION = "oauth/revoke"; @@ -258,7 +260,7 @@ public void testOAuthTokenValidity() throws IOException { OAuthSettingsPage oAuthSettingsPage = new OAuthSettingsPage(context).load(); OAuthClientEditorPage editorPage = oAuthSettingsPage.editClient(CLIENT_ID_VALIDITY); - String secret = editorPage.getSecret(); + clientSecretValidity = editorPage.getSecret(); editorPage.setValidity(validity); editorPage.save(); @@ -268,7 +270,7 @@ public void testOAuthTokenValidity() throws IOException { + "oauth/access_token?grant_type=client_credentials&client_id=" + CLIENT_ID_VALIDITY + "&redirect_uri=default&client_secret=" - + secret; + + clientSecretValidity; final HttpResponse response = execute(new HttpGet(tokenGetUrl), false); final JsonNode tokenNode = readJson(mapper, response); @@ -277,7 +279,7 @@ public void testOAuthTokenValidity() throws IOException { Assert.assertEquals(days, validity); } - @Test(description = "Revoke valid tokens") + @Test(description = "Revoke valid tokens", dependsOnMethods = "testOAuthTokenValidity") public void testOAuthTokenRevocation() throws IOException { String token = requestToken(CLIENT_ID_VALIDITY); String currentUserAPIPath = context.getBaseUrl() + "api/content/currentuser"; @@ -287,7 +289,7 @@ public void testOAuthTokenRevocation() throws IOException { assertEquals(200, response.getStatusLine().getStatusCode()); // Now revoke the token. - response = revokeOauthToken(token); + response = revokeOauthToken(token, CLIENT_ID_VALIDITY, clientSecretValidity); assertResponse(response, 200, "Token revocation should return 200"); // The token should not work now. @@ -295,12 +297,35 @@ public void testOAuthTokenRevocation() throws IOException { assertEquals(403, response.getStatusLine().getStatusCode()); } - @Test(description = "Revoke invalid tokens") + @Test(description = "Revoke invalid tokens", dependsOnMethods = "testOAuthTokenValidity") public void testOAuthInvalidTokenRevocation() throws IOException { - HttpResponse response = revokeOauthToken("bad token"); + HttpResponse response = revokeOauthToken("bad token", CLIENT_ID_VALIDITY, clientSecretValidity); assertResponse(response, 200, "Revoking invalid token should return 200"); } + @Test(description = "Revoke token with bad credentials") + public void testOAuthTokenRevocationBadCred() throws IOException { + HttpResponse response = revokeOauthToken("token", "bad client ID", "bad client secret"); + assertResponse(response, 401, "Revoking token without correct credentials should return 401"); + } + + @Test( + description = "Credentials are good, but the token was not generate from this client.", + dependsOnMethods = "testOAuthTokenValidity") + public void testTokenRevocationWrongClient() throws IOException { + String token = requestToken(CLIENT_ID); + HttpResponse response = revokeOauthToken(token, CLIENT_ID_VALIDITY, clientSecretValidity); + assertResponse( + response, + 200, + "Good credentials should return 200 although the token was not generated from the client"); + + // The token should still work. + String currentUserAPIPath = context.getBaseUrl() + "api/content/currentuser"; + response = rawTokenExecute(currentUserAPIPath, token); + assertEquals(200, response.getStatusLine().getStatusCode()); + } + private HttpResponse rawTokenExecute(HttpUriRequest request, String rawToken) throws IOException { final Header tokenHeader = new BasicHeader("X-Authorization", rawToken); request.setHeader(tokenHeader); @@ -311,12 +336,26 @@ private HttpResponse rawTokenExecute(String requestUrl, String token) throws IOE return rawTokenExecute(new HttpGet(requestUrl), "access_token=" + token); } - private HttpResponse revokeOauthToken(String token) throws IOException { + private HttpResponse revokeOauthToken(String token, String clientId, String clientSecret) + throws IOException { HttpPost post = new HttpPost(context.getBaseUrl() + TOKEN_REVOCATION); + post.addHeader( + "Authorization", + "Basic " + Base64.getEncoder().encodeToString((clientId + ":" + clientSecret).getBytes())); UrlEncodedFormEntity payload = new UrlEncodedFormEntity(Collections.singletonList(new BasicNameValuePair("token", token))); post.setEntity(payload); return execute(post, false); } + + private String[] getClientCredentials() { + logon(); + OAuthSettingsPage oAuthSettingsPage = new OAuthSettingsPage(context).load(); + OAuthClientEditorPage editorPage = oAuthSettingsPage.editClient(CLIENT_ID_VALIDITY); + String clientId = editorPage.getClientId(); + String clientSecret = editorPage.getSecret(); + + return new String[] {clientId, clientSecret}; + } }