From d9d70c8177ed2b56229e54dd9989e99087780684 Mon Sep 17 00:00:00 2001 From: Dominik Frantisek Bucik Date: Thu, 3 Jun 2021 10:59:37 +0200 Subject: [PATCH] Update matching for native apps - redirectURI matching for Native apps accoridng to https://datatracker.ietf.org/doc/html/rfc8252#section-7.3 --- .../src/main/resources/log4j.xml | 14 +++++--- .../src/main/webapp/WEB-INF/authz-config.xml | 4 +-- .../impl/BlacklistAwareRedirectResolver.java | 34 ++++++++++++++----- .../TestBlacklistAwareRedirectResolver.java | 15 ++++---- 4 files changed, 44 insertions(+), 23 deletions(-) diff --git a/openid-connect-server-webapp/src/main/resources/log4j.xml b/openid-connect-server-webapp/src/main/resources/log4j.xml index caed28b32..efb4074fe 100644 --- a/openid-connect-server-webapp/src/main/resources/log4j.xml +++ b/openid-connect-server-webapp/src/main/resources/log4j.xml @@ -20,10 +20,14 @@ - - + + + + + + - + @@ -52,7 +56,7 @@ - + @@ -77,7 +81,7 @@ - + diff --git a/openid-connect-server-webapp/src/main/webapp/WEB-INF/authz-config.xml b/openid-connect-server-webapp/src/main/webapp/WEB-INF/authz-config.xml index 3b7a4faa8..4d5242ae1 100644 --- a/openid-connect-server-webapp/src/main/webapp/WEB-INF/authz-config.xml +++ b/openid-connect-server-webapp/src/main/webapp/WEB-INF/authz-config.xml @@ -41,7 +41,7 @@ error-page="/error"> - + @@ -57,4 +57,4 @@ - \ No newline at end of file + diff --git a/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/BlacklistAwareRedirectResolver.java b/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/BlacklistAwareRedirectResolver.java index f47581694..d231f496a 100644 --- a/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/BlacklistAwareRedirectResolver.java +++ b/openid-connect-server/src/main/java/org/mitre/oauth2/service/impl/BlacklistAwareRedirectResolver.java @@ -18,19 +18,20 @@ */ package org.mitre.oauth2.service.impl; +import org.mitre.oauth2.model.ClientDetailsEntity; import org.mitre.openid.connect.config.ConfigurationPropertiesBean; import org.mitre.openid.connect.service.BlacklistedSiteService; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.security.oauth2.common.exceptions.InvalidGrantException; import org.springframework.security.oauth2.common.exceptions.InvalidRequestException; import org.springframework.security.oauth2.common.exceptions.OAuth2Exception; import org.springframework.security.oauth2.common.exceptions.RedirectMismatchException; import org.springframework.security.oauth2.provider.ClientDetails; -import org.springframework.security.oauth2.provider.endpoint.DefaultRedirectResolver; import org.springframework.security.oauth2.provider.endpoint.RedirectResolver; import org.springframework.stereotype.Component; -import com.google.common.base.Strings; import org.springframework.util.Assert; import org.springframework.util.MultiValueMap; import org.springframework.util.StringUtils; @@ -40,10 +41,11 @@ import org.springframework.web.util.UriComponentsBuilder; import java.util.Arrays; import java.util.Collection; import java.util.HashSet; -import java.util.Iterator; import java.util.List; import java.util.Set; +import static org.mitre.oauth2.model.ClientDetailsEntity.AppType.NATIVE; + /** * * A redirect resolver that knows how to check against the blacklisted URIs @@ -55,6 +57,8 @@ import java.util.Set; @Component("blacklistAwareRedirectResolver") public class BlacklistAwareRedirectResolver implements RedirectResolver { + private static final Logger log = LoggerFactory.getLogger(BlacklistAwareRedirectResolver.class); + @Autowired private BlacklistedSiteService blacklistService; @@ -119,6 +123,7 @@ public class BlacklistAwareRedirectResolver implements RedirectResolver { */ @Override public String resolveRedirect(String requestedRedirect, ClientDetails client) throws OAuth2Exception { + log.error("!!!!!!!!!!!!!!!!!!!!!!!Resolving redirect!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); Set authorizedGrantTypes = client.getAuthorizedGrantTypes(); if (authorizedGrantTypes.isEmpty()) { throw new InvalidGrantException("A client must have at least one authorized grant type."); @@ -133,7 +138,9 @@ public class BlacklistAwareRedirectResolver implements RedirectResolver { throw new InvalidRequestException("At least one redirect_uri must be registered with the client."); } - String redirect = obtainMatchingRedirect(registeredRedirectUris, requestedRedirect); + ClientDetailsEntity cde = (ClientDetailsEntity) client; + + String redirect = obtainMatchingRedirect(registeredRedirectUris, requestedRedirect, cde.getApplicationType()); if (blacklistService.isBlacklisted(redirect)) { // don't let it go through @@ -154,16 +161,22 @@ public class BlacklistAwareRedirectResolver implements RedirectResolver { * * @param requestedRedirect The requested redirect URI. * @param redirectUri The registered redirect URI. + * @param applicationType * @return Whether the requested redirect URI "matches" the specified redirect URI. */ - protected boolean redirectMatches(String requestedRedirect, String redirectUri) { + protected boolean redirectMatches(String requestedRedirect, String redirectUri, + ClientDetailsEntity.AppType applicationType) + { UriComponents requestedRedirectUri = UriComponentsBuilder.fromUriString(requestedRedirect).build(); UriComponents registeredRedirectUri = UriComponentsBuilder.fromUriString(redirectUri).build(); boolean schemeMatch = isEqual(registeredRedirectUri.getScheme(), requestedRedirectUri.getScheme()); boolean userInfoMatch = isEqual(registeredRedirectUri.getUserInfo(), requestedRedirectUri.getUserInfo()); boolean hostMatch = hostMatches(registeredRedirectUri.getHost(), requestedRedirectUri.getHost()); - boolean portMatch = !matchPorts || registeredRedirectUri.getPort() == requestedRedirectUri.getPort(); + boolean portMatch = true; + if (!NATIVE.equals(applicationType)) { + portMatch = !matchPorts || registeredRedirectUri.getPort() == requestedRedirectUri.getPort(); + } boolean pathMatch = true; boolean queryParamMatch = true; if (strictMatch) { @@ -194,10 +207,13 @@ public class BlacklistAwareRedirectResolver implements RedirectResolver { * * @param redirectUris the set of the registered URIs to try and find a match. This cannot be null or empty. * @param requestedRedirect the URI used as part of the request + * @param applicationType * @return redirect uri * @throws RedirectMismatchException if no match was found */ - private String obtainMatchingRedirect(Set redirectUris, String requestedRedirect) { + private String obtainMatchingRedirect(Set redirectUris, String requestedRedirect, + ClientDetailsEntity.AppType applicationType) + { Assert.notEmpty(redirectUris, "Redirect URIs cannot be empty"); if (redirectUris.size() == 1 && requestedRedirect == null) { @@ -205,7 +221,7 @@ public class BlacklistAwareRedirectResolver implements RedirectResolver { } for (String redirectUri : redirectUris) { - if (requestedRedirect != null && redirectMatches(requestedRedirect, redirectUri)) { + if (requestedRedirect != null && redirectMatches(requestedRedirect, redirectUri, applicationType)) { // Initialize with the registered redirect-uri UriComponentsBuilder redirectUriBuilder = UriComponentsBuilder.fromUriString(redirectUri); UriComponents requestedRedirectUri = UriComponentsBuilder.fromUriString(requestedRedirect).build(); @@ -213,7 +229,7 @@ public class BlacklistAwareRedirectResolver implements RedirectResolver { if (this.matchSubdomains) { redirectUriBuilder.host(requestedRedirectUri.getHost()); } - if (!this.matchPorts) { + if (!this.matchPorts || NATIVE.equals(applicationType)) { redirectUriBuilder.port(requestedRedirectUri.getPort()); } if (!this.strictMatch) { diff --git a/openid-connect-server/src/test/java/org/mitre/oauth2/service/impl/TestBlacklistAwareRedirectResolver.java b/openid-connect-server/src/test/java/org/mitre/oauth2/service/impl/TestBlacklistAwareRedirectResolver.java index ae0b525a7..5507707f7 100644 --- a/openid-connect-server/src/test/java/org/mitre/oauth2/service/impl/TestBlacklistAwareRedirectResolver.java +++ b/openid-connect-server/src/test/java/org/mitre/oauth2/service/impl/TestBlacklistAwareRedirectResolver.java @@ -21,6 +21,7 @@ import static org.mockito.Matchers.anyString; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mitre.oauth2.model.ClientDetailsEntity; import org.mitre.openid.connect.config.ConfigurationPropertiesBean; import org.mitre.openid.connect.service.BlacklistedSiteService; import org.mockito.InjectMocks; @@ -49,7 +50,7 @@ public class TestBlacklistAwareRedirectResolver { private BlacklistedSiteService blacklistService; @Mock - private ClientDetails client; + private ClientDetailsEntity client; @Mock private ConfigurationPropertiesBean config; @@ -108,12 +109,12 @@ public class TestBlacklistAwareRedirectResolver { public void testRedirectMatches_default() { // this is not an exact match - boolean res1 = resolver.redirectMatches(pathUri, goodUri); + boolean res1 = resolver.redirectMatches(pathUri, goodUri, ClientDetailsEntity.AppType.WEB); assertThat(res1, is(false)); // this is an exact match - boolean res2 = resolver.redirectMatches(goodUri, goodUri); + boolean res2 = resolver.redirectMatches(goodUri, goodUri, ClientDetailsEntity.AppType.WEB); assertThat(res2, is(true)); @@ -126,12 +127,12 @@ public class TestBlacklistAwareRedirectResolver { resolver.setStrictMatch(false); // this is not an exact match (but that's OK) - boolean res1 = resolver.redirectMatches(pathUri, goodUri); + boolean res1 = resolver.redirectMatches(pathUri, goodUri, ClientDetailsEntity.AppType.WEB); assertThat(res1, is(true)); // this is an exact match - boolean res2 = resolver.redirectMatches(goodUri, goodUri); + boolean res2 = resolver.redirectMatches(goodUri, goodUri, ClientDetailsEntity.AppType.WEB); assertThat(res2, is(true)); @@ -140,12 +141,12 @@ public class TestBlacklistAwareRedirectResolver { @Test public void testHeartMode() { // this is not an exact match - boolean res1 = resolver.redirectMatches(pathUri, goodUri); + boolean res1 = resolver.redirectMatches(pathUri, goodUri, ClientDetailsEntity.AppType.WEB); assertThat(res1, is(false)); // this is an exact match - boolean res2 = resolver.redirectMatches(goodUri, goodUri); + boolean res2 = resolver.redirectMatches(goodUri, goodUri, ClientDetailsEntity.AppType.WEB); assertThat(res2, is(true)); }