diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/OpenshiftTokenInitializationFilter.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/OpenshiftTokenInitializationFilter.java index b1a0229561..9d4862df58 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/OpenshiftTokenInitializationFilter.java +++ b/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 { 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 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 diff --git a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/OpenshiftTokenInitializationFilterTest.java b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/OpenshiftTokenInitializationFilterTest.java index 9eed03571a..a649dde5af 100644 --- a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/OpenshiftTokenInitializationFilterTest.java +++ b/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 u = openshiftTokenInitializationFilter.processToken(TOKEN); + assertTrue(u.isEmpty()); + } } diff --git a/multiuser/api/che-multiuser-api-authentication-commons/src/main/java/org/eclipse/che/multiuser/api/authentication/commons/filter/MultiUserEnvironmentInitializationFilter.java b/multiuser/api/che-multiuser-api-authentication-commons/src/main/java/org/eclipse/che/multiuser/api/authentication/commons/filter/MultiUserEnvironmentInitializationFilter.java index 82e03bf660..c100be3417 100644 --- a/multiuser/api/che-multiuser-api-authentication-commons/src/main/java/org/eclipse/che/multiuser/api/authentication/commons/filter/MultiUserEnvironmentInitializationFilter.java +++ b/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; *
  • Set subject for current request into {@link EnvironmentContext} * * + * @param 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 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 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 may 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 + *

    This may 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 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 diff --git a/multiuser/api/che-multiuser-api-authentication-commons/src/test/java/org/eclipse/che/multiuser/api/authentication/commons/filter/MultiUserEnvironmentInitializationFilterTest.java b/multiuser/api/che-multiuser-api-authentication-commons/src/test/java/org/eclipse/che/multiuser/api/authentication/commons/filter/MultiUserEnvironmentInitializationFilterTest.java index 9b1a826fe3..62847c8e7c 100644 --- a/multiuser/api/che-multiuser-api-authentication-commons/src/test/java/org/eclipse/che/multiuser/api/authentication/commons/filter/MultiUserEnvironmentInitializationFilterTest.java +++ b/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 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() {} - } } diff --git a/multiuser/keycloak/che-multiuser-keycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakEnvironmentInitializationFilter.java b/multiuser/keycloak/che-multiuser-keycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakEnvironmentInitializationFilter.java index a03d802be1..84bf08d59f 100644 --- a/multiuser/keycloak/che-multiuser-keycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakEnvironmentInitializationFilter.java +++ b/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> { 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> processToken(String token) { + return Optional.ofNullable(jwtParser.parseClaimsJws(token)); } @Override - public Subject extractSubject(String token) throws ServletException { + protected String getUserId(Jws processedToken) { + return processedToken.getBody().getSubject(); + } - Jws jwt = jwtParser.parseClaimsJws(token); - Claims claims = jwt.getBody(); - LOG.debug("JWT = {}", jwt); + @Override + public Subject extractSubject(String token, Jws processedToken) throws ServletException { + Claims claims = processedToken.getBody(); + LOG.debug("JWT = {}", processedToken); // OK, we can trust this JWT try { diff --git a/multiuser/machine-auth/che-multiuser-machine-authentication/src/main/java/org/eclipse/che/multiuser/machine/authentication/server/MachineLoginFilter.java b/multiuser/machine-auth/che-multiuser-machine-authentication/src/main/java/org/eclipse/che/multiuser/machine/authentication/server/MachineLoginFilter.java index 4d6b401032..a1c1943c23 100644 --- a/multiuser/machine-auth/che-multiuser-machine-authentication/src/main/java/org/eclipse/che/multiuser/machine/authentication/server/MachineLoginFilter.java +++ b/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 { 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 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); }