From b6d861fea695b9600b52dfff99c12ffc81080c8f Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Thu, 7 Apr 2016 13:46:30 +0200 Subject: [PATCH] KEYCLOAK-2751 Separate HTTP status codes for REST API errors --- ...nstallationClientRegistrationProvider.java | 41 +----- .../ClientRegistrationAuth.java | 133 ++++++++++++------ .../ClientRegistrationTokenUtils.java | 15 +- .../client/AdapterInstallationConfigTest.java | 8 +- .../client/ClientRegistrationTest.java | 18 ++- .../client/InitialAccessTokenTest.java | 6 +- .../client/RegistrationAccessTokenTest.java | 16 +-- 7 files changed, 129 insertions(+), 108 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/clientregistration/AdapterInstallationClientRegistrationProvider.java b/services/src/main/java/org/keycloak/services/clientregistration/AdapterInstallationClientRegistrationProvider.java index 65a4339423..86a71e33fd 100644 --- a/services/src/main/java/org/keycloak/services/clientregistration/AdapterInstallationClientRegistrationProvider.java +++ b/services/src/main/java/org/keycloak/services/clientregistration/AdapterInstallationClientRegistrationProvider.java @@ -17,18 +17,17 @@ package org.keycloak.services.clientregistration; -import org.keycloak.authentication.AuthenticationProcessor; -import org.keycloak.events.Errors; import org.keycloak.events.EventBuilder; import org.keycloak.events.EventType; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; -import org.keycloak.protocol.oidc.utils.AuthorizeClientUtil; -import org.keycloak.services.ForbiddenException; import org.keycloak.services.managers.ClientManager; import org.keycloak.services.managers.RealmManager; -import javax.ws.rs.*; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.Produces; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; @@ -52,12 +51,7 @@ public class AdapterInstallationClientRegistrationProvider implements ClientRegi event.event(EventType.CLIENT_INFO); ClientModel client = session.getContext().getRealm().getClientByClientId(clientId); - - if (auth.isAuthenticated()) { - auth.requireView(client); - } else { - authenticateClient(client); - } + auth.requireView(client); ClientManager clientManager = new ClientManager(new RealmManager(session)); Object rep = clientManager.toInstallationRepresentation(session.getContext().getRealm(), client, session.getContext().getAuthServerUrl()); @@ -80,29 +74,4 @@ public class AdapterInstallationClientRegistrationProvider implements ClientRegi public void close() { } - private void authenticateClient(ClientModel client) { - if (client.isPublicClient()) { - return; - } - - AuthenticationProcessor processor = AuthorizeClientUtil.getAuthenticationProcessor(session, event); - - Response response = processor.authenticateClient(); - if (response != null) { - event.client(client.getClientId()).error(Errors.NOT_ALLOWED); - throw new ForbiddenException(); - } - - ClientModel authClient = processor.getClient(); - if (client == null) { - event.client(client.getClientId()).error(Errors.NOT_ALLOWED); - throw new ForbiddenException(); - } - - if (!authClient.getClientId().equals(client.getClientId())) { - event.client(client.getClientId()).error(Errors.NOT_ALLOWED); - throw new ForbiddenException(); - } - } - } diff --git a/services/src/main/java/org/keycloak/services/clientregistration/ClientRegistrationAuth.java b/services/src/main/java/org/keycloak/services/clientregistration/ClientRegistrationAuth.java index 5dc72854a1..5564ef282d 100644 --- a/services/src/main/java/org/keycloak/services/clientregistration/ClientRegistrationAuth.java +++ b/services/src/main/java/org/keycloak/services/clientregistration/ClientRegistrationAuth.java @@ -17,20 +17,28 @@ package org.keycloak.services.clientregistration; -import com.sun.xml.bind.v2.runtime.reflect.opt.Const; +import org.jboss.resteasy.spi.Failure; +import org.jboss.resteasy.spi.NotFoundException; import org.jboss.resteasy.spi.UnauthorizedException; import org.keycloak.Config; +import org.keycloak.authentication.AuthenticationProcessor; import org.keycloak.common.util.Time; import org.keycloak.events.Errors; import org.keycloak.events.EventBuilder; -import org.keycloak.models.*; +import org.keycloak.models.AdminRoles; +import org.keycloak.models.ClientInitialAccessModel; +import org.keycloak.models.ClientModel; +import org.keycloak.models.Constants; +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.RealmModel; +import org.keycloak.protocol.oidc.utils.AuthorizeClientUtil; import org.keycloak.representations.JsonWebToken; import org.keycloak.services.ForbiddenException; import org.keycloak.util.TokenUtil; import javax.ws.rs.core.HttpHeaders; +import javax.ws.rs.core.Response; import javax.ws.rs.core.UriInfo; -import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -49,8 +57,6 @@ public class ClientRegistrationAuth { public ClientRegistrationAuth(KeycloakSession session, EventBuilder event) { this.session = session; this.event = event; - - init(); } private void init() { @@ -67,41 +73,39 @@ public class ClientRegistrationAuth { return; } - jwt = ClientRegistrationTokenUtils.parseToken(realm, uri, split[1]); + jwt = ClientRegistrationTokenUtils.verifyToken(realm, uri, split[1]); + if (jwt == null) { + throw unauthorized(); + } if (isInitialAccessToken()) { initialAccessModel = session.sessions().getClientInitialAccessModel(session.getContext().getRealm(), jwt.getId()); if (initialAccessModel == null) { - throw new ForbiddenException(); + throw unauthorized(); } } } - public boolean isAuthenticated() { - return jwt != null; - } - - public boolean isBearerToken() { - return TokenUtil.TOKEN_TYPE_BEARER.equals(jwt.getType()); + private boolean isBearerToken() { + return jwt != null && TokenUtil.TOKEN_TYPE_BEARER.equals(jwt.getType()); } public boolean isInitialAccessToken() { - return ClientRegistrationTokenUtils.TYPE_INITIAL_ACCESS_TOKEN.equals(jwt.getType()); + return jwt != null && ClientRegistrationTokenUtils.TYPE_INITIAL_ACCESS_TOKEN.equals(jwt.getType()); } public boolean isRegistrationAccessToken() { - return ClientRegistrationTokenUtils.TYPE_REGISTRATION_ACCESS_TOKEN.equals(jwt.getType()); + return jwt != null && ClientRegistrationTokenUtils.TYPE_REGISTRATION_ACCESS_TOKEN.equals(jwt.getType()); } public void requireCreate() { - if (!isAuthenticated()) { - event.error(Errors.NOT_ALLOWED); - throw new UnauthorizedException(); - } + init(); if (isBearerToken()) { if (hasRole(AdminRoles.MANAGE_CLIENTS, AdminRoles.CREATE_CLIENT)) { return; + } else { + throw forbidden(); } } else if (isInitialAccessToken()) { if (initialAccessModel.getRemainingCount() > 0) { @@ -111,58 +115,55 @@ public class ClientRegistrationAuth { } } - event.error(Errors.NOT_ALLOWED); - throw new ForbiddenException(); + throw unauthorized(); } public void requireView(ClientModel client) { - if (!isAuthenticated()) { - event.error(Errors.NOT_ALLOWED); - throw new UnauthorizedException(); - } - - if (client == null) { - event.error(Errors.NOT_ALLOWED); - throw new ForbiddenException(); - } + init(); if (isBearerToken()) { if (hasRole(AdminRoles.MANAGE_CLIENTS, AdminRoles.VIEW_CLIENTS)) { + if (client == null) { + throw notFound(); + } return; + } else { + throw forbidden(); } } else if (isRegistrationAccessToken()) { - if (client.getRegistrationToken() != null && client.getRegistrationToken().equals(jwt.getId())) { + if (client.getRegistrationToken() != null && client != null && client.getRegistrationToken().equals(jwt.getId())) { + return; + } + } else if (isInitialAccessToken()) { + throw unauthorized(); + } else { + if (authenticateClient(client)) { return; } } - event.error(Errors.NOT_ALLOWED); - throw new ForbiddenException(); + throw unauthorized(); } public void requireUpdate(ClientModel client) { - if (!isAuthenticated()) { - event.error(Errors.NOT_ALLOWED); - throw new UnauthorizedException(); - } - - if (client == null) { - event.error(Errors.NOT_ALLOWED); - throw new ForbiddenException(); - } + init(); if (isBearerToken()) { if (hasRole(AdminRoles.MANAGE_CLIENTS)) { + if (client == null) { + throw notFound(); + } return; + } else { + throw forbidden(); } } else if (isRegistrationAccessToken()) { - if (client.getRegistrationToken() != null && client.getRegistrationToken().equals(jwt.getId())) { + if (client.getRegistrationToken() != null && client != null && client.getRegistrationToken().equals(jwt.getId())) { return; } } - event.error(Errors.NOT_ALLOWED); - throw new ForbiddenException(); + throw unauthorized(); } public ClientInitialAccessModel getInitialAccessModel() { @@ -207,4 +208,46 @@ public class ClientRegistrationAuth { } } + private boolean authenticateClient(ClientModel client) { + if (client.isPublicClient()) { + return true; + } + + AuthenticationProcessor processor = AuthorizeClientUtil.getAuthenticationProcessor(session, event); + + Response response = processor.authenticateClient(); + if (response != null) { + event.client(client.getClientId()).error(Errors.NOT_ALLOWED); + throw unauthorized(); + } + + ClientModel authClient = processor.getClient(); + if (client == null) { + event.client(client.getClientId()).error(Errors.NOT_ALLOWED); + throw unauthorized(); + } + + if (!authClient.getClientId().equals(client.getClientId())) { + event.client(client.getClientId()).error(Errors.NOT_ALLOWED); + throw unauthorized(); + } + + return true; + } + + private Failure unauthorized() { + event.error(Errors.NOT_ALLOWED); + return new UnauthorizedException(); + } + + private Failure forbidden() { + event.error(Errors.NOT_ALLOWED); + return new ForbiddenException(); + } + + private Failure notFound() { + event.error(Errors.NOT_ALLOWED); + return new NotFoundException("Client not found"); + } + } diff --git a/services/src/main/java/org/keycloak/services/clientregistration/ClientRegistrationTokenUtils.java b/services/src/main/java/org/keycloak/services/clientregistration/ClientRegistrationTokenUtils.java index 2cd74ba2e7..2fe65cc8fd 100755 --- a/services/src/main/java/org/keycloak/services/clientregistration/ClientRegistrationTokenUtils.java +++ b/services/src/main/java/org/keycloak/services/clientregistration/ClientRegistrationTokenUtils.java @@ -28,7 +28,6 @@ import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.representations.JsonWebToken; -import org.keycloak.services.ForbiddenException; import org.keycloak.services.Urls; import org.keycloak.util.TokenUtil; @@ -57,37 +56,37 @@ public class ClientRegistrationTokenUtils { return createToken(realm, uri, model.getId(), TYPE_INITIAL_ACCESS_TOKEN, model.getExpiration() > 0 ? model.getTimestamp() + model.getExpiration() : 0); } - public static JsonWebToken parseToken(RealmModel realm, UriInfo uri, String token) { + public static JsonWebToken verifyToken(RealmModel realm, UriInfo uri, String token) { JWSInput input; try { input = new JWSInput(token); } catch (JWSInputException e) { - throw new ForbiddenException(e); + return null; } if (!RSAProvider.verify(input, realm.getPublicKey())) { - throw new ForbiddenException("Invalid signature"); + return null; } JsonWebToken jwt; try { jwt = input.readJsonContent(JsonWebToken.class); } catch (JWSInputException e) { - throw new ForbiddenException(e); + return null; } if (!getIssuer(realm, uri).equals(jwt.getIssuer())) { - throw new ForbiddenException("Issuer doesn't match"); + return null; } if (!jwt.isActive()) { - throw new ForbiddenException("Expired token"); + return null; } if (!(TokenUtil.TOKEN_TYPE_BEARER.equals(jwt.getType()) || TYPE_INITIAL_ACCESS_TOKEN.equals(jwt.getType()) || TYPE_REGISTRATION_ACCESS_TOKEN.equals(jwt.getType()))) { - throw new ForbiddenException("Invalid token type"); + return null; } return jwt; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AdapterInstallationConfigTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AdapterInstallationConfigTest.java index 427202548d..0a4fe4bea7 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AdapterInstallationConfigTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AdapterInstallationConfigTest.java @@ -103,9 +103,9 @@ public class AdapterInstallationConfigTest extends AbstractClientRegistrationTes try { reg.getAdapterConfig(client.getClientId()); - fail("Expected 403"); + fail("Expected 401"); } catch (ClientRegistrationException e) { - assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); + assertEquals(401, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); } } @@ -115,9 +115,9 @@ public class AdapterInstallationConfigTest extends AbstractClientRegistrationTes try { reg.getAdapterConfig(client2.getClientId()); - fail("Expected 403"); + fail("Expected 401"); } catch (ClientRegistrationException e) { - assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); + assertEquals(401, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientRegistrationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientRegistrationTest.java index 70bfed0e0a..0ef5e26c1e 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientRegistrationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientRegistrationTest.java @@ -126,8 +126,14 @@ public class ClientRegistrationTest extends AbstractClientRegistrationTest { @Test public void getClientNotFound() throws ClientRegistrationException { authManageClients(); + assertNull(reg.get("invalid")); + } + + @Test + public void getClientNotFoundNoAccess() throws ClientRegistrationException { + authNoAccess(); try { - reg.get(CLIENT_ID); + reg.get("invalid"); fail("Expected 403"); } catch (ClientRegistrationException e) { assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); @@ -181,10 +187,14 @@ public class ClientRegistrationTest extends AbstractClientRegistrationTest { public void updateClientNotFound() throws ClientRegistrationException { authManageClients(); try { - updateClient(); - fail("Expected 403"); + ClientRepresentation client = new ClientRepresentation(); + client.setClientId("invalid"); + + reg.update(client); + + fail("Expected 404"); } catch (ClientRegistrationException e) { - assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); + assertEquals(404, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/InitialAccessTokenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/InitialAccessTokenTest.java index 4f4d204b11..ad33f52e9e 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/InitialAccessTokenTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/InitialAccessTokenTest.java @@ -58,7 +58,7 @@ public class InitialAccessTokenTest extends AbstractClientRegistrationTest { try { reg.create(rep); } catch (ClientRegistrationException e) { - Assert.assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); + Assert.assertEquals(401, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); } } @@ -79,7 +79,7 @@ public class InitialAccessTokenTest extends AbstractClientRegistrationTest { try { reg.create(rep); } catch (ClientRegistrationException e) { - Assert.assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); + Assert.assertEquals(401, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); } } @@ -113,7 +113,7 @@ public class InitialAccessTokenTest extends AbstractClientRegistrationTest { try { reg.create(rep); } catch (ClientRegistrationException e) { - Assert.assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); + Assert.assertEquals(401, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/RegistrationAccessTokenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/RegistrationAccessTokenTest.java index c6fb960594..96a0010c93 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/RegistrationAccessTokenTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/RegistrationAccessTokenTest.java @@ -59,8 +59,8 @@ public class RegistrationAccessTokenTest extends AbstractClientRegistrationTest try { reg.get(client.getClientId()); fail("Expected 403"); - } catch (ClientRegistrationException e) { - assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); + } catch (Exception e) { + assertEquals(401, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); } } return null; @@ -82,9 +82,9 @@ public class RegistrationAccessTokenTest extends AbstractClientRegistrationTest reg.auth(Auth.token("invalid")); try { reg.get(client.getClientId()); - fail("Expected 403"); + fail("Expected 401"); } catch (ClientRegistrationException e) { - assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); + assertEquals(401, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); } } @@ -109,9 +109,9 @@ public class RegistrationAccessTokenTest extends AbstractClientRegistrationTest reg.auth(Auth.token("invalid")); try { reg.update(client); - fail("Expected 403"); + fail("Expected 401"); } catch (ClientRegistrationException e) { - assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); + assertEquals(401, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); } assertEquals("http://root", getClient(client.getId()).getRootUrl()); @@ -128,9 +128,9 @@ public class RegistrationAccessTokenTest extends AbstractClientRegistrationTest reg.auth(Auth.token("invalid")); try { reg.delete(client); - fail("Expected 403"); + fail("Expected 401"); } catch (ClientRegistrationException e) { - assertEquals(403, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); + assertEquals(401, ((HttpErrorException) e.getCause()).getStatusLine().getStatusCode()); } assertNotNull(getClient(client.getId())); }