Added redirect URI validation in authorization request (addressed #374)
Change-Id: I7e3bbc9cdfcf85fa897e0425cdc6bdb3eeda94f4
diff --git a/full/Changes b/full/Changes
index 22123c1..6eff087 100644
--- a/full/Changes
+++ b/full/Changes
@@ -2,6 +2,7 @@
2022-05-25
- Added a new API: list plugins (e.g for marketplace)
+ - Added redirect URI validation in authorization request (addressed #374)
# version 0.67.1
@@ -27,7 +28,7 @@
2022-03-31
- Updated query and user-group name pattern.
- 2022-04-08
+2022-04-08
- Added redirect_uri to client info API.
2022-04-11
- Added registration_date, refresh_token_expiry, source and is_permitted
@@ -41,8 +42,8 @@
- Updated authorization error response. (Included error and error
description in the client redirect URI except for missing or
invalid client id or redirect URI.
-
-
+
+
# version 0.65.2
2022-03-03
diff --git a/full/src/main/java/de/ids_mannheim/korap/oauth2/oltu/service/OltuAuthorizationService.java b/full/src/main/java/de/ids_mannheim/korap/oauth2/oltu/service/OltuAuthorizationService.java
index 0ac3da6..5898beb 100644
--- a/full/src/main/java/de/ids_mannheim/korap/oauth2/oltu/service/OltuAuthorizationService.java
+++ b/full/src/main/java/de/ids_mannheim/korap/oauth2/oltu/service/OltuAuthorizationService.java
@@ -143,7 +143,8 @@
int statusCode = e.getStatusCode();
if (!clientId.isEmpty()
&& statusCode != StatusCodes.CLIENT_NOT_FOUND
- && statusCode != StatusCodes.AUTHORIZATION_FAILED) {
+ && statusCode != StatusCodes.AUTHORIZATION_FAILED
+ && statusCode != StatusCodes.INVALID_REDIRECT_URI) {
String registeredUri = null;
try {
OAuth2Client client = clientService.retrieveClient(clientId);
diff --git a/full/src/main/java/de/ids_mannheim/korap/oauth2/service/OAuth2AuthorizationService.java b/full/src/main/java/de/ids_mannheim/korap/oauth2/service/OAuth2AuthorizationService.java
index 69713f8..581fa09 100644
--- a/full/src/main/java/de/ids_mannheim/korap/oauth2/service/OAuth2AuthorizationService.java
+++ b/full/src/main/java/de/ids_mannheim/korap/oauth2/service/OAuth2AuthorizationService.java
@@ -4,6 +4,7 @@
import java.time.ZonedDateTime;
import java.util.Set;
+import org.apache.commons.validator.routines.UrlValidator;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.springframework.beans.factory.annotation.Autowired;
@@ -38,6 +39,8 @@
protected OAuth2ScopeServiceImpl scopeService;
@Autowired
private AuthorizationDao authorizationDao;
+ @Autowired
+ private UrlValidator redirectURIValidator;
@Autowired
protected FullConfiguration config;
@@ -110,24 +113,24 @@
throws KustvaktException {
String registeredUri = client.getRedirectURI();
+
if (redirectUri != null && !redirectUri.isEmpty()) {
// check if the redirect URI the same as that in DB
- if (registeredUri != null && !registeredUri.isEmpty()
- && !redirectUri.equals(registeredUri)) {
+ if (!redirectURIValidator.isValid(redirectUri) ||
+ (registeredUri != null && !registeredUri.isEmpty()
+ && !redirectUri.equals(registeredUri))) {
throw new KustvaktException(StatusCodes.INVALID_REDIRECT_URI,
"Invalid redirect URI", OAuth2Error.INVALID_REQUEST);
}
}
- else {
- // redirect_uri is not required in client registration!
- if (registeredUri != null && !registeredUri.isEmpty()) {
+ // redirect_uri is not required in client registration
+ else if (registeredUri != null && !registeredUri.isEmpty()) {
redirectUri = registeredUri;
- }
- else {
- throw new KustvaktException(StatusCodes.MISSING_REDIRECT_URI,
- "Redirect URI is required",
- OAuth2Error.INVALID_REQUEST);
- }
+ }
+ else {
+ throw new KustvaktException(StatusCodes.MISSING_REDIRECT_URI,
+ "Redirect URI is required",
+ OAuth2Error.INVALID_REQUEST);
}
return redirectUri;
diff --git a/full/src/main/java/de/ids_mannheim/korap/oauth2/service/OAuth2ClientService.java b/full/src/main/java/de/ids_mannheim/korap/oauth2/service/OAuth2ClientService.java
index 45dbd6b..00f9e70 100644
--- a/full/src/main/java/de/ids_mannheim/korap/oauth2/service/OAuth2ClientService.java
+++ b/full/src/main/java/de/ids_mannheim/korap/oauth2/service/OAuth2ClientService.java
@@ -98,15 +98,15 @@
if (url != null && !url.isEmpty()) {
if (!urlValidator.isValid(url)) {
throw new KustvaktException(StatusCodes.INVALID_ARGUMENT,
- url + " is invalid.", OAuth2Error.INVALID_REQUEST);
+ "Invalid URL", OAuth2Error.INVALID_REQUEST);
}
}
String redirectURI = clientJson.getRedirectURI();
if (redirectURI != null && !redirectURI.isEmpty()
&& !redirectURIValidator.isValid(redirectURI)) {
- throw new KustvaktException(StatusCodes.INVALID_ARGUMENT,
- redirectURI + " is invalid.", OAuth2Error.INVALID_REQUEST);
+ throw new KustvaktException(StatusCodes.INVALID_REDIRECT_URI,
+ "Invalid redirect URI", OAuth2Error.INVALID_REQUEST);
}
// boolean isNative = isNativeClient(url, redirectURI);
diff --git a/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2ClientControllerTest.java b/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2ClientControllerTest.java
index 528efba..c52b7a7 100644
--- a/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2ClientControllerTest.java
+++ b/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2ClientControllerTest.java
@@ -16,6 +16,7 @@
import org.apache.commons.io.IOUtils;
import org.apache.http.entity.ContentType;
+import org.apache.oltu.oauth2.common.error.OAuthError;
import org.junit.Test;
import com.fasterxml.jackson.databind.JsonNode;
@@ -223,15 +224,75 @@
assertEquals("invalid_request", node.at("/error").asText());
assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus());
}
+
+ @Test
+ public void testRegisterClientInvalidRedirectURI ()
+ throws UniformInterfaceException, ClientHandlerException,
+ KustvaktException {
+ // invalid hostname
+ String redirectUri = "https://test.public.client/redirect";
+ OAuth2ClientJson clientJson =
+ createOAuth2ClientJson("OAuth2PublicClient",
+ OAuth2ClientType.PUBLIC, "A public test client.");
+ clientJson.setRedirectURI(redirectUri);
+ ClientResponse response = registerClient(username, clientJson);
+ testInvalidRedirectUri(response.getEntity(String.class), false,
+ response.getStatus());
+
+ // localhost is not allowed
+ redirectUri = "http://localhost:1410";
+ clientJson.setRedirectURI(redirectUri);
+ response = registerClient(username, clientJson);
+ testInvalidRedirectUri(response.getEntity(String.class), false,
+ response.getStatus());
+
+ // fragment is not allowed
+ redirectUri = "https://public.client.com/redirect.html#bar";
+ clientJson.setRedirectURI(redirectUri);
+ response = registerClient(username, clientJson);
+ testInvalidRedirectUri(response.getEntity(String.class), false,
+ response.getStatus());
+ }
+
+ @Test
+ public void testRegisterClientInvalidURL ()
+ throws UniformInterfaceException, ClientHandlerException,
+ KustvaktException {
+ // invalid hostname
+ String url = "https://test.public.client";
+ OAuth2ClientJson clientJson =
+ createOAuth2ClientJson("OAuth2PublicClient",
+ OAuth2ClientType.PUBLIC, "A public test client.");
+ clientJson.setUrl(url);
+ ClientResponse response = registerClient(username, clientJson);
+ testInvalidUrl(response.getEntity(String.class), response.getStatus());
+
+ // localhost is not allowed
+ url = "http://localhost:1410";
+ clientJson.setRedirectURI(url);
+ response = registerClient(username, clientJson);
+ testInvalidUrl(response.getEntity(String.class), response.getStatus());
+ }
+
+ private void testInvalidUrl (String entity,
+ int status) throws KustvaktException {
+ JsonNode node = JsonUtils.readTree(entity);
+ assertEquals(OAuthError.CodeResponse.INVALID_REQUEST,
+ node.at("/error").asText());
+ assertEquals("Invalid URL",
+ node.at("/error_description").asText());
+ assertEquals(Status.BAD_REQUEST.getStatusCode(), status);
+ }
@Test
public void testRegisterPublicClient () throws UniformInterfaceException,
ClientHandlerException, KustvaktException {
- String redirectUri = "https://test.public.client.com/redirect";
+ String redirectUri = "https://public.client.com/redirect";
OAuth2ClientJson clientJson =
createOAuth2ClientJson("OAuth2PublicClient",
OAuth2ClientType.PUBLIC, "A public test client.");
- clientJson.setUrl("http://test.public.client.com");
+ // http and fragment are allowed
+ clientJson.setUrl("http://public.client.com/index.html#bar");
clientJson.setRedirectURI(redirectUri);
ClientResponse response = registerClient(username, clientJson);
diff --git a/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2ControllerTest.java b/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2ControllerTest.java
index ed28cf6..15e1954 100644
--- a/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2ControllerTest.java
+++ b/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2ControllerTest.java
@@ -55,7 +55,7 @@
MultivaluedMap<String, String> params =
getQueryParamsFromURI(redirectUri);
assertNotNull(params.getFirst("code"));
- assertEquals("thisIsMyState", params.getFirst("state"));
+ assertEquals(state, params.getFirst("state"));
assertEquals("match_info search client_info", params.getFirst("scope"));
}
@@ -67,23 +67,23 @@
}
@Test
- public void testAuthorizePublicClientWithRedirectUri () throws KustvaktException {
+ public void testAuthorizeWithRedirectUri () throws KustvaktException {
ClientResponse response =
requestAuthorizationCode("code", publicClientId2,
"https://public.com/redirect", "", "", userAuthHeader);
assertEquals(Status.TEMPORARY_REDIRECT.getStatusCode(),
response.getStatus());
-
+
URI redirectUri = response.getLocation();
- assertEquals(redirectUri.getScheme(), "https");
- assertEquals(redirectUri.getHost(), "public.com");
- assertEquals(redirectUri.getPath(), "/redirect");
+ assertEquals("https", redirectUri.getScheme());
+ assertEquals("public.com", redirectUri.getHost());
+ assertEquals("/redirect", redirectUri.getPath());
String[] queryParts = redirectUri.getQuery().split("&");
assertTrue(queryParts[0].startsWith("code="));
- assertEquals(queryParts[1], "scope=match_info+search");
+ assertEquals("scope=match_info+search", queryParts[1]);
}
-
+
@Test
public void testAuthorizeWithoutScope () throws KustvaktException {
ClientResponse response = requestAuthorizationCode("code",
@@ -124,7 +124,7 @@
node.at("/error").asText());
assertEquals("Redirect URI is required",
node.at("/error_description").asText());
- assertEquals("thisIsMyState", node.at("/state").asText());
+ assertEquals(state, node.at("/state").asText());
}
@Test
@@ -159,29 +159,50 @@
public void testAuthorizeInvalidClientId () throws KustvaktException {
ClientResponse response = requestAuthorizationCode("code",
"unknown-client-id", "", "", "", userAuthHeader);
-// assertEquals(Status.UNAUTHORIZED.getStatusCode(), response.getStatus());
+ assertEquals(Status.UNAUTHORIZED.getStatusCode(), response.getStatus());
String entity = response.getEntity(String.class);
-// System.out.println(entity);
JsonNode node = JsonUtils.readTree(entity);
+ assertEquals(OAuth2Error.INVALID_CLIENT, node.at("/error").asText());
assertEquals("Unknown client with unknown-client-id.",
node.at("/error_description").asText());
}
@Test
- public void testAuthorizeInvalidRedirectUri () throws KustvaktException {
+ public void testAuthorizeDifferentRedirectUri () throws KustvaktException {
String redirectUri = "https://different.uri/redirect";
ClientResponse response = requestAuthorizationCode("code",
confidentialClientId, redirectUri, "", state, userAuthHeader);
+ testInvalidRedirectUri(response.getEntity(String.class), true,
+ response.getStatus());
+ }
- assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus());
+ @Test
+ public void testAuthorizeWithRedirectUriLocalhost ()
+ throws KustvaktException {
+ ClientResponse response =
+ requestAuthorizationCode("code", publicClientId2,
+ "http://localhost:1410", "", state, userAuthHeader);
+ testInvalidRedirectUri(response.getEntity(String.class), true,
+ response.getStatus()); }
- String entity = response.getEntity(String.class);
- JsonNode node = JsonUtils.readTree(entity);
- assertEquals(OAuthError.CodeResponse.INVALID_REQUEST,
- node.at("/error").asText());
- assertEquals("Invalid redirect URI",
- node.at("/error_description").asText());
- assertEquals("thisIsMyState", node.at("/state").asText());
+ @Test
+ public void testAuthorizeWithRedirectUriFragment ()
+ throws KustvaktException {
+ ClientResponse response = requestAuthorizationCode("code",
+ publicClientId2, "http://public.com/index.html#redirect", "",
+ state, userAuthHeader);
+ testInvalidRedirectUri(response.getEntity(String.class), true,
+ response.getStatus());
+ }
+
+ @Test
+ public void testAuthorizeInvalidRedirectUri () throws KustvaktException {
+ // host not allowed by Apache URI Validator
+ String redirectUri = "https://public.uri/redirect";
+ ClientResponse response = requestAuthorizationCode("code",
+ publicClientId2, redirectUri, "", state, userAuthHeader);
+ testInvalidRedirectUri(response.getEntity(String.class), true,
+ response.getStatus());
}
@Test
@@ -219,7 +240,7 @@
node.at("/error").asText());
assertEquals("Invalid redirect URI",
node.at("/error_description").asText());
- assertEquals("thisIsMyState", node.at("/state").asText());
+ assertEquals(state, node.at("/state").asText());
// without redirect URI in the request and no registered
// redirect URI
@@ -232,7 +253,7 @@
node.at("/error").asText());
assertEquals("Redirect URI is required",
node.at("/error_description").asText());
- assertEquals("thisIsMyState", node.at("/state").asText());
+ assertEquals(state, node.at("/state").asText());
}
@Test
@@ -245,8 +266,8 @@
assertEquals(
"https://third.party.com/confidential/redirect?"
- + "error_description=read_address+is+an+invalid+scope&"
- + "state=thisIsMyState&error=invalid_scope",
+ + "error_description=read_address+is+an+invalid+scope&"
+ + "state=thisIsMyState&error=invalid_scope",
response.getLocation().toString());
}
@@ -267,8 +288,7 @@
@Test
public void testRequestTokenAuthorizationPublic ()
throws KustvaktException {
- String code =
- requestAuthorizationCode(publicClientId, userAuthHeader);
+ String code = requestAuthorizationCode(publicClientId, userAuthHeader);
ClientResponse response = requestTokenWithAuthorizationCodeAndForm(
publicClientId, clientSecret, code);
@@ -318,7 +338,8 @@
testRequestRefreshTokenInvalidClient(refreshToken);
testRequestRefreshTokenInvalidRefreshToken(confidentialClientId);
- testRequestRefreshToken(confidentialClientId, clientSecret, refreshToken);
+ testRequestRefreshToken(confidentialClientId, clientSecret,
+ refreshToken);
}
private void testRequestTokenWithUsedAuthorization (String code)
@@ -693,7 +714,7 @@
JsonNode node = JsonUtils.readTree(entity);
assertNotNull(node.at("/access_token").asText());
-
+
String newRefreshToken = node.at("/refresh_token").asText();
assertNotNull(newRefreshToken);
assertEquals(TokenType.BEARER.toString(),
@@ -701,10 +722,10 @@
assertNotNull(node.at("/expires_in").asText());
assertTrue(!newRefreshToken.equals(refreshToken));
-
+
testRequestTokenWithRevokedRefreshToken(clientId, clientSecret,
refreshToken);
-
+
testRevokeToken(newRefreshToken, clientId, clientSecret,
REFRESH_TOKEN_TYPE);
testRequestTokenWithRevokedRefreshToken(clientId, clientSecret,
@@ -757,10 +778,10 @@
form.add("super_client_secret", clientSecret);
form.add("token_type", tokenType);
- if (clientId != null && !clientId.isEmpty()){
+ if (clientId != null && !clientId.isEmpty()) {
form.add("client_id", clientId);
}
-
+
ClientResponse response = resource().path(API_VERSION).path("oauth2")
.path("token").path("list")
.header(Attributes.AUTHORIZATION, userAuthHeader)
@@ -773,14 +794,15 @@
String entity = response.getEntity(String.class);
return JsonUtils.readTree(entity);
}
-
+
private JsonNode requestTokenList (String userAuthHeader, String tokenType)
throws KustvaktException {
return requestTokenList(userAuthHeader, tokenType, null);
}
-
+
@Test
- public void testListRefreshTokenConfidentialClient () throws KustvaktException {
+ public void testListRefreshTokenConfidentialClient ()
+ throws KustvaktException {
String username = "gurgle";
String password = "pwd";
userAuthHeader = HttpAuthorizationHandler
@@ -841,9 +863,10 @@
assertEquals(3, node.size());
// list refresh tokens from client 1
- node = requestTokenList(userAuthHeader, REFRESH_TOKEN_TYPE, confidentialClientId);
+ node = requestTokenList(userAuthHeader, REFRESH_TOKEN_TYPE,
+ confidentialClientId);
assertEquals(2, node.size());
-
+
testRevokeToken(refreshToken1, superClientId, clientSecret,
REFRESH_TOKEN_TYPE);
testRevokeToken(node.at("/0/token").asText(), confidentialClientId,
@@ -870,7 +893,6 @@
assertEquals(0, node.size());
}
-
@Test
public void testListTokenPublicClient () throws KustvaktException {
String username = "nemo";
@@ -880,8 +902,8 @@
// access token 1
String code = requestAuthorizationCode(publicClientId, userAuthHeader);
- ClientResponse response = requestTokenWithAuthorizationCodeAndForm(publicClientId, "",
- code);
+ ClientResponse response = requestTokenWithAuthorizationCodeAndForm(
+ publicClientId, "", code);
assertEquals(Status.OK.getStatusCode(), response.getStatus());
JsonNode node = JsonUtils.readTree(response.getEntity(String.class));
String accessToken1 = node.at("/access_token").asText();
@@ -893,31 +915,30 @@
assertEquals(Status.OK.getStatusCode(), response.getStatus());
node = JsonUtils.readTree(response.getEntity(String.class));
String accessToken2 = node.at("/access_token").asText();
-
+
// list access tokens
node = requestTokenList(userAuthHeader, ACCESS_TOKEN_TYPE);
assertEquals(2, node.size());
-
+
// list refresh tokens
node = requestTokenList(userAuthHeader, REFRESH_TOKEN_TYPE);
assertEquals(0, node.size());
-
+
testRevokeTokenViaSuperClient(accessToken1, userAuthHeader);
node = requestTokenList(userAuthHeader, ACCESS_TOKEN_TYPE);
-// System.out.println(node);
+ // System.out.println(node);
assertEquals(1, node.size());
assertEquals(accessToken2, node.at("/0/token").asText());
- assertTrue(node.at("/0/scope").size()>0);
+ assertTrue(node.at("/0/scope").size() > 0);
assertNotNull(node.at("/0/created_date").asText());
assertNotNull(node.at("/0/expires_in").asLong());
assertNotNull(node.at("/0/user_authentication_time").asText());
-
+
assertEquals(publicClientId, node.at("/0/client_id").asText());
assertNotNull(node.at("/0/client_name").asText());
assertNotNull(node.at("/0/client_description").asText());
assertNotNull(node.at("/0/client_url").asText());
-
-
+
testRevokeTokenViaSuperClient(accessToken2, userAuthHeader);
node = requestTokenList(userAuthHeader, ACCESS_TOKEN_TYPE);
assertEquals(0, node.size());
diff --git a/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2TestBase.java b/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2TestBase.java
index 43cb07e..67fe2d8 100644
--- a/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2TestBase.java
+++ b/full/src/test/java/de/ids_mannheim/korap/web/controller/OAuth2TestBase.java
@@ -10,6 +10,7 @@
import javax.ws.rs.core.Response.Status;
import org.apache.http.entity.ContentType;
+import org.apache.oltu.oauth2.common.error.OAuthError;
import org.apache.oltu.oauth2.common.message.types.GrantType;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.util.MultiValueMap;
@@ -370,4 +371,18 @@
String entity = response.getEntity(String.class);
return JsonUtils.readTree(entity);
}
+
+ protected void testInvalidRedirectUri (String entity, boolean includeState,
+ int status) throws KustvaktException {
+ JsonNode node = JsonUtils.readTree(entity);
+ assertEquals(OAuthError.CodeResponse.INVALID_REQUEST,
+ node.at("/error").asText());
+ assertEquals("Invalid redirect URI",
+ node.at("/error_description").asText());
+ if (includeState) {
+ assertEquals(state, node.at("/state").asText());
+ }
+
+ assertEquals(Status.BAD_REQUEST.getStatusCode(), status);
+ }
}