fix: Token validity check (#136)

Handle missing or expired token with a 401 HTTP status instead of 500
with OpenShift OAuth the same as it is already done with Keycloak.

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/OpenshiftTokenInitializationFilter.java

@@ -25,13 +25,13 @@ import jakarta.servlet.http.HttpServletRequest;
 import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
 import javax.inject.Inject;
 import javax.inject.Singleton;
 import org.eclipse.che.api.core.ConflictException;
 import org.eclipse.che.api.core.ServerException;
 import org.eclipse.che.api.core.model.user.User;
 import org.eclipse.che.api.user.server.UserManager;
-import org.eclipse.che.api.workspace.server.spi.InfrastructureException;
 import org.eclipse.che.commons.subject.Subject;
 import org.eclipse.che.commons.subject.SubjectImpl;
 import org.eclipse.che.multiuser.api.authentication.commons.SessionStore;
@@ -51,7 +51,8 @@ import org.slf4j.LoggerFactory;
  * unauthenticated paths, that are allowed without token.
  */
 @Singleton
-public class OpenshiftTokenInitializationFilter extends MultiUserEnvironmentInitializationFilter {
+public class OpenshiftTokenInitializationFilter
+    extends MultiUserEnvironmentInitializationFilter<io.fabric8.openshift.api.model.User> {
 
   private static final Logger LOG =
       LoggerFactory.getLogger(OpenshiftTokenInitializationFilter.class);
@@ -78,44 +79,41 @@ public class OpenshiftTokenInitializationFilter extends MultiUserEnvironmentInit
   }
 
   @Override
-  protected String getUserId(String token) {
+  protected Optional<io.fabric8.openshift.api.model.User> processToken(String token) {
+    // We're effectively creating new client for each request. It might be a good idea to somehow
+    // cache the client or user object. However, it may require non-trivial refactoring which may
+    // be unnecessary. Keeping it as is for now to avoid premature optimization.
     try {
-      io.fabric8.openshift.api.model.User user = getCurrentUser(token);
-      return firstNonNull(user.getMetadata().getUid(), user.getMetadata().getName());
+      OpenShiftClient client = clientFactory.createAuthenticatedClient(token);
+      return Optional.ofNullable(client.currentUser());
     } catch (KubernetesClientException e) {
       if (e.getCode() == 401) {
         LOG.error(
             "Unauthorized when getting current user. Invalid OpenShift token, probably expired. Re-login? Re-request the token?");
+        return Optional.empty();
       }
-      throw new RuntimeException(e);
-    } catch (InfrastructureException e) {
-      throw new RuntimeException(e);
+
+      throw e;
     }
   }
 
   @Override
-  protected Subject extractSubject(String token) {
-    try {
-      ObjectMeta userMeta = getCurrentUser(token).getMetadata();
-      User user =
-          userManager.getOrCreateUser(
-              firstNonNull(userMeta.getUid(), userMeta.getName()),
-              openshiftUserEmail(userMeta),
-              userMeta.getName());
-      return new AuthorizedSubject(
-          new SubjectImpl(user.getName(), user.getId(), token, false), permissionChecker);
-    } catch (InfrastructureException | ServerException | ConflictException e) {
-      throw new RuntimeException(e);
-    }
+  protected String getUserId(io.fabric8.openshift.api.model.User user) {
+    return firstNonNull(user.getMetadata().getUid(), user.getMetadata().getName());
   }
 
-  private io.fabric8.openshift.api.model.User getCurrentUser(String token)
-      throws InfrastructureException {
-    // We're effectively creating new client for each request. It might be a good idea to somehow
-    // cache the client or user object. However, it may require non-trivial refactoring which may
-    // be unnecessary. Keeping it as is for now to avoid premature optimization.
-    OpenShiftClient client = clientFactory.createAuthenticatedClient(token);
-    return client.currentUser();
+  @Override
+  protected Subject extractSubject(String token, io.fabric8.openshift.api.model.User osu) {
+    try {
+      ObjectMeta userMeta = osu.getMetadata();
+      User user =
+          userManager.getOrCreateUser(
+              getUserId(osu), openshiftUserEmail(userMeta), userMeta.getName());
+      return new AuthorizedSubject(
+          new SubjectImpl(user.getName(), user.getId(), token, false), permissionChecker);
+    } catch (ServerException | ConflictException e) {
+      throw new RuntimeException(e);
+    }
   }
 
   protected String openshiftUserEmail(ObjectMeta userMeta) {
@@ -144,8 +142,8 @@ public class OpenshiftTokenInitializationFilter extends MultiUserEnvironmentInit
       }
     }
 
-    LOG.error("Rejecting the request due to missing token in Authorization header.");
-    sendError(response, 401, "Authorization token is missing");
+    LOG.error("Rejecting the request due to missing/expired token in Authorization header.");
+    sendError(response, 401, "Authorization token is missing or expired");
   }
 
   @Override

infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/OpenshiftTokenInitializationFilterTest.java

@@ -18,6 +18,7 @@ import static org.mockito.Mockito.when;
 import static org.testng.Assert.*;
 
 import io.fabric8.kubernetes.api.model.ObjectMeta;
+import io.fabric8.kubernetes.client.KubernetesClientException;
 import io.fabric8.openshift.api.model.User;
 import io.fabric8.openshift.client.OpenShiftClient;
 import jakarta.servlet.FilterChain;
@@ -25,6 +26,7 @@ import jakarta.servlet.ServletException;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import java.io.IOException;
+import java.util.Optional;
 import org.eclipse.che.api.core.ConflictException;
 import org.eclipse.che.api.core.ServerException;
 import org.eclipse.che.api.user.server.UserManager;
@@ -77,8 +79,10 @@ public class OpenshiftTokenInitializationFilterTest {
     when(openshiftUser.getMetadata()).thenReturn(openshiftUserMeta);
     when(openshiftUserMeta.getUid()).thenReturn(USER_UID);
 
-    String userId = openshiftTokenInitializationFilter.getUserId(TOKEN);
+    User u = openshiftTokenInitializationFilter.processToken(TOKEN).get();
+    String userId = openshiftTokenInitializationFilter.getUserId(u);
 
+    assertEquals(u, openshiftUser);
     assertEquals(userId, USER_UID);
     verify(openShiftClientFactory).createAuthenticatedClient(TOKEN);
     verify(openShiftClient).currentUser();
@@ -98,14 +102,15 @@ public class OpenshiftTokenInitializationFilterTest {
         .thenReturn(
             new UserImpl(KUBE_ADMIN_USERNAME, KUBE_ADMIN_USERNAME + "@che", KUBE_ADMIN_USERNAME));
 
-    Subject subject = openshiftTokenInitializationFilter.extractSubject(TOKEN);
+    User u = openshiftTokenInitializationFilter.processToken(TOKEN).get();
+    Subject subject = openshiftTokenInitializationFilter.extractSubject(TOKEN, u);
 
     assertEquals(subject.getUserId(), KUBE_ADMIN_USERNAME);
     assertEquals(subject.getUserName(), KUBE_ADMIN_USERNAME);
   }
 
   @Test
-  public void extractSubjectCreatesSubjectWithCurretlyAuthenticatedUser()
+  public void extractSubjectCreatesSubjectWithCurrentlyAuthenticatedUser()
       throws InfrastructureException, ServerException, ConflictException {
     when(openShiftClientFactory.createAuthenticatedClient(TOKEN)).thenReturn(openShiftClient);
     when(openShiftClient.currentUser()).thenReturn(openshiftUser);
@@ -115,8 +120,10 @@ public class OpenshiftTokenInitializationFilterTest {
     when(userManager.getOrCreateUser(USER_UID, USERNAME + "@che", USERNAME))
         .thenReturn(new UserImpl(USER_UID, USERNAME + "@che", USERNAME));
 
-    Subject subject = openshiftTokenInitializationFilter.extractSubject(TOKEN);
+    User u = openshiftTokenInitializationFilter.processToken(TOKEN).get();
+    Subject subject = openshiftTokenInitializationFilter.extractSubject(TOKEN, u);
 
+    assertEquals(u, openshiftUser);
     assertEquals(subject.getUserId(), USER_UID);
     assertEquals(subject.getUserName(), USERNAME);
   }
@@ -141,4 +148,14 @@ public class OpenshiftTokenInitializationFilterTest {
 
     verify(servletResponse).sendError(eq(401), anyString());
   }
+
+  @Test
+  public void invalidTokenShouldBeHandledAsMissing() throws Exception {
+    when(openShiftClientFactory.createAuthenticatedClient(TOKEN)).thenReturn(openShiftClient);
+    when(openShiftClient.currentUser())
+        .thenThrow(new KubernetesClientException("failah", 401, null));
+
+    Optional<User> u = openshiftTokenInitializationFilter.processToken(TOKEN);
+    assertTrue(u.isEmpty());
+  }
 }

multiuser/api/che-multiuser-api-authentication-commons/src/main/java/org/eclipse/che/multiuser/api/authentication/commons/filter/MultiUserEnvironmentInitializationFilter.java

@@ -23,6 +23,7 @@ import jakarta.servlet.http.HttpServletRequestWrapper;
 import jakarta.servlet.http.HttpServletResponse;
 import jakarta.servlet.http.HttpSession;
 import java.io.IOException;
+import java.util.Optional;
 import org.eclipse.che.commons.env.EnvironmentContext;
 import org.eclipse.che.commons.subject.Subject;
 import org.eclipse.che.multiuser.api.authentication.commons.SessionStore;
@@ -42,9 +43,10 @@ import org.slf4j.LoggerFactory;
  *   <li>Set subject for current request into {@link EnvironmentContext}
  * </ul>
  *
+ * @param <T> the type of intermediary type used for conversion from a string token to a Subject
  * @author Max Shaposhnyk (mshaposh@redhat.com)
  */
-public abstract class MultiUserEnvironmentInitializationFilter implements Filter {
+public abstract class MultiUserEnvironmentInitializationFilter<T> implements Filter {
 
   private static final Logger LOG =
       LoggerFactory.getLogger(MultiUserEnvironmentInitializationFilter.class);
@@ -114,14 +116,23 @@ public abstract class MultiUserEnvironmentInitializationFilter implements Filter
       handleMissingToken(request, response, chain);
       return;
     }
-    String userId = getUserId(token); // make sure token still valid before continue
+    Optional<T> maybeProcessedToken = processToken(token);
+    if (maybeProcessedToken.isEmpty()) {
+      handleMissingToken(request, response, chain);
+      return;
+    }
+
+    T processedToken = maybeProcessedToken.get();
+
+    String userId = getUserId(processedToken);
+
     // retrieve cached session if any or create new
     httpRequest = new SessionCachedHttpRequest(request, userId);
     HttpSession session = httpRequest.getSession(true);
     // retrieve and check / create new subject
     sessionSubject = (Subject) session.getAttribute(CHE_SUBJECT_ATTRIBUTE);
     if (sessionSubject == null) {
-      sessionSubject = extractSubject(token);
+      sessionSubject = extractSubject(token, processedToken);
       session.setAttribute(CHE_SUBJECT_ATTRIBUTE, sessionSubject);
     } else if (!sessionSubject.getUserId().equals(userId)) {
       LOG.debug(
@@ -130,10 +141,10 @@ public abstract class MultiUserEnvironmentInitializationFilter implements Filter
           userId);
       session.invalidate();
       HttpSession new_session = httpRequest.getSession(true);
-      sessionSubject = extractSubject(token);
+      sessionSubject = extractSubject(token, processedToken);
       new_session.setAttribute(CHE_SUBJECT_ATTRIBUTE, sessionSubject);
     } else if (!sessionSubject.getToken().equals(token)) {
-      sessionSubject = extractSubject(token);
+      sessionSubject = extractSubject(token, processedToken);
       session.setAttribute(CHE_SUBJECT_ATTRIBUTE, sessionSubject);
     }
     // set current subject
@@ -146,21 +157,34 @@ public abstract class MultiUserEnvironmentInitializationFilter implements Filter
   }
 
   /**
-   * Calculates id of the user from given authentication token. This <b>may</b> also imply
-   * verification of token signature or encryption for encrypted/signed tokens (like JWT etc)
+   * Processes the token and creates implementation-specific intermediary type using which the
+   * subclasses can extract different kinds of information like user ID or subject.
    *
-   * @param token authentication token
+   * <p>This <b>may</b> also imply verification of token signature or encryption for
+   * encrypted/signed tokens (like JWT etc).
+   *
+   * @param token the token to process
+   * @return a processed token or null if the token could not be processed for valid reasons (like
+   *     expiry or not matching any user).
+   */
+  protected abstract Optional<T> processToken(String token);
+
+  /**
+   * Retrieves the id of the user from given authentication token.
+   *
+   * @param processedToken the processed authentication string
    * @return user id given token belongs to
    */
-  protected abstract String getUserId(String token);
+  protected abstract String getUserId(T processedToken);
 
   /**
    * Calculates user {@link Subject} from given authentication token.
    *
-   * @param token authentication token
+   * @param token the original authentication token string
+   * @param processedToken the processed authentication string
    * @return constructed subject
    */
-  protected abstract Subject extractSubject(String token) throws ServletException;
+  protected abstract Subject extractSubject(String token, T processedToken) throws ServletException;
 
   /**
    * Describes behavior when the token is missed. In case if performing authentication by given

multiuser/api/che-multiuser-api-authentication-commons/src/test/java/org/eclipse/che/multiuser/api/authentication/commons/filter/MultiUserEnvironmentInitializationFilterTest.java

@@ -16,25 +16,26 @@ import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.withSettings;
 
 import jakarta.servlet.FilterChain;
-import jakarta.servlet.FilterConfig;
-import jakarta.servlet.ServletRequest;
-import jakarta.servlet.ServletResponse;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import jakarta.servlet.http.HttpSession;
+import java.util.Optional;
 import org.eclipse.che.commons.env.EnvironmentContext;
 import org.eclipse.che.commons.subject.Subject;
 import org.eclipse.che.commons.subject.SubjectImpl;
 import org.eclipse.che.multiuser.api.authentication.commons.SessionStore;
 import org.eclipse.che.multiuser.api.authentication.commons.token.RequestTokenExtractor;
 import org.mockito.Mock;
+import org.mockito.Mockito;
 import org.mockito.testng.MockitoTestNGListener;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Listeners;
@@ -54,25 +55,54 @@ public class MultiUserEnvironmentInitializationFilterTest {
   private final String token = "token-abc123";
   private final Subject subject = new SubjectImpl("user", userId, token, false);
 
-  private MultiUserEnvironmentInitializationFilter filter;
+  private MultiUserEnvironmentInitializationFilter<Object> filter;
 
   @BeforeMethod
+  @SuppressWarnings("unchecked")
   public void setUp() throws Exception {
-    filter = spy(new MockMultiUserEnvironmentInitializationFilter(sessionStore, tokenExtractor));
-    lenient().when(filter.getUserId(anyString())).thenReturn(userId);
-    lenient().when(filter.extractSubject(anyString())).thenReturn(subject);
+    filter =
+        mock(
+            MultiUserEnvironmentInitializationFilter.class,
+            withSettings()
+                .defaultAnswer(Mockito.CALLS_REAL_METHODS)
+                .useConstructor(sessionStore, tokenExtractor));
+    lenient().when(filter.getUserId(any())).thenReturn(userId);
+    lenient().when(filter.extractSubject(anyString(), any())).thenReturn(subject);
+    // pretend like we successfully processed the token unless defined otherwise in the tests
+    lenient().when(filter.processToken(anyString())).thenReturn(Optional.of(new Object()));
   }
 
   @Test
   public void shouldCallHandleMissingTokenIfTokenIsNull() throws Exception {
+    // given
     when(tokenExtractor.getToken(any(HttpServletRequest.class))).thenReturn(null);
+
     // when
     filter.doFilter(request, response, chain);
+
+    // then
     verify(tokenExtractor).getToken(eq(request));
     verify(filter).handleMissingToken(eq(request), eq(response), eq(chain));
     verifyNoMoreInteractions(request);
-    verify(filter, never()).getUserId(anyString());
-    verify(filter, never()).extractSubject(anyString());
+    verify(filter, never()).getUserId(any());
+    verify(filter, never()).extractSubject(anyString(), any());
+  }
+
+  @Test
+  public void shouldCallHandleMissingTokenIfTokenCannotBeProcessed() throws Exception {
+    // given
+    when(tokenExtractor.getToken(any(HttpServletRequest.class))).thenReturn("abc");
+    when(filter.processToken(anyString())).thenReturn(Optional.empty());
+
+    // when
+    filter.doFilter(request, response, chain);
+
+    // then
+    verify(tokenExtractor).getToken(eq(request));
+    verify(filter).handleMissingToken(eq(request), eq(response), eq(chain));
+    verifyNoMoreInteractions(request);
+    verify(filter, never()).getUserId(any());
+    verify(filter, never()).extractSubject(anyString(), any());
   }
 
   @Test
@@ -85,7 +115,7 @@ public class MultiUserEnvironmentInitializationFilterTest {
     // then
     verify(request).getSession(eq(false));
     verify(tokenExtractor).getToken(eq(request));
-    verify(filter).getUserId(eq(token));
+    verify(filter).getUserId(any());
     verify(sessionStore).getSession(eq(userId), any());
   }
 
@@ -97,7 +127,7 @@ public class MultiUserEnvironmentInitializationFilterTest {
     // when
     filter.doFilter(request, response, chain);
     // then
-    verify(filter).extractSubject(eq(token));
+    verify(filter).extractSubject(eq(token), any());
   }
 
   @Test
@@ -110,7 +140,7 @@ public class MultiUserEnvironmentInitializationFilterTest {
     // when
     filter.doFilter(request, response, chain);
     // then
-    verify(filter).extractSubject(eq(token));
+    verify(filter).extractSubject(eq(token), any());
   }
 
   @Test
@@ -123,7 +153,7 @@ public class MultiUserEnvironmentInitializationFilterTest {
     filter.doFilter(request, response, chain);
     // then
     verify(session).invalidate();
-    verify(filter).extractSubject(eq(token));
+    verify(filter).extractSubject(eq(token), any());
   }
 
   @Test
@@ -138,33 +168,4 @@ public class MultiUserEnvironmentInitializationFilterTest {
     // then
     verify(context).setSubject(eq(subject));
   }
-
-  private static class MockMultiUserEnvironmentInitializationFilter
-      extends MultiUserEnvironmentInitializationFilter {
-
-    MockMultiUserEnvironmentInitializationFilter(
-        SessionStore sessionStore, RequestTokenExtractor tokenExtractor) {
-      super(sessionStore, tokenExtractor);
-    }
-
-    @Override
-    protected String getUserId(String token) {
-      return null;
-    }
-
-    @Override
-    protected Subject extractSubject(String token) {
-      return null;
-    }
-
-    @Override
-    protected void handleMissingToken(
-        ServletRequest request, ServletResponse response, FilterChain chain) {}
-
-    @Override
-    public void init(FilterConfig filterConfig) {}
-
-    @Override
-    public void destroy() {}
-  }
 }

multiuser/keycloak/che-multiuser-keycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakEnvironmentInitializationFilter.java

@@ -54,7 +54,7 @@ import org.slf4j.LoggerFactory;
  */
 @Singleton
 public class KeycloakEnvironmentInitializationFilter
-    extends MultiUserEnvironmentInitializationFilter {
+    extends MultiUserEnvironmentInitializationFilter<Jws<Claims>> {
 
   private static final Logger LOG =
       LoggerFactory.getLogger(KeycloakEnvironmentInitializationFilter.class);
@@ -106,17 +106,19 @@ public class KeycloakEnvironmentInitializationFilter
   }
 
   @Override
-  protected String getUserId(String token) {
-    Claims claims = jwtParser.parseClaimsJws(token).getBody();
-    return claims.getSubject();
+  protected Optional<Jws<Claims>> processToken(String token) {
+    return Optional.ofNullable(jwtParser.parseClaimsJws(token));
   }
 
   @Override
-  public Subject extractSubject(String token) throws ServletException {
+  protected String getUserId(Jws<Claims> processedToken) {
+    return processedToken.getBody().getSubject();
+  }
 
-    Jws<Claims> jwt = jwtParser.parseClaimsJws(token);
-    Claims claims = jwt.getBody();
-    LOG.debug("JWT = {}", jwt);
+  @Override
+  public Subject extractSubject(String token, Jws<Claims> processedToken) throws ServletException {
+    Claims claims = processedToken.getBody();
+    LOG.debug("JWT = {}", processedToken);
     // OK, we can trust this JWT
 
     try {

multiuser/machine-auth/che-multiuser-machine-authentication/src/main/java/org/eclipse/che/multiuser/machine/authentication/server/MachineLoginFilter.java

@@ -26,6 +26,7 @@ import jakarta.servlet.ServletException;
 import jakarta.servlet.ServletRequest;
 import jakarta.servlet.ServletResponse;
 import java.io.IOException;
+import java.util.Optional;
 import javax.inject.Inject;
 import javax.inject.Singleton;
 import org.eclipse.che.api.core.NotFoundException;
@@ -45,7 +46,7 @@ import org.eclipse.che.multiuser.api.permission.server.PermissionChecker;
  * @author Anton Korneta
  */
 @Singleton
-public class MachineLoginFilter extends MultiUserEnvironmentInitializationFilter {
+public class MachineLoginFilter extends MultiUserEnvironmentInitializationFilter<Claims> {
 
   private final UserManager userManager;
   private final JwtParser jwtParser;
@@ -84,9 +85,13 @@ public class MachineLoginFilter extends MultiUserEnvironmentInitializationFilter
   }
 
   @Override
-  public Subject extractSubject(String token) {
+  protected Optional<Claims> processToken(String token) {
+    return Optional.ofNullable(jwtParser.parseClaimsJws(token).getBody());
+  }
+
+  @Override
+  public Subject extractSubject(String token, Claims claims) {
     try {
-      final Claims claims = jwtParser.parseClaimsJws(token).getBody();
       final String userId = claims.get(USER_ID_CLAIM, String.class);
       // check if user with such id exists
       final String userName = userManager.getById(userId).getName();
@@ -101,8 +106,7 @@ public class MachineLoginFilter extends MultiUserEnvironmentInitializationFilter
   }
 
   @Override
-  protected String getUserId(String token) {
-    final Claims claims = jwtParser.parseClaimsJws(token).getBody();
+  protected String getUserId(Claims claims) {
     return claims.get(USER_ID_CLAIM, String.class);
   }