Merge remote-tracking branch 'gitlab/develop' into develop

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

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";

Source/Plugins/Core/com.equella.core/src/com/tle/web/oauth/service/OAuthWebService.java

@@ -103,11 +103,12 @@ List<Map.Entry<String, String>> 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;

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. */

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<String> 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 <a href="https://tools.ietf.org/html/rfc7009#section-2.2">the spec for OAuth 2.0
-   * Token Revocation </a>>, 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.
+   *
+   * <p>According to <a href="https://tools.ietf.org/html/rfc7009#section-2.2">the spec for OAuth
+   * 2.0 Token Revocation </a>>, 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"));
+            });
   }
 }

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,20 +289,43 @@ 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.
     response = rawTokenExecute(currentUserAPIPath, token);
     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};
+  }
 }