From 1326570906df4c73645de2744cd46222e65a52a8 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Fri, 2 Feb 2024 13:32:58 +0100 Subject: [PATCH 01/42] Add getter for maxUsers in ScimUserEndpoints --- .../identity/uaa/scim/endpoints/ScimUserEndpoints.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java index 0271b100ffc..6436e0a0e99 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java @@ -93,6 +93,8 @@ import static org.cloudfoundry.identity.uaa.codestore.ExpiringCodeType.REGISTRATION; import static org.springframework.util.StringUtils.isEmpty; +import lombok.Getter; + /** * User provisioning and query endpoints. Implements the core API from the * Simple Cloud Identity Management (SCIM) @@ -122,6 +124,7 @@ public class ScimUserEndpoints implements InitializingBean, ApplicationEventPubl private final UserMfaCredentialsProvisioning mfaCredentialsProvisioning; private final ApprovalStore approvalStore; private final ScimGroupMembershipManager membershipManager; + @Getter private final int userMaxCount; private final HttpMessageConverter[] messageConverters; private final AtomicInteger scimUpdates; From dd5af1cf2eb64d6fd3a759e79e88a90ae1b0442c Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Fri, 2 Feb 2024 13:39:34 +0100 Subject: [PATCH 02/42] Fix lookup of users in "ids/Users" endpoint --- .../uaa/scim/endpoints/ScimUserEndpoints.java | 2 +- .../endpoints/UserIdConversionEndpoints.java | 122 ++++++++-- .../UserIdConversionEndpointsTests.java | 220 +++++++++++++++--- 3 files changed, 287 insertions(+), 57 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java index 6436e0a0e99..f26d00133a7 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java @@ -385,7 +385,7 @@ private int getVersion(String userId, String etag) { @RequestMapping(value = "/Users", method = RequestMethod.GET) @ResponseBody - public SearchResults findUsers( + public SearchResults findUsers( @RequestParam(value = "attributes", required = false) String attributesCommaSeparated, @RequestParam(required = false, defaultValue = "id pr") String filter, @RequestParam(required = false, defaultValue = "created") String sortBy, diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java index e6477d36ead..d5cac58d60f 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java @@ -1,14 +1,26 @@ package org.cloudfoundry.identity.uaa.scim.endpoints; -import com.unboundid.scim.sdk.SCIMException; -import com.unboundid.scim.sdk.SCIMFilter; +import static java.util.Collections.emptyList; +import static java.util.stream.Collectors.toList; +import static java.util.stream.Collectors.toSet; + +import javax.servlet.http.HttpServletRequest; + +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Set; + import org.cloudfoundry.identity.uaa.constants.OriginKeys; import org.cloudfoundry.identity.uaa.provider.IdentityProvider; import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning; -import org.cloudfoundry.identity.uaa.resources.SearchResults; +import org.cloudfoundry.identity.uaa.resources.SearchResultsFactory; import org.cloudfoundry.identity.uaa.scim.ScimCore; +import org.cloudfoundry.identity.uaa.scim.ScimUser; +import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning; import org.cloudfoundry.identity.uaa.scim.exception.ScimException; import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor; +import org.cloudfoundry.identity.uaa.util.UaaPagingUtils; import org.cloudfoundry.identity.uaa.util.UaaStringUtils; import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder; import org.slf4j.Logger; @@ -20,6 +32,7 @@ import org.springframework.http.ResponseEntity; import org.springframework.stereotype.Controller; import org.springframework.util.Assert; +import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; @@ -28,61 +41,120 @@ import org.springframework.web.servlet.View; import org.springframework.web.util.HtmlUtils; -import javax.servlet.http.HttpServletRequest; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.stream.Collectors; +import com.unboundid.scim.sdk.SCIMException; +import com.unboundid.scim.sdk.SCIMFilter; @Controller public class UserIdConversionEndpoints implements InitializingBean { private final Logger logger = LoggerFactory.getLogger(getClass()); - private final IdentityProviderProvisioning provisioning; + private final IdentityProviderProvisioning identityProviderProvisioning; private final SecurityContextAccessor securityContextAccessor; private final ScimUserEndpoints scimUserEndpoints; + private final ScimUserProvisioning scimUserProvisioning; private boolean enabled; - public UserIdConversionEndpoints(final @Qualifier("identityProviderProvisioning") IdentityProviderProvisioning provisioning, - final SecurityContextAccessor securityContextAccessor, - final ScimUserEndpoints scimUserEndpoints, - final @Value("${scim.userids_enabled:true}") boolean enabled) { - this.provisioning = provisioning; + public UserIdConversionEndpoints( + final @Qualifier("identityProviderProvisioning") IdentityProviderProvisioning identityProviderProvisioning, + final SecurityContextAccessor securityContextAccessor, + final ScimUserEndpoints scimUserEndpoints, + final @Qualifier("scimUserProvisioning") ScimUserProvisioning scimUserProvisioning, + final @Value("${scim.userids_enabled:true}") boolean enabled + ) { + this.identityProviderProvisioning = identityProviderProvisioning; this.securityContextAccessor = securityContextAccessor; this.scimUserEndpoints = scimUserEndpoints; this.enabled = enabled; + this.scimUserProvisioning = scimUserProvisioning; } @RequestMapping(value = "/ids/Users") @ResponseBody public ResponseEntity findUsers( @RequestParam(defaultValue = "") String filter, - @RequestParam(required = false, defaultValue = "ascending") String sortOrder, + @RequestParam(required = false, defaultValue = "ascending") final String sortOrder, @RequestParam(required = false, defaultValue = "1") int startIndex, @RequestParam(required = false, defaultValue = "100") int count, - @RequestParam(required = false, defaultValue = "false") boolean includeInactive) { + @RequestParam(required = false, defaultValue = "false") final boolean includeInactive + ) { if (!enabled) { logger.info("Request from user {} received at disabled Id translation endpoint with filter:{}", - UaaStringUtils.getCleanedUserControlString(securityContextAccessor.getAuthenticationInfo()), - UaaStringUtils.getCleanedUserControlString(filter)); + UaaStringUtils.getCleanedUserControlString(securityContextAccessor.getAuthenticationInfo()), + UaaStringUtils.getCleanedUserControlString(filter)); return new ResponseEntity<>("Illegal Operation: Endpoint not enabled.", HttpStatus.BAD_REQUEST); } + if (startIndex < 1) { + startIndex = 1; + } + + if (count > scimUserEndpoints.getUserMaxCount()) { + count = scimUserEndpoints.getUserMaxCount(); + } + filter = filter.trim(); checkFilter(filter); - List activeIdentityProviders = provisioning.retrieveActive(IdentityZoneHolder.get().getId()); + // get all users for the given filter and the current page + final List filteredUsers = getFilteredScimUsers(filter, sortOrder, includeInactive); + final List usersCurrentPage = UaaPagingUtils.subList(filteredUsers, startIndex, count); + + // map to result structure + final List> result = usersCurrentPage.stream() + .map(scimUser -> Map.of( + "id", scimUser.getId(), + "userName", scimUser.getUserName(), + "origin", scimUser.getOrigin() + )) + .collect(toList()); + + return new ResponseEntity<>( + SearchResultsFactory.buildSearchResultFrom( + result, + startIndex, + count, + filteredUsers.size(), + new String[]{"id", "userName", "origin"}, + Arrays.asList(ScimCore.SCHEMAS) + ), + HttpStatus.OK + ); + } - if (!includeInactive) { - if (activeIdentityProviders.isEmpty()) { - return new ResponseEntity<>(new SearchResults<>(Arrays.asList(ScimCore.SCHEMAS), new ArrayList<>(), startIndex, count, 0), HttpStatus.OK); + private List getFilteredScimUsers( + final String filter, + final String sortOrder, + final boolean includeInactive + ) { + final String idzId = IdentityZoneHolder.get().getId(); + + final List filteredScimUsers; + try { + filteredScimUsers = scimUserProvisioning.query(filter, "userName", sortOrder.equals("ascending"), idzId); + } catch (final IllegalArgumentException e) { + String msg = "Invalid filter expression: [" + filter + "]"; + if (StringUtils.hasText("userName")) { + msg += " [" + "userName" + "]"; } - String originFilter = activeIdentityProviders.stream().map(identityProvider -> "".concat("origin eq \"" + identityProvider.getOriginKey() + "\"")).collect(Collectors.joining(" OR ")); - filter += " AND (" + originFilter + " )"; + throw new ScimException(HtmlUtils.htmlEscape(msg), HttpStatus.BAD_REQUEST); } - return new ResponseEntity<>(scimUserEndpoints.findUsers("id,userName,origin", filter, "userName", sortOrder, startIndex, count), HttpStatus.OK); + if (includeInactive) { + return filteredScimUsers; + } + + // remove users from inactive IdPs + final List activeIdentityProviders = identityProviderProvisioning.retrieveActive(idzId); + if (activeIdentityProviders.isEmpty()) { + return emptyList(); + } + final Set originsOfActiveIdps = activeIdentityProviders.stream() + .map(IdentityProvider::getOriginKey) + .collect(toSet()); + return filteredScimUsers.stream() + .filter(scimUser -> originsOfActiveIdps.contains(scimUser.getOrigin())) + .collect(toList()); } @ExceptionHandler diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java index 58a0994b151..e848ea22fbb 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java @@ -13,65 +13,93 @@ package org.cloudfoundry.identity.uaa.scim.endpoints; +import static java.util.Collections.singletonList; +import static java.util.UUID.randomUUID; +import static java.util.stream.Collectors.toList; +import static junit.framework.Assert.assertEquals; +import static junit.framework.Assert.assertTrue; +import static org.assertj.core.api.Assertions.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.function.Function; +import java.util.stream.Stream; + +import org.cloudfoundry.identity.uaa.provider.IdentityProvider; import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning; import org.cloudfoundry.identity.uaa.resources.SearchResults; +import org.cloudfoundry.identity.uaa.scim.ScimUser; +import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning; import org.cloudfoundry.identity.uaa.scim.exception.ScimException; import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor; -import org.cloudfoundry.identity.uaa.zone.MultitenancyFixture; +import org.cloudfoundry.identity.uaa.zone.IdentityZone; +import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder; +import org.junit.After; import org.junit.Assert; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; -import org.mockito.Mockito; +import org.mockito.MockedStatic; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.security.core.authority.AuthorityUtils; -import java.util.Collection; -import java.util.Collections; - -import static junit.framework.Assert.assertEquals; -import static junit.framework.Assert.assertTrue; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.is; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.when; - /** * @author Dave Syer * @author Luke Taylor - * */ public class UserIdConversionEndpointsTests { @Rule public ExpectedException expected = ExpectedException.none(); - private IdentityProviderProvisioning provisioning = Mockito.mock(IdentityProviderProvisioning.class); + private final IdentityProviderProvisioning identityProviderProvisioning = mock(IdentityProviderProvisioning.class); private UserIdConversionEndpoints endpoints; private SecurityContextAccessor mockSecurityContextAccessor; - private ScimUserEndpoints scimUserEndpoints = Mockito.mock(ScimUserEndpoints.class); + private final ScimUserEndpoints scimUserEndpoints = mock(ScimUserEndpoints.class); + private final ScimUserProvisioning scimUserProvisioning = mock(ScimUserProvisioning.class); + + private final MockedStatic idzHolderMockedStatic; @SuppressWarnings("rawtypes") - private Collection authorities = AuthorityUtils - .commaSeparatedStringToAuthorityList("orgs.foo,uaa.user"); + private final Collection authorities = AuthorityUtils + .commaSeparatedStringToAuthorityList("orgs.foo,uaa.user"); + + public UserIdConversionEndpointsTests() { + this.idzHolderMockedStatic = mockStatic(IdentityZoneHolder.class); + } @SuppressWarnings("unchecked") @Before public void init() { - mockSecurityContextAccessor = Mockito.mock(SecurityContextAccessor.class); - endpoints = new UserIdConversionEndpoints(provisioning, mockSecurityContextAccessor, scimUserEndpoints, true); + mockSecurityContextAccessor = mock(SecurityContextAccessor.class); + endpoints = new UserIdConversionEndpoints(identityProviderProvisioning, mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, true); when(mockSecurityContextAccessor.getAuthorities()).thenReturn(authorities); when(mockSecurityContextAccessor.getAuthenticationInfo()).thenReturn("mock object"); - when(provisioning.retrieveActive(anyString())).thenReturn(Collections.singletonList(MultitenancyFixture.identityProvider("test-origin", "uaa"))); + when(scimUserEndpoints.getUserMaxCount()).thenReturn(10_000); + } + + @After + public void tearDown() throws Exception { + idzHolderMockedStatic.close(); } @Test public void testHappyDay() { + arrangeCurrentIdentityZone("uaa"); endpoints.findUsers("userName eq \"marissa\"", "ascending", 0, 100, false); } @@ -97,19 +125,69 @@ public void testBadFilterWithGroup() { } @Test - public void testGoodFilter1() { - final ResponseEntity response = endpoints.findUsers( - "(id eq \"foo\" and username eq \"bar\") or id eq \"bar\"", - "ascending", - 0, - 100, - false - ); - assertEquals(HttpStatus.OK, response.getStatusCode()); + public void testGoodFilter_IncludeInactive() { + final String idzId = randomUUID().toString(); + arrangeCurrentIdentityZone(idzId); + + final String filter = "(username eq \"foo\" and id eq \"bar\") or username eq \"bar\""; + + final List allScimUsers = new ArrayList<>(); + for (int i = 0; i < 5; ++i) { + final ScimUser scimUser = new ScimUser(randomUUID().toString(), "bar", "Some", "Name"); + scimUser.setOrigin("idp2"); + allScimUsers.add(scimUser); + } + final ScimUser scimUser6 = new ScimUser("bar", "foo", "Some", "Name"); + scimUser6.setOrigin("idp1"); + allScimUsers.add(scimUser6); + assertThat(allScimUsers).hasSize(6); + arrangeScimUsersForFilter(filter, idzId, allScimUsers); + + // only IdP 1 is active + arrangeActiveIdps(idzId, "idp1"); + + // check different page sizes -> should return all users, since 'includeInactive' is true + assertEndpointReturnsCorrectResult(filter, 1, allScimUsers, true); + assertEndpointReturnsCorrectResult(filter, 2, allScimUsers, true); + assertEndpointReturnsCorrectResult(filter, 3, allScimUsers, true); + assertEndpointReturnsCorrectResult(filter, 4, allScimUsers, true); + assertEndpointReturnsCorrectResult(filter, 10, allScimUsers, true); + } + + @Test + public void testGoodFilter_OnlyActive() { + final String idzId = randomUUID().toString(); + arrangeCurrentIdentityZone(idzId); + + final String filter = "(username eq \"foo\" and id eq \"bar\") or username eq \"bar\""; + + final List allScimUsers = new ArrayList<>(); + for (int i = 0; i < 5; ++i) { + final ScimUser scimUser = new ScimUser(randomUUID().toString(), "bar", "Some", "Name"); + scimUser.setOrigin("idp2"); + allScimUsers.add(scimUser); + } + final ScimUser scimUser6 = new ScimUser("bar", "foo", "Some", "Name"); + scimUser6.setOrigin("idp1"); + allScimUsers.add(scimUser6); + assertThat(allScimUsers).hasSize(6); + arrangeScimUsersForFilter(filter, idzId, allScimUsers); + + // only IdP 1 is active -> only user 6 should be returned + arrangeActiveIdps(idzId, "idp1"); + final List expectedUsers = singletonList(scimUser6); + + // check different page sizes + assertEndpointReturnsCorrectResult(filter, 1, expectedUsers, false); + assertEndpointReturnsCorrectResult(filter, 2, expectedUsers, false); + assertEndpointReturnsCorrectResult(filter, 3, expectedUsers, false); + assertEndpointReturnsCorrectResult(filter, 4, expectedUsers, false); + assertEndpointReturnsCorrectResult(filter, 10, expectedUsers, false); } @Test public void testGoodFilter2() { + arrangeCurrentIdentityZone("uaa"); final ResponseEntity response = endpoints.findUsers( "id eq \"bar\" and (id eq \"foo\" and username eq \"bar\")", "ascending", @@ -154,18 +232,21 @@ public void testBadFilter5() { expected.expectMessage(containsString("Invalid operator.")); endpoints.findUsers("id gt \"foo\"", "ascending", 0, 100, false); } + @Test public void testBadFilter6() { expected.expect(ScimException.class); expected.expectMessage(containsString("Invalid operator.")); endpoints.findUsers("id gt \"foo\"", "ascending", 0, 100, false); } + @Test public void testBadFilter7() { expected.expect(ScimException.class); expected.expectMessage(containsString("Invalid operator.")); endpoints.findUsers("id lt \"foo\"", "ascending", 0, 100, false); } + @Test public void testBadFilter8() { expected.expect(ScimException.class); @@ -234,7 +315,8 @@ public void testBadFilter15() { @Test public void testDisabled() { - endpoints = new UserIdConversionEndpoints(provisioning, mockSecurityContextAccessor, scimUserEndpoints, false); + arrangeCurrentIdentityZone("uaa"); + endpoints = new UserIdConversionEndpoints(identityProviderProvisioning, mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, false); ResponseEntity response = endpoints.findUsers("id eq \"foo\"", "ascending", 0, 100, false); Assert.assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode()); Assert.assertEquals("Illegal Operation: Endpoint not enabled.", response.getBody()); @@ -242,8 +324,84 @@ public void testDisabled() { @Test public void noActiveIdps_ReturnsEmptyResources() { - when(provisioning.retrieveActive(anyString())).thenReturn(Collections.emptyList()); + arrangeCurrentIdentityZone("uaa"); + when(identityProviderProvisioning.retrieveActive(anyString())).thenReturn(Collections.emptyList()); SearchResults searchResults = (SearchResults) endpoints.findUsers("username eq \"foo\"", "ascending", 0, 100, false).getBody(); assertTrue(searchResults.getResources().isEmpty()); } + + private void arrangeCurrentIdentityZone(final String idzId) { + final IdentityZone identityZone = new IdentityZone(); + identityZone.setId(idzId); + idzHolderMockedStatic.when(IdentityZoneHolder::get).thenReturn(identityZone); + } + + private void arrangeActiveIdps(final String idzId, final String... idpIds) { + final List idps = Stream.of(idpIds).map(idpId -> { + final IdentityProvider idp = new IdentityProvider<>(); + idp.setActive(true); + idp.setOriginKey("idp1"); + return idp; + }).collect(toList()); + when(identityProviderProvisioning.retrieveActive(idzId)).thenReturn(idps); + } + + private void arrangeScimUsersForFilter(final String filter, final String idzId, final List allScimUsers) { + when(scimUserProvisioning.query(filter, "userName", true, idzId)) + .thenReturn(allScimUsers); + } + + private void assertEndpointReturnsCorrectResult( + final String filter, + final int resultsPerPage, + final List expectedUsers, + final boolean includeInactive + ) { + final boolean lastPageIncomplete = expectedUsers.size() % resultsPerPage != 0; + final int expectedPages = expectedUsers.size() / resultsPerPage + (lastPageIncomplete ? 1 : 0); + + final Function> fetchNextPage = (startIndex) -> endpoints.findUsers( + filter, "ascending", startIndex, resultsPerPage, includeInactive + ); + + // collect all users in several pages + final List> observedUsers = new ArrayList<>(); + int currentStartIndex = 1; + for (int i = 0; i < expectedPages; i++) { + final ResponseEntity response = fetchNextPage.apply(currentStartIndex); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); + assertThat(response.getBody()).isNotNull().isInstanceOf(SearchResults.class); + final SearchResults> responseBody = (SearchResults>) response.getBody(); + assertThat(responseBody.getTotalResults()).isEqualTo(expectedUsers.size()); + + final int expectedNumberOfResultsInPage; + if (i == expectedPages - 1 && lastPageIncomplete) { + // last page -> might contain less elements + expectedNumberOfResultsInPage = expectedUsers.size() % resultsPerPage; + } else { + // complete page + expectedNumberOfResultsInPage = resultsPerPage; + } + assertThat(responseBody.getResources()).hasSize(expectedNumberOfResultsInPage); + + observedUsers.addAll(responseBody.getResources()); + currentStartIndex += responseBody.getResources().size(); + } + + // check next page -> should be empty + final ResponseEntity response = fetchNextPage.apply(currentStartIndex); + assertThat(response.getBody()).isNotNull(); + assertThat(response.getBody()).isNotNull().isInstanceOf(SearchResults.class); + final SearchResults> responseBody = (SearchResults>) response.getBody(); + assertThat(responseBody.getTotalResults()).isEqualTo(expectedUsers.size()); + assertThat(responseBody.getResources()).isNotNull().isEmpty(); + + final List> expectedResponse = expectedUsers.stream().map(scimUser -> Map.of( + "id", (Object) scimUser.getId(), + "userName", scimUser.getUserName(), + "origin", scimUser.getOrigin() + )).collect(toList()); + + assertThat(observedUsers).hasSameElementsAs(expectedResponse); + } } From d7cd323e51b7f938d563ea1e594e84d00e53d7c6 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Fri, 2 Feb 2024 15:11:42 +0100 Subject: [PATCH 03/42] Revert removing generic wildcard from findUsers response type --- .../identity/uaa/scim/endpoints/ScimUserEndpoints.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java index f26d00133a7..6436e0a0e99 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java @@ -385,7 +385,7 @@ private int getVersion(String userId, String etag) { @RequestMapping(value = "/Users", method = RequestMethod.GET) @ResponseBody - public SearchResults findUsers( + public SearchResults findUsers( @RequestParam(value = "attributes", required = false) String attributesCommaSeparated, @RequestParam(required = false, defaultValue = "id pr") String filter, @RequestParam(required = false, defaultValue = "created") String sortBy, From 395dee51e54d96699ce312ea6d27373c20232111 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Fri, 2 Feb 2024 16:07:49 +0100 Subject: [PATCH 04/42] Move reading all users for a given filter to a separate method in ScimUserEndpoints and reuse it in UserIdConversionEndpoints --- .../uaa/scim/endpoints/ScimUserEndpoints.java | 44 ++++++++++++------- .../endpoints/UserIdConversionEndpoints.java | 20 +-------- .../UserIdConversionEndpointsTests.java | 15 +++---- 3 files changed, 36 insertions(+), 43 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java index 6436e0a0e99..513230a82b9 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java @@ -1,6 +1,8 @@ package org.cloudfoundry.identity.uaa.scim.endpoints; import com.jayway.jsonpath.JsonPathException; + +import org.apache.commons.lang3.tuple.Pair; import org.cloudfoundry.identity.uaa.account.UserAccountStatus; import org.cloudfoundry.identity.uaa.account.event.UserAccountUnlockedEvent; import org.cloudfoundry.identity.uaa.approval.Approval; @@ -401,26 +403,18 @@ public SearchResults findUsers( count = userMaxCount; } + List result = getScimUsers(filter, sortBy, sortOrder); + List input = new ArrayList<>(); - List result; Set attributes = StringUtils.commaDelimitedListToSet(attributesCommaSeparated); - try { - result = scimUserProvisioning.query(filter, sortBy, sortOrder.equals("ascending"), identityZoneManager.getCurrentIdentityZoneId()); - for (ScimUser user : UaaPagingUtils.subList(result, startIndex, count)) { - if (attributes.isEmpty() || attributes.stream().anyMatch("groups"::equalsIgnoreCase)) { - syncGroups(user); - } - if (attributes.isEmpty() || attributes.stream().anyMatch("approvals"::equalsIgnoreCase)) { - syncApprovals(user); - } - input.add(user); + for (final ScimUser user : UaaPagingUtils.subList(result, startIndex, count)) { + if (attributes.isEmpty() || attributes.stream().anyMatch("groups"::equalsIgnoreCase)) { + syncGroups(user); } - } catch (IllegalArgumentException e) { - String msg = "Invalid filter expression: [" + filter + "]"; - if (StringUtils.hasText(sortBy)) { - msg += " [" + sortBy + "]"; + if (attributes.isEmpty() || attributes.stream().anyMatch("approvals"::equalsIgnoreCase)) { + syncApprovals(user); } - throw new ScimException(HtmlUtils.htmlEscape(msg), HttpStatus.BAD_REQUEST); + input.add(user); } if (!StringUtils.hasLength(attributesCommaSeparated)) { @@ -448,6 +442,24 @@ public SearchResults findUsers( } } + public List getScimUsers( + final String filter, + final String sortBy, + final String sortOrder + ) { + final List result; + try { + result = scimUserProvisioning.query(filter, sortBy, sortOrder.equals("ascending"), identityZoneManager.getCurrentIdentityZoneId()); + } catch (final IllegalArgumentException e) { + String msg = "Invalid filter expression: [" + filter + "]"; + if (StringUtils.hasText(sortBy)) { + msg += " [" + sortBy + "]"; + } + throw new ScimException(HtmlUtils.htmlEscape(msg), HttpStatus.BAD_REQUEST); + } + return result; + } + @RequestMapping(value = "/Users/{userId}/status", method = RequestMethod.PATCH) public UserAccountStatus updateAccountStatus(@RequestBody UserAccountStatus status, @PathVariable String userId) { ScimUser user = scimUserProvisioning.retrieve(userId, identityZoneManager.getCurrentIdentityZoneId()); diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java index d5cac58d60f..d59d17f3d2e 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java @@ -17,7 +17,6 @@ import org.cloudfoundry.identity.uaa.resources.SearchResultsFactory; import org.cloudfoundry.identity.uaa.scim.ScimCore; import org.cloudfoundry.identity.uaa.scim.ScimUser; -import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning; import org.cloudfoundry.identity.uaa.scim.exception.ScimException; import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor; import org.cloudfoundry.identity.uaa.util.UaaPagingUtils; @@ -32,7 +31,6 @@ import org.springframework.http.ResponseEntity; import org.springframework.stereotype.Controller; import org.springframework.util.Assert; -import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; @@ -51,7 +49,6 @@ public class UserIdConversionEndpoints implements InitializingBean { private final IdentityProviderProvisioning identityProviderProvisioning; private final SecurityContextAccessor securityContextAccessor; private final ScimUserEndpoints scimUserEndpoints; - private final ScimUserProvisioning scimUserProvisioning; private boolean enabled; @@ -59,14 +56,12 @@ public UserIdConversionEndpoints( final @Qualifier("identityProviderProvisioning") IdentityProviderProvisioning identityProviderProvisioning, final SecurityContextAccessor securityContextAccessor, final ScimUserEndpoints scimUserEndpoints, - final @Qualifier("scimUserProvisioning") ScimUserProvisioning scimUserProvisioning, final @Value("${scim.userids_enabled:true}") boolean enabled ) { this.identityProviderProvisioning = identityProviderProvisioning; this.securityContextAccessor = securityContextAccessor; this.scimUserEndpoints = scimUserEndpoints; this.enabled = enabled; - this.scimUserProvisioning = scimUserProvisioning; } @RequestMapping(value = "/ids/Users") @@ -127,25 +122,14 @@ private List getFilteredScimUsers( final String sortOrder, final boolean includeInactive ) { - final String idzId = IdentityZoneHolder.get().getId(); - - final List filteredScimUsers; - try { - filteredScimUsers = scimUserProvisioning.query(filter, "userName", sortOrder.equals("ascending"), idzId); - } catch (final IllegalArgumentException e) { - String msg = "Invalid filter expression: [" + filter + "]"; - if (StringUtils.hasText("userName")) { - msg += " [" + "userName" + "]"; - } - throw new ScimException(HtmlUtils.htmlEscape(msg), HttpStatus.BAD_REQUEST); - } + final List filteredScimUsers = scimUserEndpoints.getScimUsers(filter, "userName", sortOrder); if (includeInactive) { return filteredScimUsers; } // remove users from inactive IdPs - final List activeIdentityProviders = identityProviderProvisioning.retrieveActive(idzId); + final List activeIdentityProviders = identityProviderProvisioning.retrieveActive(IdentityZoneHolder.get().getId()); if (activeIdentityProviders.isEmpty()) { return emptyList(); } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java index e848ea22fbb..0019e05abfd 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java @@ -38,7 +38,6 @@ import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning; import org.cloudfoundry.identity.uaa.resources.SearchResults; import org.cloudfoundry.identity.uaa.scim.ScimUser; -import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning; import org.cloudfoundry.identity.uaa.scim.exception.ScimException; import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor; import org.cloudfoundry.identity.uaa.zone.IdentityZone; @@ -70,7 +69,6 @@ public class UserIdConversionEndpointsTests { private SecurityContextAccessor mockSecurityContextAccessor; private final ScimUserEndpoints scimUserEndpoints = mock(ScimUserEndpoints.class); - private final ScimUserProvisioning scimUserProvisioning = mock(ScimUserProvisioning.class); private final MockedStatic idzHolderMockedStatic; @@ -86,7 +84,7 @@ public UserIdConversionEndpointsTests() { @Before public void init() { mockSecurityContextAccessor = mock(SecurityContextAccessor.class); - endpoints = new UserIdConversionEndpoints(identityProviderProvisioning, mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, true); + endpoints = new UserIdConversionEndpoints(identityProviderProvisioning, mockSecurityContextAccessor, scimUserEndpoints, true); when(mockSecurityContextAccessor.getAuthorities()).thenReturn(authorities); when(mockSecurityContextAccessor.getAuthenticationInfo()).thenReturn("mock object"); when(scimUserEndpoints.getUserMaxCount()).thenReturn(10_000); @@ -141,7 +139,7 @@ public void testGoodFilter_IncludeInactive() { scimUser6.setOrigin("idp1"); allScimUsers.add(scimUser6); assertThat(allScimUsers).hasSize(6); - arrangeScimUsersForFilter(filter, idzId, allScimUsers); + arrangeScimUsersForFilter(filter, allScimUsers); // only IdP 1 is active arrangeActiveIdps(idzId, "idp1"); @@ -170,8 +168,7 @@ public void testGoodFilter_OnlyActive() { final ScimUser scimUser6 = new ScimUser("bar", "foo", "Some", "Name"); scimUser6.setOrigin("idp1"); allScimUsers.add(scimUser6); - assertThat(allScimUsers).hasSize(6); - arrangeScimUsersForFilter(filter, idzId, allScimUsers); + arrangeScimUsersForFilter(filter, allScimUsers); // only IdP 1 is active -> only user 6 should be returned arrangeActiveIdps(idzId, "idp1"); @@ -316,7 +313,7 @@ public void testBadFilter15() { @Test public void testDisabled() { arrangeCurrentIdentityZone("uaa"); - endpoints = new UserIdConversionEndpoints(identityProviderProvisioning, mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, false); + endpoints = new UserIdConversionEndpoints(identityProviderProvisioning, mockSecurityContextAccessor, scimUserEndpoints, false); ResponseEntity response = endpoints.findUsers("id eq \"foo\"", "ascending", 0, 100, false); Assert.assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode()); Assert.assertEquals("Illegal Operation: Endpoint not enabled.", response.getBody()); @@ -346,8 +343,8 @@ private void arrangeActiveIdps(final String idzId, final String... idpIds) { when(identityProviderProvisioning.retrieveActive(idzId)).thenReturn(idps); } - private void arrangeScimUsersForFilter(final String filter, final String idzId, final List allScimUsers) { - when(scimUserProvisioning.query(filter, "userName", true, idzId)) + private void arrangeScimUsersForFilter(final String filter, final List allScimUsers) { + when(scimUserEndpoints.getScimUsers(filter, "userName", "ascending")) .thenReturn(allScimUsers); } From 95457f1e43f1f1f1ab5eed8a9b4869a29c40585d Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Fri, 2 Feb 2024 16:10:17 +0100 Subject: [PATCH 05/42] Remove unused import --- .../identity/uaa/scim/endpoints/ScimUserEndpoints.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java index 513230a82b9..77e86b3f052 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java @@ -1,8 +1,6 @@ package org.cloudfoundry.identity.uaa.scim.endpoints; import com.jayway.jsonpath.JsonPathException; - -import org.apache.commons.lang3.tuple.Pair; import org.cloudfoundry.identity.uaa.account.UserAccountStatus; import org.cloudfoundry.identity.uaa.account.event.UserAccountUnlockedEvent; import org.cloudfoundry.identity.uaa.approval.Approval; From c2dc58d20f4e32e6b6c0fa1757b7e965ee00804d Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 13:26:59 +0100 Subject: [PATCH 06/42] Add javadoc comment to SimpleSearchQueryConverter#getWhereClause --- .../uaa/resources/jdbc/SimpleSearchQueryConverter.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/SimpleSearchQueryConverter.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/SimpleSearchQueryConverter.java index 368eb3326f9..b1774ba674a 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/SimpleSearchQueryConverter.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/SimpleSearchQueryConverter.java @@ -9,6 +9,7 @@ import org.cloudfoundry.identity.uaa.util.AlphanumericRandomValueStringGenerator; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.springframework.lang.Nullable; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.util.StringUtils; @@ -125,9 +126,12 @@ private String generateParameterPrefix(String filter) { } } + /** + * @return the WHERE and (optional) ORDER BY clauses WITHOUT the "WHERE" keyword in the beginning + */ private String getWhereClause( - final String filter, - final String sortBy, + @Nullable final String filter, + @Nullable final String sortBy, final boolean ascending, final Map values, final AttributeNameMapper mapper, From 8d139c8ca919a15044840d3221de4e32e89b3b50 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 13:28:32 +0100 Subject: [PATCH 07/42] Add method JdbcScimUserProvisioning#retrieveByScimFilter for filtering users by SCIM filter and active flag --- .../uaa/scim/ScimUserProvisioning.java | 8 ++ .../scim/jdbc/JdbcScimUserProvisioning.java | 103 +++++++++++++----- 2 files changed, 85 insertions(+), 26 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUserProvisioning.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUserProvisioning.java index 93965e0f219..749380f6b89 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUserProvisioning.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUserProvisioning.java @@ -29,6 +29,14 @@ public interface ScimUserProvisioning extends ResourceManager, Queryab List retrieveByUsernameAndZone(String username, String zoneId); + List retrieveByScimFilter( + String filter, + boolean includeInactive, + String sortBy, + boolean ascending, + String zoneId + ); + List retrieveByUsernameAndOriginAndZone(String username, String origin, String zoneId); void changePassword(String id, String oldPassword, String newPassword, String zoneId) throws ScimResourceNotFoundException; diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java index cc22c92f194..7bcd8623aa5 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java @@ -12,19 +12,32 @@ *******************************************************************************/ package org.cloudfoundry.identity.uaa.scim.jdbc; -import org.cloudfoundry.identity.uaa.util.UaaStringUtils; -import org.cloudfoundry.identity.uaa.zone.IdentityZone; -import org.cloudfoundry.identity.uaa.zone.JdbcIdentityZoneProvisioning; -import org.cloudfoundry.identity.uaa.zone.UserConfig; -import org.cloudfoundry.identity.uaa.zone.ZoneDoesNotExistsException; -import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import static java.sql.Types.VARCHAR; +import static java.util.stream.Collectors.joining; +import static org.springframework.util.StringUtils.hasText; + +import javax.annotation.Nullable; + +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Timestamp; +import java.sql.Types; +import java.util.Arrays; +import java.util.Calendar; +import java.util.Date; +import java.util.GregorianCalendar; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import java.util.regex.Pattern; + import org.cloudfoundry.identity.uaa.audit.event.SystemDeletable; import org.cloudfoundry.identity.uaa.constants.OriginKeys; import org.cloudfoundry.identity.uaa.resources.ResourceMonitor; import org.cloudfoundry.identity.uaa.resources.jdbc.AbstractQueryable; import org.cloudfoundry.identity.uaa.resources.jdbc.JdbcPagingListFactory; +import org.cloudfoundry.identity.uaa.resources.jdbc.SearchQueryConverter.ProcessedFilter; import org.cloudfoundry.identity.uaa.resources.jdbc.SimpleSearchQueryConverter; import org.cloudfoundry.identity.uaa.scim.ScimMeta; import org.cloudfoundry.identity.uaa.scim.ScimUser; @@ -39,34 +52,25 @@ import org.cloudfoundry.identity.uaa.user.JdbcUaaUserDatabase; import org.cloudfoundry.identity.uaa.util.TimeService; import org.cloudfoundry.identity.uaa.util.TimeServiceImpl; +import org.cloudfoundry.identity.uaa.util.UaaStringUtils; +import org.cloudfoundry.identity.uaa.zone.IdentityZone; +import org.cloudfoundry.identity.uaa.zone.JdbcIdentityZoneProvisioning; +import org.cloudfoundry.identity.uaa.zone.UserConfig; +import org.cloudfoundry.identity.uaa.zone.ZoneDoesNotExistsException; +import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.dao.DuplicateKeyException; import org.springframework.dao.EmptyResultDataAccessException; import org.springframework.dao.IncorrectResultSizeDataAccessException; import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.jdbc.core.RowMapper; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; import org.springframework.security.authentication.BadCredentialsException; import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.util.Assert; -import java.sql.ResultSet; -import java.sql.SQLException; -import java.sql.Timestamp; -import java.sql.Types; -import java.util.Calendar; -import java.util.Date; -import java.util.GregorianCalendar; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.UUID; -import java.util.regex.Pattern; - -import javax.annotation.Nullable; - -import static java.sql.Types.VARCHAR; -import static org.springframework.util.StringUtils.hasText; - public class JdbcScimUserProvisioning extends AbstractQueryable implements ScimUserProvisioning, ResourceMonitor, SystemDeletable { @@ -172,6 +176,53 @@ public List retrieveByUsernameAndZone(String username, String zoneId) return jdbcTemplate.query(USER_BY_USERNAME_AND_ZONE_QUERY , mapper, username, zoneId); } + /** + * This method should behave similar to {@link JdbcScimUserProvisioning#query(String, String, boolean, String)}, + * with the filtering for active IdPs (if required) being the only difference. + */ + @Override + public List retrieveByScimFilter( + final String filter, + final boolean includeInactive, + final String sortBy, + final boolean ascending, + final String zoneId + ) { + if (includeInactive) { + // no filtering for active IdPs necessary + return query(filter, sortBy, ascending, zoneId); + } + + final SimpleSearchQueryConverter queryConverter = new SimpleSearchQueryConverter(); + validateOrderBy(queryConverter.map(sortBy)); + + // build WHERE clause + final ProcessedFilter where = queryConverter.convert(filter, sortBy, ascending, zoneId); + final String whereClauseScimFilter = where.getSql(); + String whereClause = "idp.active is true and ("; + if (where.hasOrderBy()) { + whereClause += whereClauseScimFilter.replace(ProcessedFilter.ORDER_BY, ")" + ProcessedFilter.ORDER_BY); + } else { + whereClause += whereClauseScimFilter + ")"; + } + + final String userFieldsWithPrefix = Arrays.stream(USER_FIELDS.split(",")) + .map(field -> "u." + field) + .collect(joining(", ")); + final String sql = String.format( + "select %s from users u join identity_provider idp on u.origin = idp.origin_key and u.identity_zone_id = idp.identity_zone_id where %s", + userFieldsWithPrefix, + whereClause + ); + + if (getPageSize() > 0 && getPageSize() < Integer.MAX_VALUE) { + return pagingListFactory.createJdbcPagingList(sql, where.getParams(), rowMapper, getPageSize()); + } + + final NamedParameterJdbcTemplate namedParameterJdbcTemplate = new NamedParameterJdbcTemplate(jdbcTemplate); + return namedParameterJdbcTemplate.query(sql, where.getParams(), rowMapper); + } + @Override public List retrieveByUsernameAndOriginAndZone(String username, String origin, String zoneId) { return jdbcTemplate.query(USER_BY_USERNAME_AND_ORIGIN_AND_ZONE_QUERY , mapper, username, origin, zoneId); From 8a75e22c0e659909f9e008004ea3a1f6a2a296c3 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 13:29:28 +0100 Subject: [PATCH 08/42] Fix visibility of AbstractQueryable#pagingListFactory --- .../identity/uaa/resources/jdbc/AbstractQueryable.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index 6d6b42a6dd1..28292abf8b2 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -19,7 +19,7 @@ public abstract class AbstractQueryable implements Queryable { private NamedParameterJdbcTemplate namedParameterJdbcTemplate; - private JdbcPagingListFactory pagingListFactory; + protected final JdbcPagingListFactory pagingListFactory; protected RowMapper rowMapper; From 2598f6461befe143e397de9bfe43bb8bff13a4f9 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 13:33:43 +0100 Subject: [PATCH 09/42] Use new method for filtering SCIM users in UserIdConversionEndpoints --- .../uaa/scim/endpoints/ScimUserEndpoints.java | 29 ++--- .../endpoints/UserIdConversionEndpoints.java | 47 ++------ .../UserIdConversionEndpointsTests.java | 59 +++------- .../jdbc/JdbcScimUserProvisioningTests.java | 111 +++++++++++++----- 4 files changed, 125 insertions(+), 121 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java index 457c826f419..73b07f1737c 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java @@ -397,7 +397,16 @@ public SearchResults findUsers( count = userMaxCount; } - List result = getScimUsers(filter, sortBy, sortOrder); + final List result; + try { + result = scimUserProvisioning.query(filter, sortBy, sortOrder.equals("ascending"), identityZoneManager.getCurrentIdentityZoneId()); + } catch (final IllegalArgumentException e1) { + String msg = "Invalid filter expression: [" + filter + "]"; + if (StringUtils.hasText(sortBy)) { + msg += " [" + sortBy + "]"; + } + throw new ScimException(HtmlUtils.htmlEscape(msg), HttpStatus.BAD_REQUEST); + } List input = new ArrayList<>(); Set attributes = StringUtils.commaDelimitedListToSet(attributesCommaSeparated); @@ -436,24 +445,6 @@ public SearchResults findUsers( } } - public List getScimUsers( - final String filter, - final String sortBy, - final String sortOrder - ) { - final List result; - try { - result = scimUserProvisioning.query(filter, sortBy, sortOrder.equals("ascending"), identityZoneManager.getCurrentIdentityZoneId()); - } catch (final IllegalArgumentException e) { - String msg = "Invalid filter expression: [" + filter + "]"; - if (StringUtils.hasText(sortBy)) { - msg += " [" + sortBy + "]"; - } - throw new ScimException(HtmlUtils.htmlEscape(msg), HttpStatus.BAD_REQUEST); - } - return result; - } - @RequestMapping(value = "/Users/{userId}/status", method = RequestMethod.PATCH) public UserAccountStatus updateAccountStatus(@RequestBody UserAccountStatus status, @PathVariable String userId) { ScimUser user = scimUserProvisioning.retrieve(userId, identityZoneManager.getCurrentIdentityZoneId()); diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java index cfd2c373a68..5834fc5ac44 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java @@ -1,22 +1,18 @@ package org.cloudfoundry.identity.uaa.scim.endpoints; -import static java.util.Collections.emptyList; import static java.util.stream.Collectors.toList; -import static java.util.stream.Collectors.toSet; import javax.servlet.http.HttpServletRequest; import java.util.Arrays; import java.util.List; import java.util.Map; -import java.util.Set; import org.cloudfoundry.identity.uaa.constants.OriginKeys; -import org.cloudfoundry.identity.uaa.provider.IdentityProvider; -import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning; import org.cloudfoundry.identity.uaa.resources.SearchResultsFactory; import org.cloudfoundry.identity.uaa.scim.ScimCore; import org.cloudfoundry.identity.uaa.scim.ScimUser; +import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning; import org.cloudfoundry.identity.uaa.scim.exception.ScimException; import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor; import org.cloudfoundry.identity.uaa.util.UaaPagingUtils; @@ -46,21 +42,20 @@ public class UserIdConversionEndpoints implements InitializingBean { private final Logger logger = LoggerFactory.getLogger(getClass()); - private final IdentityProviderProvisioning identityProviderProvisioning; + private final ScimUserProvisioning scimUserProvisioning; private final SecurityContextAccessor securityContextAccessor; private final ScimUserEndpoints scimUserEndpoints; - - private boolean enabled; + private final boolean enabled; public UserIdConversionEndpoints( - final @Qualifier("identityProviderProvisioning") IdentityProviderProvisioning identityProviderProvisioning, final SecurityContextAccessor securityContextAccessor, final ScimUserEndpoints scimUserEndpoints, + final @Qualifier("scimUserProvisioning") ScimUserProvisioning scimUserProvisioning, final @Value("${scim.userids_enabled:true}") boolean enabled ) { - this.identityProviderProvisioning = identityProviderProvisioning; this.securityContextAccessor = securityContextAccessor; this.scimUserEndpoints = scimUserEndpoints; + this.scimUserProvisioning = scimUserProvisioning; this.enabled = enabled; } @@ -92,7 +87,13 @@ public ResponseEntity findUsers( checkFilter(filter); // get all users for the given filter and the current page - final List filteredUsers = getFilteredScimUsers(filter, sortOrder, includeInactive); + final List filteredUsers = scimUserProvisioning.retrieveByScimFilter( + filter, + includeInactive, + "userName", + sortOrder.equalsIgnoreCase("ascending"), + IdentityZoneHolder.getCurrentZoneId() + ); final List usersCurrentPage = UaaPagingUtils.subList(filteredUsers, startIndex, count); // map to result structure @@ -117,30 +118,6 @@ public ResponseEntity findUsers( ); } - private List getFilteredScimUsers( - final String filter, - final String sortOrder, - final boolean includeInactive - ) { - final List filteredScimUsers = scimUserEndpoints.getScimUsers(filter, "userName", sortOrder); - - if (includeInactive) { - return filteredScimUsers; - } - - // remove users from inactive IdPs - final List activeIdentityProviders = identityProviderProvisioning.retrieveActive(IdentityZoneHolder.get().getId()); - if (activeIdentityProviders.isEmpty()) { - return emptyList(); - } - final Set originsOfActiveIdps = activeIdentityProviders.stream() - .map(IdentityProvider::getOriginKey) - .collect(toSet()); - return filteredScimUsers.stream() - .filter(scimUser -> originsOfActiveIdps.contains(scimUser.getOrigin())) - .collect(toList()); - } - @ExceptionHandler public View handleException(Exception t, HttpServletRequest request) throws ScimException { return scimUserEndpoints.handleException(t, request); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java index 753e08b25cf..4397f1d1639 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java @@ -21,23 +21,19 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; -import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.when; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.List; import java.util.Map; import java.util.function.Function; -import java.util.stream.Stream; -import org.cloudfoundry.identity.uaa.provider.IdentityProvider; -import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning; import org.cloudfoundry.identity.uaa.resources.SearchResults; import org.cloudfoundry.identity.uaa.scim.ScimUser; +import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning; import org.cloudfoundry.identity.uaa.scim.exception.ScimException; import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor; import org.cloudfoundry.identity.uaa.zone.IdentityZone; @@ -62,14 +58,14 @@ public class UserIdConversionEndpointsTests { @Rule public ExpectedException expected = ExpectedException.none(); - private final IdentityProviderProvisioning identityProviderProvisioning = mock(IdentityProviderProvisioning.class); - private UserIdConversionEndpoints endpoints; private SecurityContextAccessor mockSecurityContextAccessor; private final ScimUserEndpoints scimUserEndpoints = mock(ScimUserEndpoints.class); + private final ScimUserProvisioning scimUserProvisioning = mock(ScimUserProvisioning.class); + private final MockedStatic idzHolderMockedStatic; @SuppressWarnings("rawtypes") @@ -84,7 +80,7 @@ public UserIdConversionEndpointsTests() { @Before public void init() { mockSecurityContextAccessor = mock(SecurityContextAccessor.class); - endpoints = new UserIdConversionEndpoints(identityProviderProvisioning, mockSecurityContextAccessor, scimUserEndpoints, true); + endpoints = new UserIdConversionEndpoints(mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, true); when(mockSecurityContextAccessor.getAuthorities()).thenReturn(authorities); when(mockSecurityContextAccessor.getAuthenticationInfo()).thenReturn("mock object"); when(scimUserEndpoints.getUserMaxCount()).thenReturn(10_000); @@ -139,10 +135,7 @@ public void testGoodFilter_IncludeInactive() { scimUser6.setOrigin("idp1"); allScimUsers.add(scimUser6); assertThat(allScimUsers).hasSize(6); - arrangeScimUsersForFilter(filter, allScimUsers); - - // only IdP 1 is active - arrangeActiveIdps(idzId, "idp1"); + arrangeScimUsersForFilter(filter, allScimUsers, true, idzId); // check different page sizes -> should return all users, since 'includeInactive' is true assertEndpointReturnsCorrectResult(filter, 1, allScimUsers, true); @@ -159,20 +152,11 @@ public void testGoodFilter_OnlyActive() { final String filter = "(username eq \"foo\" and id eq \"bar\") or username eq \"bar\""; - final List allScimUsers = new ArrayList<>(); - for (int i = 0; i < 5; ++i) { - final ScimUser scimUser = new ScimUser(randomUUID().toString(), "bar", "Some", "Name"); - scimUser.setOrigin("idp2"); - allScimUsers.add(scimUser); - } - final ScimUser scimUser6 = new ScimUser("bar", "foo", "Some", "Name"); - scimUser6.setOrigin("idp1"); - allScimUsers.add(scimUser6); - arrangeScimUsersForFilter(filter, allScimUsers); - - // only IdP 1 is active -> only user 6 should be returned - arrangeActiveIdps(idzId, "idp1"); - final List expectedUsers = singletonList(scimUser6); + // one active user + final ScimUser scimUser = new ScimUser("bar", "foo", "Some", "Name"); + scimUser.setOrigin("idp1"); + final List expectedUsers = singletonList(scimUser); + arrangeScimUsersForFilter(filter, expectedUsers, false, idzId); // check different page sizes assertEndpointReturnsCorrectResult(filter, 1, expectedUsers, false); @@ -316,7 +300,7 @@ public void testBadFilter11() { @Test public void testDisabled() { arrangeCurrentIdentityZone("uaa"); - endpoints = new UserIdConversionEndpoints(identityProviderProvisioning, mockSecurityContextAccessor, scimUserEndpoints, false); + endpoints = new UserIdConversionEndpoints(mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, false); ResponseEntity response = endpoints.findUsers("id eq \"foo\"", "ascending", 0, 100, false); Assert.assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode()); Assert.assertEquals("Illegal Operation: Endpoint not enabled.", response.getBody()); @@ -325,7 +309,6 @@ public void testDisabled() { @Test public void noActiveIdps_ReturnsEmptyResources() { arrangeCurrentIdentityZone("uaa"); - when(identityProviderProvisioning.retrieveActive(anyString())).thenReturn(Collections.emptyList()); SearchResults searchResults = (SearchResults) endpoints.findUsers("username eq \"foo\"", "ascending", 0, 100, false).getBody(); assertTrue(searchResults.getResources().isEmpty()); } @@ -334,20 +317,16 @@ private void arrangeCurrentIdentityZone(final String idzId) { final IdentityZone identityZone = new IdentityZone(); identityZone.setId(idzId); idzHolderMockedStatic.when(IdentityZoneHolder::get).thenReturn(identityZone); + idzHolderMockedStatic.when(IdentityZoneHolder::getCurrentZoneId).thenReturn(idzId); } - private void arrangeActiveIdps(final String idzId, final String... idpIds) { - final List idps = Stream.of(idpIds).map(idpId -> { - final IdentityProvider idp = new IdentityProvider<>(); - idp.setActive(true); - idp.setOriginKey("idp1"); - return idp; - }).collect(toList()); - when(identityProviderProvisioning.retrieveActive(idzId)).thenReturn(idps); - } - - private void arrangeScimUsersForFilter(final String filter, final List allScimUsers) { - when(scimUserEndpoints.getScimUsers(filter, "userName", "ascending")) + private void arrangeScimUsersForFilter( + final String filter, + final List allScimUsers, + final boolean includeInactive, + final String zoneId + ) { + when(scimUserProvisioning.retrieveByScimFilter(filter, includeInactive, "userName", true, zoneId)) .thenReturn(allScimUsers); } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java index d1a07443069..b3383da1dd3 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java @@ -1,5 +1,35 @@ package org.cloudfoundry.identity.uaa.scim.jdbc; +import static java.util.stream.Collectors.toList; +import static org.cloudfoundry.identity.uaa.constants.OriginKeys.LOGIN_SERVER; +import static org.cloudfoundry.identity.uaa.constants.OriginKeys.UAA; +import static org.cloudfoundry.identity.uaa.util.AssertThrowsWithMessage.assertThrowsWithMessageThat; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.core.Is.is; +import static org.junit.Assert.fail; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; + +import java.sql.Timestamp; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import java.util.function.Function; + +import org.assertj.core.api.Assertions; import org.cloudfoundry.identity.uaa.annotations.WithDatabaseContext; import org.cloudfoundry.identity.uaa.audit.event.EntityDeletedEvent; import org.cloudfoundry.identity.uaa.constants.OriginKeys; @@ -32,33 +62,6 @@ import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.security.oauth2.common.util.RandomValueStringGenerator; -import java.sql.Timestamp; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.UUID; - -import static org.cloudfoundry.identity.uaa.constants.OriginKeys.LOGIN_SERVER; -import static org.cloudfoundry.identity.uaa.constants.OriginKeys.UAA; -import static org.cloudfoundry.identity.uaa.util.AssertThrowsWithMessage.assertThrowsWithMessageThat; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.core.Is.is; -import static org.junit.Assert.fail; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNotSame; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.when; - @WithDatabaseContext class JdbcScimUserProvisioningTests { @@ -231,6 +234,60 @@ void canDeleteProviderUsersInDefaultZone() { assertThat(jdbcTemplate.queryForObject("select count(*) from group_membership where member_id=?", new Object[]{created.getId()}, Integer.class), is(0)); } + @Test + void testRetrieveByScimFilter() { + final String origin1 = randomString(); + addIdentityProvider(jdbcTemplate, currentIdentityZoneId, origin1); + + final String origin2 = randomString(); + addIdentityProvider(jdbcTemplate, currentIdentityZoneId, origin2); + + final ScimUser user1 = new ScimUser(null, "jo@foo.com", "Jo", "User"); + user1.addEmail("jo@blah.com"); + user1.setOrigin(origin1); + final ScimUser created1 = jdbcScimUserProvisioning.createUser(user1, "j7hyqpassX", currentIdentityZoneId); + + final ScimUser user2 = new ScimUser(null, "jo2@foo.com", "Jo", "User"); + user2.addEmail("jo2@blah.com"); + user2.setOrigin(origin2); + final ScimUser created2 = jdbcScimUserProvisioning.createUser(user2, "j7hyqpassX", currentIdentityZoneId); + + final Function> retrieveByScimFilter = (scimFilter) -> { + final List result = jdbcScimUserProvisioning.retrieveByScimFilter( + scimFilter, + false, + "userName", + true, + currentIdentityZoneId + ); + Assertions.assertThat(result).isNotNull(); + final List usernames = result.stream().map(ScimUser::getUserName).collect(toList()); + Assertions.assertThat(usernames).isSorted(); + return usernames; + }; + + // case 1: should return both + String filter = String.format("id eq '%s' or origin eq '%s'", created1.getId(), created2.getOrigin()); + List usernames = retrieveByScimFilter.apply(filter); + Assertions.assertThat(usernames) + .hasSize(2) + .contains(created1.getUserName(), created2.getUserName()); + + // case 2: should return user 2 + filter = String.format("origin eq '%s'", created2.getOrigin()); + usernames = retrieveByScimFilter.apply(filter); + Assertions.assertThat(usernames) + .hasSize(1) + .contains(created2.getUserName()); + + // case 3: should return user 2 (filtered by origin and ID) + filter = String.format("origin eq '%s' and id eq '%s'", created2.getOrigin(), created2.getId()); + usernames = retrieveByScimFilter.apply(filter); + Assertions.assertThat(usernames) + .hasSize(1) + .contains(created2.getUserName()); + } + @Test void canDeleteProviderUsersInOtherZone() { ScimUser user = new ScimUser(null, "jo@foo.com", "Jo", "User"); From cf645620060483a402f5807a37256860816bae37 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 13:36:44 +0100 Subject: [PATCH 10/42] Revert obsolete changes to ScimUserEndpoints --- .../uaa/scim/endpoints/ScimUserEndpoints.java | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java index 73b07f1737c..2239aa2590b 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java @@ -397,10 +397,21 @@ public SearchResults findUsers( count = userMaxCount; } - final List result; + List input = new ArrayList<>(); + List result; + Set attributes = StringUtils.commaDelimitedListToSet(attributesCommaSeparated); try { result = scimUserProvisioning.query(filter, sortBy, sortOrder.equals("ascending"), identityZoneManager.getCurrentIdentityZoneId()); - } catch (final IllegalArgumentException e1) { + for (ScimUser user : UaaPagingUtils.subList(result, startIndex, count)) { + if (attributes.isEmpty() || attributes.stream().anyMatch("groups"::equalsIgnoreCase)) { + syncGroups(user); + } + if (attributes.isEmpty() || attributes.stream().anyMatch("approvals"::equalsIgnoreCase)) { + syncApprovals(user); + } + input.add(user); + } + } catch (IllegalArgumentException e) { String msg = "Invalid filter expression: [" + filter + "]"; if (StringUtils.hasText(sortBy)) { msg += " [" + sortBy + "]"; @@ -408,18 +419,6 @@ public SearchResults findUsers( throw new ScimException(HtmlUtils.htmlEscape(msg), HttpStatus.BAD_REQUEST); } - List input = new ArrayList<>(); - Set attributes = StringUtils.commaDelimitedListToSet(attributesCommaSeparated); - for (final ScimUser user : UaaPagingUtils.subList(result, startIndex, count)) { - if (attributes.isEmpty() || attributes.stream().anyMatch("groups"::equalsIgnoreCase)) { - syncGroups(user); - } - if (attributes.isEmpty() || attributes.stream().anyMatch("approvals"::equalsIgnoreCase)) { - syncApprovals(user); - } - input.add(user); - } - if (!StringUtils.hasLength(attributesCommaSeparated)) { // Return all user data return new SearchResults<>(Arrays.asList(ScimCore.SCHEMAS), input, startIndex, count, result.size()); From b4d9050aef37369c709adcd9a95e1fef8ff25fff Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 13:57:20 +0100 Subject: [PATCH 11/42] Fix Sonar finding: use constant instead of duplicating String literal --- .../endpoints/UserIdConversionEndpoints.java | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java index 5834fc5ac44..531dc10c17c 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java @@ -40,6 +40,10 @@ @Controller public class UserIdConversionEndpoints implements InitializingBean { + private static final String FIELD_USERNAME = "userName"; + private static final String FIELD_ID = "id"; + private static final String FIELD_ORIGIN = "origin"; + private final Logger logger = LoggerFactory.getLogger(getClass()); private final ScimUserProvisioning scimUserProvisioning; @@ -90,7 +94,7 @@ public ResponseEntity findUsers( final List filteredUsers = scimUserProvisioning.retrieveByScimFilter( filter, includeInactive, - "userName", + FIELD_USERNAME, sortOrder.equalsIgnoreCase("ascending"), IdentityZoneHolder.getCurrentZoneId() ); @@ -99,9 +103,9 @@ public ResponseEntity findUsers( // map to result structure final List> result = usersCurrentPage.stream() .map(scimUser -> Map.of( - "id", scimUser.getId(), - "userName", scimUser.getUserName(), - "origin", scimUser.getOrigin() + FIELD_ID, scimUser.getId(), + FIELD_USERNAME, scimUser.getUserName(), + FIELD_ORIGIN, scimUser.getOrigin() )) .collect(toList()); @@ -111,7 +115,7 @@ public ResponseEntity findUsers( startIndex, count, filteredUsers.size(), - new String[]{"id", "userName", "origin"}, + new String[]{FIELD_ID, FIELD_USERNAME, FIELD_ORIGIN}, Arrays.asList(ScimCore.SCHEMAS) ), HttpStatus.OK @@ -156,8 +160,8 @@ private boolean containsIdOrUserNameClause(SCIMFilter filter) { return containsIdOrUserNameClause(filter.getFilterComponents().get(1)) || resultLeftOperand; case EQUALITY: String name = filter.getFilterAttribute().getAttributeName(); - if ("id".equalsIgnoreCase(name) || - "userName".equalsIgnoreCase(name)) { + if (FIELD_ID.equalsIgnoreCase(name) || + FIELD_USERNAME.equalsIgnoreCase(name)) { return true; } else if (OriginKeys.ORIGIN.equalsIgnoreCase(name)) { return false; From cea196348bbba20e535967e57a5d00de5079d265 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 13:58:56 +0100 Subject: [PATCH 12/42] Fix Sonar finding: replace .collect(toList()) with .toList() --- .../uaa/scim/endpoints/UserIdConversionEndpoints.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java index 531dc10c17c..6480008607b 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java @@ -1,7 +1,5 @@ package org.cloudfoundry.identity.uaa.scim.endpoints; -import static java.util.stream.Collectors.toList; - import javax.servlet.http.HttpServletRequest; import java.util.Arrays; @@ -107,7 +105,7 @@ public ResponseEntity findUsers( FIELD_USERNAME, scimUser.getUserName(), FIELD_ORIGIN, scimUser.getOrigin() )) - .collect(toList()); + .toList(); return new ResponseEntity<>( SearchResultsFactory.buildSearchResultFrom( From b8a8b7fe602a82b19239d66b51def426455ff7a8 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 14:03:42 +0100 Subject: [PATCH 13/42] Fix Sonar finding: replace IdentityZoneHolder usage --- .../uaa/scim/endpoints/UserIdConversionEndpoints.java | 7 +++++-- .../uaa/scim/endpoints/UserIdConversionEndpointsTests.java | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java index 6480008607b..4f6b098c7be 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java @@ -15,7 +15,7 @@ import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor; import org.cloudfoundry.identity.uaa.util.UaaPagingUtils; import org.cloudfoundry.identity.uaa.util.UaaStringUtils; -import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder; +import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.InitializingBean; @@ -47,17 +47,20 @@ public class UserIdConversionEndpoints implements InitializingBean { private final ScimUserProvisioning scimUserProvisioning; private final SecurityContextAccessor securityContextAccessor; private final ScimUserEndpoints scimUserEndpoints; + private final IdentityZoneManager identityZoneManager; private final boolean enabled; public UserIdConversionEndpoints( final SecurityContextAccessor securityContextAccessor, final ScimUserEndpoints scimUserEndpoints, final @Qualifier("scimUserProvisioning") ScimUserProvisioning scimUserProvisioning, + final IdentityZoneManager identityZoneManager, final @Value("${scim.userids_enabled:true}") boolean enabled ) { this.securityContextAccessor = securityContextAccessor; this.scimUserEndpoints = scimUserEndpoints; this.scimUserProvisioning = scimUserProvisioning; + this.identityZoneManager = identityZoneManager; this.enabled = enabled; } @@ -94,7 +97,7 @@ public ResponseEntity findUsers( includeInactive, FIELD_USERNAME, sortOrder.equalsIgnoreCase("ascending"), - IdentityZoneHolder.getCurrentZoneId() + identityZoneManager.getCurrentIdentityZoneId() ); final List usersCurrentPage = UaaPagingUtils.subList(filteredUsers, startIndex, count); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java index 4397f1d1639..e532b1a418c 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java @@ -38,6 +38,7 @@ import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor; import org.cloudfoundry.identity.uaa.zone.IdentityZone; import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder; +import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -67,6 +68,7 @@ public class UserIdConversionEndpointsTests { private final ScimUserProvisioning scimUserProvisioning = mock(ScimUserProvisioning.class); private final MockedStatic idzHolderMockedStatic; + private final IdentityZoneManager identityZoneManager = mock(IdentityZoneManager.class); @SuppressWarnings("rawtypes") private final Collection authorities = AuthorityUtils @@ -80,7 +82,7 @@ public UserIdConversionEndpointsTests() { @Before public void init() { mockSecurityContextAccessor = mock(SecurityContextAccessor.class); - endpoints = new UserIdConversionEndpoints(mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, true); + endpoints = new UserIdConversionEndpoints(mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, identityZoneManager, true); when(mockSecurityContextAccessor.getAuthorities()).thenReturn(authorities); when(mockSecurityContextAccessor.getAuthenticationInfo()).thenReturn("mock object"); when(scimUserEndpoints.getUserMaxCount()).thenReturn(10_000); @@ -300,7 +302,7 @@ public void testBadFilter11() { @Test public void testDisabled() { arrangeCurrentIdentityZone("uaa"); - endpoints = new UserIdConversionEndpoints(mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, false); + endpoints = new UserIdConversionEndpoints(mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, identityZoneManager, false); ResponseEntity response = endpoints.findUsers("id eq \"foo\"", "ascending", 0, 100, false); Assert.assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode()); Assert.assertEquals("Illegal Operation: Endpoint not enabled.", response.getBody()); @@ -318,6 +320,7 @@ private void arrangeCurrentIdentityZone(final String idzId) { identityZone.setId(idzId); idzHolderMockedStatic.when(IdentityZoneHolder::get).thenReturn(identityZone); idzHolderMockedStatic.when(IdentityZoneHolder::getCurrentZoneId).thenReturn(idzId); + when(identityZoneManager.getCurrentIdentityZoneId()).thenReturn(idzId); } private void arrangeScimUsersForFilter( From 28c59d19c3ee1b29016d3b1f1c5d622a93d90c5e Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 14:06:06 +0100 Subject: [PATCH 14/42] Fix Sonar finding: introduce parameterized test --- .../UserIdConversionEndpointsTests.java | 36 +++++-------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java index e532b1a418c..e9d344b3711 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java @@ -170,48 +170,28 @@ public void testGoodFilter_OnlyActive() { @Test public void testGoodFilter1() { - arrangeCurrentIdentityZone("uaa"); - final ResponseEntity response = endpoints.findUsers( - "(id eq \"foo\" or username eq \"bar\") and origin eq \"uaa\"", - "ascending", - 0, - 100, - false - ); - assertEquals(HttpStatus.OK, response.getStatusCode()); + testGoodFilter("(id eq \"foo\" or username eq \"bar\") and origin eq \"uaa\""); } @Test public void testGoodFilter2() { - arrangeCurrentIdentityZone("uaa"); - final ResponseEntity response = endpoints.findUsers( - "origin eq \"uaa\" and (id eq \"foo\" or username eq \"bar\")", - "ascending", - 0, - 100, - false - ); - assertEquals(HttpStatus.OK, response.getStatusCode()); + testGoodFilter("origin eq \"uaa\" and (id eq \"foo\" or username eq \"bar\")"); } @Test public void testGoodFilter3() { - arrangeCurrentIdentityZone("uaa"); - final ResponseEntity response = endpoints.findUsers( - "(id eq \"foo\" and username eq \"bar\") or id eq \"bar\"", - "ascending", - 0, - 100, - false - ); - assertEquals(HttpStatus.OK, response.getStatusCode()); + testGoodFilter("(id eq \"foo\" and username eq \"bar\") or id eq \"bar\""); } @Test public void testGoodFilter4() { + testGoodFilter("id eq \"bar\" and (id eq \"foo\" and username eq \"bar\")"); + } + + private void testGoodFilter(final String filter) { arrangeCurrentIdentityZone("uaa"); final ResponseEntity response = endpoints.findUsers( - "id eq \"bar\" and (id eq \"foo\" and username eq \"bar\")", + filter, "ascending", 0, 100, From 3df2648146359bdfbd14088c89c8ccfe79c37197 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 14:31:04 +0100 Subject: [PATCH 15/42] Add additional unit tests for JdbcScimUserProvisioning#retrieveByScimFilter --- .../jdbc/JdbcScimUserProvisioningTests.java | 70 ++++++++++++++++--- 1 file changed, 62 insertions(+), 8 deletions(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java index b3383da1dd3..dabbae415c1 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java @@ -235,21 +235,21 @@ void canDeleteProviderUsersInDefaultZone() { } @Test - void testRetrieveByScimFilter() { - final String origin1 = randomString(); - addIdentityProvider(jdbcTemplate, currentIdentityZoneId, origin1); + void retrieveByScimFilter_OnlyActive() { + final String originActive = randomString(); + addIdentityProvider(jdbcTemplate, currentIdentityZoneId, originActive, true); - final String origin2 = randomString(); - addIdentityProvider(jdbcTemplate, currentIdentityZoneId, origin2); + final String originInactive = randomString(); + addIdentityProvider(jdbcTemplate, currentIdentityZoneId, originInactive, false); final ScimUser user1 = new ScimUser(null, "jo@foo.com", "Jo", "User"); user1.addEmail("jo@blah.com"); - user1.setOrigin(origin1); + user1.setOrigin(originActive); final ScimUser created1 = jdbcScimUserProvisioning.createUser(user1, "j7hyqpassX", currentIdentityZoneId); final ScimUser user2 = new ScimUser(null, "jo2@foo.com", "Jo", "User"); user2.addEmail("jo2@blah.com"); - user2.setOrigin(origin2); + user2.setOrigin(originInactive); final ScimUser created2 = jdbcScimUserProvisioning.createUser(user2, "j7hyqpassX", currentIdentityZoneId); final Function> retrieveByScimFilter = (scimFilter) -> { @@ -266,6 +266,56 @@ void testRetrieveByScimFilter() { return usernames; }; + // case 1: should return only user 1 + String filter = String.format("id eq '%s' or origin eq '%s'", created1.getId(), created2.getOrigin()); + List usernames = retrieveByScimFilter.apply(filter); + Assertions.assertThat(usernames) + .hasSize(1) + .contains(created1.getUserName()); + + // case 2: should return empty list + filter = String.format("origin eq '%s'", created2.getOrigin()); + usernames = retrieveByScimFilter.apply(filter); + Assertions.assertThat(usernames).isEmpty(); + + // case 3: should return empty list (filtered by origin and ID) + filter = String.format("origin eq '%s' and id eq '%s'", created2.getOrigin(), created2.getId()); + usernames = retrieveByScimFilter.apply(filter); + Assertions.assertThat(usernames).isEmpty(); + } + + @Test + void retrieveByScimFilter_IncludeInactive() { + final String originActive = randomString(); + addIdentityProvider(jdbcTemplate, currentIdentityZoneId, originActive, true); + + final String originInactive = randomString(); + addIdentityProvider(jdbcTemplate, currentIdentityZoneId, originInactive, false); + + final ScimUser user1 = new ScimUser(null, "jo@foo.com", "Jo", "User"); + user1.addEmail("jo@blah.com"); + user1.setOrigin(originActive); + final ScimUser created1 = jdbcScimUserProvisioning.createUser(user1, "j7hyqpassX", currentIdentityZoneId); + + final ScimUser user2 = new ScimUser(null, "jo2@foo.com", "Jo", "User"); + user2.addEmail("jo2@blah.com"); + user2.setOrigin(originInactive); + final ScimUser created2 = jdbcScimUserProvisioning.createUser(user2, "j7hyqpassX", currentIdentityZoneId); + + final Function> retrieveByScimFilter = (scimFilter) -> { + final List result = jdbcScimUserProvisioning.retrieveByScimFilter( + scimFilter, + true, + "userName", + true, + currentIdentityZoneId + ); + Assertions.assertThat(result).isNotNull(); + final List usernames = result.stream().map(ScimUser::getUserName).collect(toList()); + Assertions.assertThat(usernames).isSorted(); + return usernames; + }; + // case 1: should return both String filter = String.format("id eq '%s' or origin eq '%s'", created1.getId(), created2.getOrigin()); List usernames = retrieveByScimFilter.apply(filter); @@ -1269,7 +1319,11 @@ private static void addMembership(final JdbcTemplate jdbcTemplate, } private static void addIdentityProvider(JdbcTemplate jdbcTemplate, String idzId, String originKey) { - jdbcTemplate.update("insert into identity_provider (id,identity_zone_id,name,origin_key,type) values (?,?,?,?,'UNKNOWN')", UUID.randomUUID().toString(), idzId, originKey, originKey); + addIdentityProvider(jdbcTemplate, idzId, originKey, true); + } + + private static void addIdentityProvider(JdbcTemplate jdbcTemplate, String idzId, String originKey, boolean active) { + jdbcTemplate.update("insert into identity_provider (id,identity_zone_id,name,origin_key,type,active) values (?,?,?,?,'UNKNOWN',?)", UUID.randomUUID().toString(), idzId, originKey, originKey, active); } private String randomString() { From 3d0a7e9e3d8d73f777654a347bdc9c34fe2287a6 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Mon, 12 Feb 2024 15:11:16 +0100 Subject: [PATCH 16/42] Refactor --- .../identity/uaa/scim/endpoints/UserIdConversionEndpoints.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java index 4f6b098c7be..f5b91bcd526 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java @@ -6,7 +6,6 @@ import java.util.List; import java.util.Map; -import org.cloudfoundry.identity.uaa.constants.OriginKeys; import org.cloudfoundry.identity.uaa.resources.SearchResultsFactory; import org.cloudfoundry.identity.uaa.scim.ScimCore; import org.cloudfoundry.identity.uaa.scim.ScimUser; @@ -164,7 +163,7 @@ private boolean containsIdOrUserNameClause(SCIMFilter filter) { if (FIELD_ID.equalsIgnoreCase(name) || FIELD_USERNAME.equalsIgnoreCase(name)) { return true; - } else if (OriginKeys.ORIGIN.equalsIgnoreCase(name)) { + } else if (FIELD_ORIGIN.equalsIgnoreCase(name)) { return false; } else { throw new ScimException("Invalid filter attribute.", HttpStatus.BAD_REQUEST); From 9d70584867d6c9fed3efd1ae1ee3cfadcad5e876 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Tue, 13 Feb 2024 10:24:49 +0100 Subject: [PATCH 17/42] Fix table alias in WHERE clause translated from SCIM filter --- .../scim/jdbc/JdbcScimUserProvisioning.java | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java index 7bcd8623aa5..dd5dd1ae045 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java @@ -31,9 +31,11 @@ import java.util.Map; import java.util.UUID; import java.util.regex.Pattern; +import java.util.stream.Stream; import org.cloudfoundry.identity.uaa.audit.event.SystemDeletable; import org.cloudfoundry.identity.uaa.constants.OriginKeys; +import org.cloudfoundry.identity.uaa.resources.AttributeNameMapper; import org.cloudfoundry.identity.uaa.resources.ResourceMonitor; import org.cloudfoundry.identity.uaa.resources.jdbc.AbstractQueryable; import org.cloudfoundry.identity.uaa.resources.jdbc.JdbcPagingListFactory; @@ -196,6 +198,32 @@ public List retrieveByScimFilter( final SimpleSearchQueryConverter queryConverter = new SimpleSearchQueryConverter(); validateOrderBy(queryConverter.map(sortBy)); + /* since the two tables used in the query ('users' and 'identity_provider') have columns with identical names, + * we must ensure that the columns of 'users' are used in the WHERE clause generated for the SCIM filter */ + final AttributeNameMapper attributeNameMapper = new AttributeNameMapper() { + @Override + public String mapToInternal(final String attr) { + // in the later query, 'users' will have the alias 'u' + return "u." + attr; + } + + @Override + public String[] mapToInternal(final String[] attr) { + return Stream.of(attr).map(this::mapToInternal).toArray(String[]::new); + } + + @Override + public String mapFromInternal(final String attr) { + return attr.substring(2); + } + + @Override + public String[] mapFromInternal(final String[] attr) { + return Stream.of(attr).map(this::mapFromInternal).toArray(String[]::new); + } + }; + queryConverter.setAttributeNameMapper(attributeNameMapper); + // build WHERE clause final ProcessedFilter where = queryConverter.convert(filter, sortBy, ascending, zoneId); final String whereClauseScimFilter = where.getSql(); From 9ab311eb2d9d31922d36b7aa3d018d2f70ee2379 Mon Sep 17 00:00:00 2001 From: Florian Tack Date: Mon, 4 Mar 2024 14:00:39 +0100 Subject: [PATCH 18/42] Revert "Revert "Merge pull request #2718 from cloudfoundry/feat/healthzDatabaseCheck"" This reverts commit aa8349f165af0298cc666a6ad6cf9c5c62cb2001. --- .../identity/uaa/health/HealthzEndpoint.java | 39 ++++++++++++-- .../{web => health}/HealthzEndpointTests.java | 53 ++++++++++++++----- .../HealthzEndpointIntegrationTests.java | 1 + .../uaa/integration/feature/HealthzIT.java | 2 +- ...althzShouldNotBeProtectedMockMvcTests.java | 4 +- 5 files changed, 79 insertions(+), 20 deletions(-) rename server/src/test/java/org/cloudfoundry/identity/uaa/{web => health}/HealthzEndpointTests.java (63%) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/health/HealthzEndpoint.java b/server/src/main/java/org/cloudfoundry/identity/uaa/health/HealthzEndpoint.java index dba799a01f8..5765a9f71d7 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/health/HealthzEndpoint.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/health/HealthzEndpoint.java @@ -1,14 +1,19 @@ package org.cloudfoundry.identity.uaa.health; +import javax.servlet.http.HttpServletResponse; +import javax.sql.DataSource; + +import java.sql.Connection; +import java.sql.Statement; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Value; +import org.springframework.scheduling.annotation.Scheduled; import org.springframework.stereotype.Controller; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.ResponseBody; -import javax.servlet.http.HttpServletResponse; - /** * Simple controller that just returns "ok" in a request body for the purposes * of monitoring health of the application. It also registers a shutdown hook @@ -18,10 +23,13 @@ public class HealthzEndpoint { private static Logger logger = LoggerFactory.getLogger(HealthzEndpoint.class); private volatile boolean stopping = false; + private volatile Boolean wasLastConnectionSuccessful = null; + private DataSource dataSource; public HealthzEndpoint( @Value("${uaa.shutdown.sleep:10000}") final long sleepTime, - final Runtime runtime) { + final Runtime runtime, + final DataSource dataSource) { Thread shutdownHook = new Thread(() -> { stopping = true; logger.warn("Shutdown hook received, future requests to this endpoint will return 503"); @@ -35,6 +43,7 @@ public HealthzEndpoint( } }); runtime.addShutdownHook(shutdownHook); + this.dataSource = dataSource; } @GetMapping("/healthz") @@ -45,8 +54,28 @@ public String getHealthz(HttpServletResponse response) { response.setStatus(503); return "stopping\n"; } else { - return "ok\n"; + if (wasLastConnectionSuccessful == null) { + return "UAA running. Database status unknown.\n"; + } + + if (wasLastConnectionSuccessful) { + return "ok. Database connection successful.\n"; + } else { + response.setStatus(503); + return "Database Connection failed.\n"; + } } } -} \ No newline at end of file + @Scheduled(fixedRateString = "${uaa.health.db.rate:10000}") + void isDataSourceConnectionAvailable() { + try (Connection c = dataSource.getConnection(); Statement statement = c.createStatement()) { + statement.execute("SELECT 1 from identity_zone;"); //"SELECT 1;" Not supported by HSQLDB + wasLastConnectionSuccessful = true; + return; + } catch (Exception ex) { + logger.error("Could not establish connection to DB - " + ex.getMessage()); + } + wasLastConnectionSuccessful = false; + } +} diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/web/HealthzEndpointTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/health/HealthzEndpointTests.java similarity index 63% rename from server/src/test/java/org/cloudfoundry/identity/uaa/web/HealthzEndpointTests.java rename to server/src/test/java/org/cloudfoundry/identity/uaa/health/HealthzEndpointTests.java index 4baeb895211..033f2cf2797 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/web/HealthzEndpointTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/health/HealthzEndpointTests.java @@ -1,18 +1,25 @@ -package org.cloudfoundry.identity.uaa.web; - -import org.cloudfoundry.identity.uaa.health.HealthzEndpoint; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Nested; -import org.junit.jupiter.api.Test; -import org.mockito.ArgumentCaptor; -import org.springframework.mock.web.MockHttpServletResponse; +package org.cloudfoundry.identity.uaa.health; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import javax.sql.DataSource; + +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.springframework.mock.web.MockHttpServletResponse; class HealthzEndpointTests { @@ -21,11 +28,19 @@ class HealthzEndpointTests { private HealthzEndpoint endpoint; private MockHttpServletResponse response; private Thread shutdownHook; + private DataSource dataSource; + private Connection connection; + private Statement statement; @BeforeEach - void setUp() { + void setUp() throws SQLException { Runtime mockRuntime = mock(Runtime.class); - endpoint = new HealthzEndpoint(SLEEP_UPON_SHUTDOWN, mockRuntime); + dataSource = mock(DataSource.class); + connection = mock(Connection.class); + statement = mock(Statement.class); + when(dataSource.getConnection()).thenReturn(connection); + when(connection.createStatement()).thenReturn(statement); + endpoint = new HealthzEndpoint(SLEEP_UPON_SHUTDOWN, mockRuntime, dataSource); response = new MockHttpServletResponse(); ArgumentCaptor threadArgumentCaptor = ArgumentCaptor.forClass(Thread.class); @@ -35,7 +50,20 @@ void setUp() { @Test void getHealthz() { - assertEquals("ok\n", endpoint.getHealthz(response)); + assertEquals("UAA running. Database status unknown.\n", endpoint.getHealthz(response)); + } + + @Test + void getHealthz_connectionSuccess() { + endpoint.isDataSourceConnectionAvailable(); + assertEquals("ok. Database connection successful.\n", endpoint.getHealthz(response)); + } + @Test + void getHealthz_connectionFailed() throws SQLException { + when(statement.execute(anyString())).thenThrow(new SQLException()); + endpoint.isDataSourceConnectionAvailable(); + assertEquals("Database Connection failed.\n", endpoint.getHealthz(response)); + assertEquals(503, response.getStatus()); } @Test @@ -54,7 +82,8 @@ class WithoutSleeping { @BeforeEach void setUp() { Runtime mockRuntime = mock(Runtime.class); - endpoint = new HealthzEndpoint(-1, mockRuntime); + DataSource dataSource = mock(DataSource.class); + endpoint = new HealthzEndpoint(-1, mockRuntime, dataSource); response = new MockHttpServletResponse(); ArgumentCaptor threadArgumentCaptor = ArgumentCaptor.forClass(Thread.class); diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/HealthzEndpointIntegrationTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/HealthzEndpointIntegrationTests.java index 86c55dfe07e..425e597b4b8 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/HealthzEndpointIntegrationTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/HealthzEndpointIntegrationTests.java @@ -42,6 +42,7 @@ public void testHappyDay() { String body = response.getBody(); assertTrue(body.contains("ok")); + assertTrue(body.contains("Database connection successful")); } diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/HealthzIT.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/HealthzIT.java index e90a3231618..afb37fb713a 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/HealthzIT.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/HealthzIT.java @@ -53,6 +53,6 @@ public void logout_and_clear_cookies() { @Test public void testHealthz() { webDriver.get(baseUrl + "/healthz"); - Assert.assertEquals("ok", webDriver.findElement(By.tagName("body")).getText()); + Assert.assertEquals("ok. Database connection successful.", webDriver.findElement(By.tagName("body")).getText()); } } diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/config/HealthzShouldNotBeProtectedMockMvcTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/config/HealthzShouldNotBeProtectedMockMvcTests.java index c1c80c12c61..89bedc2febb 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/config/HealthzShouldNotBeProtectedMockMvcTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/config/HealthzShouldNotBeProtectedMockMvcTests.java @@ -90,7 +90,7 @@ void redirectedRequestsGoToTheConfiguredPort() throws Exception { void healthzIsNotRejected(MockHttpServletRequestBuilder getRequest) throws Exception { mockMvc.perform(getRequest) .andExpect(status().isOk()) - .andExpect(content().string("ok\n")); + .andExpect(content().string("ok. Database connection successful.\n")); } @Test @@ -128,7 +128,7 @@ void setUp() { void healthzIsNotRejected(MockHttpServletRequestBuilder getRequest) throws Exception { mockMvc.perform(getRequest) .andExpect(status().isOk()) - .andExpect(content().string("ok\n")); + .andExpect(content().string("ok. Database connection successful.\n")); } @Test From 0a56dc277261616a1333ff90b20643916a024c34 Mon Sep 17 00:00:00 2001 From: Florian Tack Date: Mon, 4 Mar 2024 14:03:58 +0100 Subject: [PATCH 19/42] adjust ok status message, so it is backwards compatible --- .../org/cloudfoundry/identity/uaa/health/HealthzEndpoint.java | 2 +- .../identity/uaa/health/HealthzEndpointTests.java | 2 +- .../uaa/integration/HealthzEndpointIntegrationTests.java | 2 -- .../identity/uaa/integration/feature/HealthzIT.java | 2 +- .../mock/config/HealthzShouldNotBeProtectedMockMvcTests.java | 4 ++-- 5 files changed, 5 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/health/HealthzEndpoint.java b/server/src/main/java/org/cloudfoundry/identity/uaa/health/HealthzEndpoint.java index 5765a9f71d7..99bc032a018 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/health/HealthzEndpoint.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/health/HealthzEndpoint.java @@ -59,7 +59,7 @@ public String getHealthz(HttpServletResponse response) { } if (wasLastConnectionSuccessful) { - return "ok. Database connection successful.\n"; + return "ok\n"; } else { response.setStatus(503); return "Database Connection failed.\n"; diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/health/HealthzEndpointTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/health/HealthzEndpointTests.java index 033f2cf2797..153cac19bc0 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/health/HealthzEndpointTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/health/HealthzEndpointTests.java @@ -56,7 +56,7 @@ void getHealthz() { @Test void getHealthz_connectionSuccess() { endpoint.isDataSourceConnectionAvailable(); - assertEquals("ok. Database connection successful.\n", endpoint.getHealthz(response)); + assertEquals("ok\n", endpoint.getHealthz(response)); } @Test void getHealthz_connectionFailed() throws SQLException { diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/HealthzEndpointIntegrationTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/HealthzEndpointIntegrationTests.java index 425e597b4b8..d339b066869 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/HealthzEndpointIntegrationTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/HealthzEndpointIntegrationTests.java @@ -42,8 +42,6 @@ public void testHappyDay() { String body = response.getBody(); assertTrue(body.contains("ok")); - assertTrue(body.contains("Database connection successful")); - } } diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/HealthzIT.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/HealthzIT.java index afb37fb713a..e90a3231618 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/HealthzIT.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/HealthzIT.java @@ -53,6 +53,6 @@ public void logout_and_clear_cookies() { @Test public void testHealthz() { webDriver.get(baseUrl + "/healthz"); - Assert.assertEquals("ok. Database connection successful.", webDriver.findElement(By.tagName("body")).getText()); + Assert.assertEquals("ok", webDriver.findElement(By.tagName("body")).getText()); } } diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/config/HealthzShouldNotBeProtectedMockMvcTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/config/HealthzShouldNotBeProtectedMockMvcTests.java index 89bedc2febb..c1c80c12c61 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/config/HealthzShouldNotBeProtectedMockMvcTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/config/HealthzShouldNotBeProtectedMockMvcTests.java @@ -90,7 +90,7 @@ void redirectedRequestsGoToTheConfiguredPort() throws Exception { void healthzIsNotRejected(MockHttpServletRequestBuilder getRequest) throws Exception { mockMvc.perform(getRequest) .andExpect(status().isOk()) - .andExpect(content().string("ok. Database connection successful.\n")); + .andExpect(content().string("ok\n")); } @Test @@ -128,7 +128,7 @@ void setUp() { void healthzIsNotRejected(MockHttpServletRequestBuilder getRequest) throws Exception { mockMvc.perform(getRequest) .andExpect(status().isOk()) - .andExpect(content().string("ok. Database connection successful.\n")); + .andExpect(content().string("ok\n")); } @Test From c5910f02e6e49be67ded2cc1f632e6d01c7a8994 Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 11:45:17 +0100 Subject: [PATCH 20/42] add methods for sanitized logging --- .../uaa/logging/SanitizedLogFactory.java | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 81956f292a0..71564682b1f 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -1,4 +1,4 @@ -package org.cloudfoundry.identity.uaa.logging; +package org.coundfoundry.identity.uaa.logging; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -16,6 +16,10 @@ public static class SanitizedLog { private Logger fallback; public SanitizedLog(Logger logger) { + setFallback(logger); + } + + public void setFallback(Logger logger) { this.fallback = logger; } @@ -24,19 +28,39 @@ public boolean isDebugEnabled() { } public void info(String message) { - fallback.info(LogSanitizerUtil.sanitize(message)); + fallback.info(sanitizeLog(message)); + } + + public void info(String message, Throwable t) { + fallback.info(sanitizeLog(message), t); } public void warn(String message) { - fallback.warn(LogSanitizerUtil.sanitize(message)); + fallback.warn(sanitizeLog(message)); + } + + public void warn(String message, Throwable t) { + fallback.warn(sanitizeLog(message), t); } public void debug(String message) { - fallback.debug(LogSanitizerUtil.sanitize(message)); + fallback.debug(sanitizeLog(message)); } public void debug(String message, Throwable t) { - fallback.debug(LogSanitizerUtil.sanitize(message), t); + fallback.debug(sanitizeLog(message), t); + } + + public void error(String message) { + fallback.error(sanitizeLog(message)); + } + + public void error(String message, Throwable t) { + fallback.error(sanitizeLog(message), t); + } + + public static String sanitizeLog(String message) { + return LogSanitizerUtil.sanitize(message); } } -} \ No newline at end of file +} From bd1fad291870a7da5997514fad65061c1a42313e Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 11:54:17 +0100 Subject: [PATCH 21/42] add trace methods --- .../identity/uaa/logging/SanitizedLogFactory.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 71564682b1f..cf22af29729 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -1,9 +1,9 @@ -package org.coundfoundry.identity.uaa.logging; +package org.cloudfoundry.identity.uaa.logging; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** +/** TODO remove fork * Returns Log instance that replaces \n, \r, \t with a | to prevent log forging. */ public class SanitizedLogFactory { @@ -59,7 +59,15 @@ public void error(String message, Throwable t) { fallback.error(sanitizeLog(message), t); } - public static String sanitizeLog(String message) { + public void trace(String message) { + fallback.trace(sanitizeLog(message)); + } + + public void trace(String message, Throwable t) { + fallback.trace(sanitizeLog(message), t); + } + + protected static String sanitizeLog(String message) { return LogSanitizerUtil.sanitize(message); } } From 7b45858474f2233ec9e30a92758f8774148498cd Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 11:55:11 +0100 Subject: [PATCH 22/42] remove comment --- .../cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index cf22af29729..ec7eaf26880 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -3,7 +3,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** TODO remove fork +/** * Returns Log instance that replaces \n, \r, \t with a | to prevent log forging. */ public class SanitizedLogFactory { From e6d82932945698e9da33960f54c0c910261b64ce Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 12:30:29 +0100 Subject: [PATCH 23/42] additional test methods --- .../uaa/logging/SanitizedLogFactoryTest.java | 96 +++++++++++++++---- 1 file changed, 77 insertions(+), 19 deletions(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java index 60bf5e795be..3f49ccf209e 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java @@ -1,61 +1,119 @@ package org.cloudfoundry.identity.uaa.logging; import org.slf4j.Logger; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class SanitizedLogFactoryTest { + private final String dirtyMessage = "one\ntwo\tthree\rfour"; + private final String sanitizedMsg = "one|two|three|four[SANITIZED]"; + private final String cleanMessage = "one two three four"; + Logger mockLog; + SanitizedLogFactory.SanitizedLog log; + Exception ex; @Before public void setUp() { mockLog = mock(Logger.class); + when(mockLog.isDebugEnabled()).thenReturn(true); + log = new SanitizedLogFactory.SanitizedLog(mockLog); + ex = new Exception(); } @Test public void testSanitizeDebug() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.debug("one\ntwo\tthree\rfour"); - verify(mockLog).debug("one|two|three|four[SANITIZED]"); + Assert.assertTrue(log.isDebugEnabled()); + log.debug(dirtyMessage); + verify(mockLog).debug(sanitizedMsg); + log.debug(dirtyMessage, ex); + verify(mockLog).debug(sanitizedMsg, ex); } @Test public void testSanitizeDebugCleanMessage() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.debug("one two three four"); - verify(mockLog).debug("one two three four"); + Assert.assertTrue(log.isDebugEnabled()); + log.debug(cleanMessage); + verify(mockLog).debug(cleanMessage); + log.debug(cleanMessage, ex); + verify(mockLog).debug(cleanMessage, ex); } @Test public void testSanitizeInfo() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.info("one\ntwo\tthree\rfour"); - verify(mockLog).info("one|two|three|four[SANITIZED]"); + log.info(dirtyMessage); + verify(mockLog).info(sanitizedMsg); + log.info(dirtyMessage, ex); + verify(mockLog).info(sanitizedMsg, ex); } @Test public void testSanitizeInfoCleanMessage() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.info("one two three four"); - verify(mockLog).info("one two three four"); + log.info(cleanMessage); + verify(mockLog).info(cleanMessage); + log.info(cleanMessage, ex); + verify(mockLog).info(cleanMessage, ex); } @Test public void testSanitizeWarn() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.warn("one\ntwo\tthree\rfour"); - verify(mockLog).warn("one|two|three|four[SANITIZED]"); + log.warn(dirtyMessage); + verify(mockLog).warn(sanitizedMsg); + log.warn(dirtyMessage, ex); + verify(mockLog).warn(sanitizedMsg, ex); } @Test public void testSanitizeWarnCleanMessage() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.warn("one two three four"); - verify(mockLog).warn("one two three four"); + log.warn(cleanMessage); + verify(mockLog).warn(cleanMessage); + log.warn(cleanMessage, ex); + verify(mockLog).warn(cleanMessage, ex); + } + + @Test + public void testSanitizeError() { + log.error(dirtyMessage); + verify(mockLog).error(sanitizedMsg); + log.error(dirtyMessage, ex); + verify(mockLog).error(sanitizedMsg, ex); } -} \ No newline at end of file + @Test + public void testSanitizeErrorCleanMessage() { + log.error(cleanMessage); + verify(mockLog).error(cleanMessage); + log.error(cleanMessage, ex); + verify(mockLog).error(cleanMessage, ex); + } + + @Test + public void testSanitizeTrace() { + log.trace(dirtyMessage); + verify(mockLog).trace(sanitizedMsg); + log.trace(dirtyMessage, ex); + verify(mockLog).trace(sanitizedMsg, ex); + } + + @Test + public void testSanitizeTraceCleanMessage() { + log.trace(cleanMessage); + verify(mockLog).trace(cleanMessage); + log.trace(cleanMessage, ex); + verify(mockLog).trace(cleanMessage, ex); + } + + @Test + public void testSanitizeLog() { + String logMsg = SanitizedLogFactory.SanitizedLog.sanitizeLog(dirtyMessage); + Assert.assertEquals(sanitizedMsg, logMsg); + logMsg = SanitizedLogFactory.SanitizedLog.sanitizeLog(cleanMessage); + Assert.assertEquals(cleanMessage, logMsg); + } +} From fe5f11480323f6412757dc80a9e98482933ea47c Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 12:32:56 +0100 Subject: [PATCH 24/42] remove method --- .../identity/uaa/logging/SanitizedLogFactory.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index ec7eaf26880..5566a4be5c6 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -16,10 +16,6 @@ public static class SanitizedLog { private Logger fallback; public SanitizedLog(Logger logger) { - setFallback(logger); - } - - public void setFallback(Logger logger) { this.fallback = logger; } From da19b79321e7c93bf193dc646c2c55ed6b4aefda Mon Sep 17 00:00:00 2001 From: D023954 Date: Tue, 5 Mar 2024 15:58:07 +0100 Subject: [PATCH 25/42] use random exception text --- .vscode/settings.json | 3 +++ .../identity/uaa/logging/SanitizedLogFactoryTest.java | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 00000000000..849f79e6b5e --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,3 @@ +{ + "java.compile.nullAnalysis.mode": "automatic" +} diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java index 3f49ccf209e..d4b316fdd39 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java @@ -9,6 +9,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import org.apache.commons.lang3.RandomStringUtils; + public class SanitizedLogFactoryTest { private final String dirtyMessage = "one\ntwo\tthree\rfour"; @@ -24,7 +26,7 @@ public void setUp() { mockLog = mock(Logger.class); when(mockLog.isDebugEnabled()).thenReturn(true); log = new SanitizedLogFactory.SanitizedLog(mockLog); - ex = new Exception(); + ex = new Exception(RandomStringUtils.randomAlphanumeric(8)); } @Test From 2b9b3e959170062683d3b98565014e33e41435bf Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 17:50:42 +0100 Subject: [PATCH 26/42] Delete .vscode/settings.json --- .vscode/settings.json | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json deleted file mode 100644 index 849f79e6b5e..00000000000 --- a/.vscode/settings.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "java.compile.nullAnalysis.mode": "automatic" -} From f214bf0d546cd74eff5596510d6e06db9855a757 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Tue, 27 Feb 2024 16:22:12 -0800 Subject: [PATCH 27/42] doc: clarify token revocation [#177045463] Co-authored-by: Peter Chen --- .../source/index.html.md.erb | 23 +++++++++++++++++++ .../identity/uaa/login/TokenEndpointDocs.java | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/uaa/slateCustomizations/source/index.html.md.erb b/uaa/slateCustomizations/source/index.html.md.erb index 2a0d77a5a47..d5dc440163b 100644 --- a/uaa/slateCustomizations/source/index.html.md.erb +++ b/uaa/slateCustomizations/source/index.html.md.erb @@ -378,6 +378,29 @@ _Response Fields_ Added in UAA 3.3.0 +When an access token is revoked using this endpoint, the UAA Introspect Token endpoint (``/introspect``) +will respond with ``"active": false`` when presented with the revoked access token. + +If the access token is in the JWT format (as opposed to the opaque format), the server config ``uaa.jwt.revocable`` or +the Identity Zone config ``config.tokenPolicy.jwtRevocable`` must be set to ``true`` for +this feature to work. However, OAuth resource servers are generally not required to call the UAA Introspect +Token endpoint to validate the status of the token. Once issued, a valid access token +in the JWT format is generally considered valid until its expiry. Hence, we do not recommend +relying on this endpoint to revoke access tokens in the JWT format. If the ability +to remove/limit access after the tokens are issued is important to you, we recommend the following instead: + +* Ask the OAuth client to use opaque tokens only, so that the OAuth resource server is required to use +the UAA Introspect Token endpoint to validate that the tokens have not been revoked. +* If the access tokens are in the JWT format, configure the access tokens to be short-lived +(e.g. a few minutes), and when needed, revoke the more long-lived refresh tokens so that they +may no longer be used to obtain refreshed access tokens. + +When a refresh token is in the opaque format and revoked using this endpoint, the refresh token +will no longer be considered valid when used to perform the Refresh Token grant. +When a refresh token is in the JWT format, the server config ``uaa.jwt.revocable`` or +the Identity Zone config ``config.tokenPolicy.jwtRevocable`` must be set to ``true`` for +this feature to work. + ### Revoke all tokens for a user <%= render('TokenEndpointDocs/revokeAllTokens_forAUser/curl-request.md') %> diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/TokenEndpointDocs.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/TokenEndpointDocs.java index 65bc30912f3..2989bc2ac54 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/TokenEndpointDocs.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/TokenEndpointDocs.java @@ -104,7 +104,7 @@ class TokenEndpointDocs extends AbstractTokenMockMvcTests { private final ParameterDescriptor clientIdParameter = parameterWithName(CLIENT_ID).optional(null).type(STRING).description("A unique string representing the registration information provided by the client, the recipient of the token. Optional if it is passed as part of the Basic Authorization header or as part of the client_assertion."); private final ParameterDescriptor clientSecretParameter = parameterWithName("client_secret").optional(null).type(STRING).description("The [secret passphrase configured](#change-secret) for the OAuth client. Optional if it is passed as part of the Basic Authorization header or if client_assertion is sent as part of private_key_jwt authentication."); - private final ParameterDescriptor opaqueFormatParameter = parameterWithName(REQUEST_TOKEN_FORMAT).optional(null).type(STRING).description("Can be set to `" + OPAQUE.getStringValue() + "` to retrieve an opaque and revocable token or to `" + JWT.getStringValue() + "` to retrieve a JWT token. If not set the zone setting config.tokenPolicy.jwtRevocable is used."); + private final ParameterDescriptor opaqueFormatParameter = parameterWithName(REQUEST_TOKEN_FORMAT).optional(null).type(STRING).description("Can be set to `" + OPAQUE.getStringValue() + "` to retrieve an opaque token or to `" + JWT.getStringValue() + "` to retrieve a JWT token. Please refer to the Revoke Tokens endpoint doc for information about the revocability of opaque vs. jwt tokens. If not set the zone setting config.tokenPolicy.jwtRevocable is used."); private final ParameterDescriptor scopeParameter = parameterWithName(SCOPE).optional(null).type(STRING).description("The list of scopes requested for the token. Use when you wish to reduce the number of scopes the token will have."); private final ParameterDescriptor loginHintParameter = parameterWithName("login_hint").optional(null).type(STRING).description("UAA 75.5.0 Indicates the identity provider to be used. The passed string has to be a URL-Encoded JSON Object, containing the field `origin` with value as `origin_key` of an identity provider. Note that this identity provider must support the grant type `password`."); private final ParameterDescriptor codeVerifier = parameterWithName(PkceValidationService.CODE_VERIFIER).description("UAA 75.5.0 [PKCE](https://tools.ietf.org/html/rfc7636) Code Verifier. A `code_verifier` parameter must be provided if a `code_challenge` parameter was present in the previous call to `/oauth/authorize`. The `code_verifier` must match the used `code_challenge` (according to the selected `code_challenge_method`)").attributes(key("constraints").value("Optional"), key("type").value(STRING)); From b3b5143b909558ff9c5a5cb3d2c0fe470c37f279 Mon Sep 17 00:00:00 2001 From: Bruce Ricard Date: Thu, 29 Feb 2024 11:48:04 -0800 Subject: [PATCH 28/42] doc: clarify token revocation (some edits) [#177045463] Co-authored-by: Peter Chen --- .../source/index.html.md.erb | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/uaa/slateCustomizations/source/index.html.md.erb b/uaa/slateCustomizations/source/index.html.md.erb index d5dc440163b..f9ba28177f8 100644 --- a/uaa/slateCustomizations/source/index.html.md.erb +++ b/uaa/slateCustomizations/source/index.html.md.erb @@ -378,14 +378,17 @@ _Response Fields_ Added in UAA 3.3.0 -When an access token is revoked using this endpoint, the UAA Introspect Token endpoint (``/introspect``) -will respond with ``"active": false`` when presented with the revoked access token. +Both access and refresh tokens can be passed to the ``/revoke`` endpoint. + +When an access token is successfully passed to the ``/revoke`` endpoint, and then when the same token is +passed to the UAA Introspect Token endpoint (``/introspect``), the UAA Introspect Token endpoint +will respond with ``"active": false``. If the access token is in the JWT format (as opposed to the opaque format), the server config ``uaa.jwt.revocable`` or the Identity Zone config ``config.tokenPolicy.jwtRevocable`` must be set to ``true`` for -this feature to work. However, OAuth resource servers are generally not required to call the UAA Introspect -Token endpoint to validate the status of the token. Once issued, a valid access token -in the JWT format is generally considered valid until its expiry. Hence, we do not recommend +the revocation to work. However, OAuth resource servers are not required to call the UAA Introspect +Token endpoint to validate the token. Once issued, from a security point of view, a valid access token +in the JWT format should be considered valid until its expiry. Hence, we do not recommend relying on this endpoint to revoke access tokens in the JWT format. If the ability to remove/limit access after the tokens are issued is important to you, we recommend the following instead: @@ -395,11 +398,11 @@ the UAA Introspect Token endpoint to validate that the tokens have not been revo (e.g. a few minutes), and when needed, revoke the more long-lived refresh tokens so that they may no longer be used to obtain refreshed access tokens. -When a refresh token is in the opaque format and revoked using this endpoint, the refresh token -will no longer be considered valid when used to perform the Refresh Token grant. +When a refresh token in the opaque format is successfully passed to the ``/revoke`` endpoint, +the refresh token can no longer be used to perform the Refresh Token grant. When a refresh token is in the JWT format, the server config ``uaa.jwt.revocable`` or the Identity Zone config ``config.tokenPolicy.jwtRevocable`` must be set to ``true`` for -this feature to work. +the "Revoke a single token" endpoint (``/oauth/token/revoke/{tokenId}``) to work. ### Revoke all tokens for a user From e1d0a6981f860642a183d2455f8dd446175dd199 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Fri, 1 Mar 2024 11:45:25 -0800 Subject: [PATCH 29/42] doc: further clarify refresh token revocation [#177045463] --- uaa/slateCustomizations/source/index.html.md.erb | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/uaa/slateCustomizations/source/index.html.md.erb b/uaa/slateCustomizations/source/index.html.md.erb index f9ba28177f8..97aa7f644da 100644 --- a/uaa/slateCustomizations/source/index.html.md.erb +++ b/uaa/slateCustomizations/source/index.html.md.erb @@ -398,11 +398,18 @@ the UAA Introspect Token endpoint to validate that the tokens have not been revo (e.g. a few minutes), and when needed, revoke the more long-lived refresh tokens so that they may no longer be used to obtain refreshed access tokens. -When a refresh token in the opaque format is successfully passed to the ``/revoke`` endpoint, +When a refresh token is successfully passed to the ``/revoke`` endpoint, the refresh token can no longer be used to perform the Refresh Token grant. -When a refresh token is in the JWT format, the server config ``uaa.jwt.revocable`` or -the Identity Zone config ``config.tokenPolicy.jwtRevocable`` must be set to ``true`` for -the "Revoke a single token" endpoint (``/oauth/token/revoke/{tokenId}``) to work. + +Refresh tokens in any format can be revoked using the "Revoke all tokens for a user" endpoint (``/oauth/token/revoke/user/{userId}``), +the "Revoke all tokens for a client" endpoint (``/oauth/token/revoke/client/{clientId}``), or +the "Revoke all tokens for a user and client combination" endpoint (``/oauth/token/revoke/user/{userId}/client/{clientId}``). + +Refresh tokens in the opaque format can be individually revoked using +the "Revoke a single token" endpoint (``/oauth/token/revoke/{tokenId}``). +However, refresh tokens in the JWT format can only be individually revoked using +the "Revoke a single token" endpoint when the server config ``uaa.jwt.revocable`` or +the Identity Zone config ``config.tokenPolicy.jwtRevocable`` is set to ``true``. ### Revoke all tokens for a user From 78908b6b88d9fbb44923ec7b47cdb20c2e4b26bc Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Fri, 1 Mar 2024 14:28:50 -0800 Subject: [PATCH 30/42] doc: clarify token revocation (some further edits) [#177045463] --- uaa/slateCustomizations/source/index.html.md.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/uaa/slateCustomizations/source/index.html.md.erb b/uaa/slateCustomizations/source/index.html.md.erb index 97aa7f644da..00c624b9bc1 100644 --- a/uaa/slateCustomizations/source/index.html.md.erb +++ b/uaa/slateCustomizations/source/index.html.md.erb @@ -380,7 +380,7 @@ _Response Fields_ Both access and refresh tokens can be passed to the ``/revoke`` endpoint. -When an access token is successfully passed to the ``/revoke`` endpoint, and then when the same token is +When the ``/revoke`` endpoint is successfully invoked with an access token, and then when the same token is passed to the UAA Introspect Token endpoint (``/introspect``), the UAA Introspect Token endpoint will respond with ``"active": false``. @@ -398,7 +398,7 @@ the UAA Introspect Token endpoint to validate that the tokens have not been revo (e.g. a few minutes), and when needed, revoke the more long-lived refresh tokens so that they may no longer be used to obtain refreshed access tokens. -When a refresh token is successfully passed to the ``/revoke`` endpoint, +When the ``/revoke`` endpoint is successfully invoked with a refresh token, the refresh token can no longer be used to perform the Refresh Token grant. Refresh tokens in any format can be revoked using the "Revoke all tokens for a user" endpoint (``/oauth/token/revoke/user/{userId}``), From 56df3a5605f44433202250155d4b480fe6a6a1c0 Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Wed, 6 Mar 2024 09:33:06 +0100 Subject: [PATCH 31/42] suppress false warnings --- .../cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 5566a4be5c6..3a7e6f09c9b 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -6,6 +6,7 @@ /** * Returns Log instance that replaces \n, \r, \t with a | to prevent log forging. */ +@SuppressWarnings({"javasecurity:S5145", "javasecurity:S2629"}) // sanitize log messages public class SanitizedLogFactory { public static SanitizedLog getLog(Class clazz) { From d7724376a0b1582feb35cf33a9f0d99cb65c3d4e Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Wed, 6 Mar 2024 10:42:19 +0100 Subject: [PATCH 32/42] suppress performance warning --- .../cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 3a7e6f09c9b..3b369590f22 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -6,7 +6,7 @@ /** * Returns Log instance that replaces \n, \r, \t with a | to prevent log forging. */ -@SuppressWarnings({"javasecurity:S5145", "javasecurity:S2629"}) // sanitize log messages +@SuppressWarnings({"javasecurity:S5145", "java:S2629"}) // sanitize log messages public class SanitizedLogFactory { public static SanitizedLog getLog(Class clazz) { From 3417acf8b389e3aeea0912786748b297f48360d0 Mon Sep 17 00:00:00 2001 From: Markus Strehle <11627201+strehle@users.noreply.github.com> Date: Thu, 7 Mar 2024 09:23:35 +0100 Subject: [PATCH 33/42] Sonar fixes (#2760) * Sonar fixes * refactor --- .../uaa/logging/LogSanitizerUtil.java | 5 +++- .../uaa/logging/SanitizedLogFactory.java | 25 +++++++++++++------ .../uaa/logging/SanitizedLogFactoryTest.java | 18 ++++++++++++- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/LogSanitizerUtil.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/LogSanitizerUtil.java index 4412bc9ff89..3e266df033d 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/LogSanitizerUtil.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/LogSanitizerUtil.java @@ -6,9 +6,12 @@ public class LogSanitizerUtil { public static final String SANITIZED_FLAG = "[SANITIZED]"; + private LogSanitizerUtil() { + } + @Nullable public static String sanitize(String original) { - if (original == null) return original; + if (original == null) return null; String cleaned = original.replace("\r","|") .replace("\n","|") diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 81956f292a0..c9e43d4c657 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -1,15 +1,18 @@ package org.cloudfoundry.identity.uaa.logging; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; /** * Returns Log instance that replaces \n, \r, \t with a | to prevent log forging. */ public class SanitizedLogFactory { + private SanitizedLogFactory() { + } + public static SanitizedLog getLog(Class clazz) { - return new SanitizedLog(LoggerFactory.getLogger(clazz)); + return new SanitizedLog(LogManager.getLogger(clazz)); } public static class SanitizedLog { @@ -24,19 +27,27 @@ public boolean isDebugEnabled() { } public void info(String message) { - fallback.info(LogSanitizerUtil.sanitize(message)); + if (fallback.isInfoEnabled()) { + fallback.info(LogSanitizerUtil.sanitize(message)); + } } public void warn(String message) { - fallback.warn(LogSanitizerUtil.sanitize(message)); + if (fallback.isWarnEnabled()) { + fallback.warn(LogSanitizerUtil.sanitize(message)); + } } public void debug(String message) { - fallback.debug(LogSanitizerUtil.sanitize(message)); + if (fallback.isDebugEnabled()) { + fallback.debug(LogSanitizerUtil.sanitize(message)); + } } public void debug(String message, Throwable t) { - fallback.debug(LogSanitizerUtil.sanitize(message), t); + if (fallback.isDebugEnabled()) { + fallback.debug(LogSanitizerUtil.sanitize(message), t); + } } } } \ No newline at end of file diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java index 60bf5e795be..15edfa1aa24 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java @@ -1,9 +1,10 @@ package org.cloudfoundry.identity.uaa.logging; -import org.slf4j.Logger; +import org.apache.logging.log4j.Logger; import org.junit.Before; import org.junit.Test; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -19,6 +20,7 @@ public void setUp() { @Test public void testSanitizeDebug() { SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); + doReturn(true).when(mockLog).isDebugEnabled(); log.debug("one\ntwo\tthree\rfour"); verify(mockLog).debug("one|two|three|four[SANITIZED]"); } @@ -26,13 +28,24 @@ public void testSanitizeDebug() { @Test public void testSanitizeDebugCleanMessage() { SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); + doReturn(true).when(mockLog).isDebugEnabled(); log.debug("one two three four"); verify(mockLog).debug("one two three four"); } + @Test + public void testSanitizeDebugCleanMessageException() { + SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); + doReturn(true).when(mockLog).isDebugEnabled(); + Exception exception = new Exception(""); + log.debug("one two three four", exception); + verify(mockLog).debug("one two three four", exception); + } + @Test public void testSanitizeInfo() { SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); + doReturn(true).when(mockLog).isInfoEnabled(); log.info("one\ntwo\tthree\rfour"); verify(mockLog).info("one|two|three|four[SANITIZED]"); } @@ -40,6 +53,7 @@ public void testSanitizeInfo() { @Test public void testSanitizeInfoCleanMessage() { SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); + doReturn(true).when(mockLog).isInfoEnabled(); log.info("one two three four"); verify(mockLog).info("one two three four"); } @@ -47,6 +61,7 @@ public void testSanitizeInfoCleanMessage() { @Test public void testSanitizeWarn() { SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); + doReturn(true).when(mockLog).isWarnEnabled(); log.warn("one\ntwo\tthree\rfour"); verify(mockLog).warn("one|two|three|four[SANITIZED]"); } @@ -54,6 +69,7 @@ public void testSanitizeWarn() { @Test public void testSanitizeWarnCleanMessage() { SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); + doReturn(true).when(mockLog).isWarnEnabled(); log.warn("one two three four"); verify(mockLog).warn("one two three four"); } From 709a3724ff32a7a98333f14045d178459848ac6a Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Thu, 7 Mar 2024 10:21:55 +0100 Subject: [PATCH 34/42] Rework --- .../uaa/scim/ScimUserProvisioning.java | 6 ++++-- .../endpoints/UserIdConversionEndpoints.java | 18 +++++++++++------- .../scim/jdbc/JdbcScimUserProvisioning.java | 12 +----------- .../UserIdConversionEndpointsTests.java | 8 ++++++-- .../jdbc/JdbcScimUserProvisioningTests.java | 8 +++----- 5 files changed, 25 insertions(+), 27 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUserProvisioning.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUserProvisioning.java index 749380f6b89..59e8992d4ff 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUserProvisioning.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUserProvisioning.java @@ -29,9 +29,11 @@ public interface ScimUserProvisioning extends ResourceManager, Queryab List retrieveByUsernameAndZone(String username, String zoneId); - List retrieveByScimFilter( + /** + * Retrieve all users that satisfy the given SCIM filter and stem from active IdPs. + */ + List retrieveByScimFilterOnlyActive( String filter, - boolean includeInactive, String sortBy, boolean ascending, String zoneId diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java index f5b91bcd526..1097f7e69ae 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpoints.java @@ -91,13 +91,17 @@ public ResponseEntity findUsers( checkFilter(filter); // get all users for the given filter and the current page - final List filteredUsers = scimUserProvisioning.retrieveByScimFilter( - filter, - includeInactive, - FIELD_USERNAME, - sortOrder.equalsIgnoreCase("ascending"), - identityZoneManager.getCurrentIdentityZoneId() - ); + final boolean ascending = sortOrder.equalsIgnoreCase("ascending"); + final List filteredUsers; + if (includeInactive) { + filteredUsers = scimUserProvisioning.query( + filter, FIELD_USERNAME, ascending, identityZoneManager.getCurrentIdentityZoneId() + ); + } else { + filteredUsers = scimUserProvisioning.retrieveByScimFilterOnlyActive( + filter, FIELD_USERNAME, ascending, identityZoneManager.getCurrentIdentityZoneId() + ); + } final List usersCurrentPage = UaaPagingUtils.subList(filteredUsers, startIndex, count); // map to result structure diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java index 010322f505c..7ead5e001eb 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java @@ -176,23 +176,13 @@ public List retrieveByUsernameAndZone(String username, String zoneId) return jdbcTemplate.query(USER_BY_USERNAME_AND_ZONE_QUERY , mapper, username, zoneId); } - /** - * This method should behave similar to {@link JdbcScimUserProvisioning#query(String, String, boolean, String)}, - * with the filtering for active IdPs (if required) being the only difference. - */ @Override - public List retrieveByScimFilter( + public List retrieveByScimFilterOnlyActive( final String filter, - final boolean includeInactive, final String sortBy, final boolean ascending, final String zoneId ) { - if (includeInactive) { - // no filtering for active IdPs necessary - return query(filter, sortBy, ascending, zoneId); - } - final SimpleSearchQueryConverter queryConverter = new SimpleSearchQueryConverter(); validateOrderBy(queryConverter.map(sortBy)); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java index e9d344b3711..63b56335269 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java @@ -309,8 +309,12 @@ private void arrangeScimUsersForFilter( final boolean includeInactive, final String zoneId ) { - when(scimUserProvisioning.retrieveByScimFilter(filter, includeInactive, "userName", true, zoneId)) - .thenReturn(allScimUsers); + if (includeInactive) { + when(scimUserProvisioning.query(filter, "userName", true, zoneId)).thenReturn(allScimUsers); + } else { + when(scimUserProvisioning.retrieveByScimFilterOnlyActive(filter, "userName", true, zoneId)) + .thenReturn(allScimUsers); + } } private void assertEndpointReturnsCorrectResult( diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java index 93960fbd2b7..ebc85c22a88 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java @@ -235,7 +235,7 @@ void canDeleteProviderUsersInDefaultZone() { } @Test - void retrieveByScimFilter_OnlyActive() { + void retrieveByScimFilterOnlyActive() { final String originActive = randomString(); addIdentityProvider(jdbcTemplate, currentIdentityZoneId, originActive, true); @@ -253,9 +253,8 @@ void retrieveByScimFilter_OnlyActive() { final ScimUser created2 = jdbcScimUserProvisioning.createUser(user2, "j7hyqpassX", currentIdentityZoneId); final Function> retrieveByScimFilter = (scimFilter) -> { - final List result = jdbcScimUserProvisioning.retrieveByScimFilter( + final List result = jdbcScimUserProvisioning.retrieveByScimFilterOnlyActive( scimFilter, - false, "userName", true, currentIdentityZoneId @@ -303,9 +302,8 @@ void retrieveByScimFilter_IncludeInactive() { final ScimUser created2 = jdbcScimUserProvisioning.createUser(user2, "j7hyqpassX", currentIdentityZoneId); final Function> retrieveByScimFilter = (scimFilter) -> { - final List result = jdbcScimUserProvisioning.retrieveByScimFilter( + final List result = jdbcScimUserProvisioning.query( scimFilter, - true, "userName", true, currentIdentityZoneId From 73979d3a60fe0c01711361f4e8664ffbef75e299 Mon Sep 17 00:00:00 2001 From: Markus Strehle <11627201+strehle@users.noreply.github.com> Date: Thu, 7 Mar 2024 11:22:57 +0100 Subject: [PATCH 35/42] Solve sonar bug (#2768) https://sonarcloud.io/project/issues?resolved=false&types=BUG&id=cloudfoundry-identity-parent&open=AY3TMOLsnEWJ5Tc4_8N_ Use constant in SPEL then no encoding issue with URL. --- .../main/java/org/cloudfoundry/identity/uaa/home/BuildInfo.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/home/BuildInfo.java b/server/src/main/java/org/cloudfoundry/identity/uaa/home/BuildInfo.java index 5a98c468537..3df68298404 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/home/BuildInfo.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/home/BuildInfo.java @@ -15,7 +15,7 @@ public class BuildInfo implements InitializingBean { private final Logger logger = LoggerFactory.getLogger(getClass()); - @Value("${uaa.url:http://localhost:8080/uaa}") + @Value("${uaa.url:#{T(org.cloudfoundry.identity.uaa.util.UaaStringUtils).DEFAULT_UAA_URL}}") private String uaaUrl; private String version; private String commitId; From c57dad7ace41ecbb65f75d1fafa335fa5e07c45c Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 7 Mar 2024 12:10:08 +0100 Subject: [PATCH 36/42] improve coverage --- .../uaa/logging/SanitizedLogFactory.java | 3 +- .../uaa/logging/SanitizedLogFactoryTest.java | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 81ed016f5c1..509d7482f9c 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -8,8 +8,7 @@ */ public class SanitizedLogFactory { - private SanitizedLogFactory() { - } + private SanitizedLogFactory() { } public static SanitizedLog getLog(Class clazz) { return new SanitizedLog(LogManager.getLogger(clazz)); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java index 4601270a47e..f464c56f936 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java @@ -1,10 +1,12 @@ package org.cloudfoundry.identity.uaa.logging; import org.apache.logging.log4j.Logger; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -27,6 +29,11 @@ public void setUp() { ex = new Exception(RandomStringUtils.randomAlphanumeric(8)); } + @Test + public void testInit() { + Assert.assertNotNull(SanitizedLogFactory.getLog(SanitizedLogFactoryTest.class)); + } + @Test public void testSanitizeInfo() { when(mockLog.isInfoEnabled()).thenReturn(true); @@ -63,6 +70,17 @@ public void testSanitizeDebugCleanMessage() { verify(mockLog).debug(cleanMessage, ex); } + @Test + public void testSanitizeDebugCleanMessageNotEnabled() { + when(mockLog.isDebugEnabled()).thenReturn(false); + log.debug(cleanMessage); + verify(mockLog, never()).debug(cleanMessage); + log.debug(cleanMessage, ex); + verify(mockLog, never()).debug(cleanMessage, ex); + Assert.assertFalse(log.isDebugEnabled()); + } + + @Test public void testSanitizeWarn() { when(mockLog.isWarnEnabled()).thenReturn(true); @@ -81,6 +99,15 @@ public void testSanitizeWarnCleanMessage() { verify(mockLog).warn(cleanMessage, ex); } + @Test + public void testSanitizeWarnCleanMessageNotEnabled() { + when(mockLog.isWarnEnabled()).thenReturn(false); + log.warn(cleanMessage); + verify(mockLog, never()).warn(cleanMessage); + log.warn(cleanMessage, ex); + verify(mockLog, never()).warn(cleanMessage, ex); + } + @Test public void testSanitizeError() { when(mockLog.isErrorEnabled()).thenReturn(true); @@ -116,4 +143,13 @@ public void testSanitizeTraceCleanMessage() { log.trace(cleanMessage, ex); verify(mockLog).trace(cleanMessage, ex); } + + @Test + public void testSanitizeTraceCleanMessageWhenNotEnabled() { + when(mockLog.isTraceEnabled()).thenReturn(false); + log.trace(cleanMessage); + verify(mockLog, never()).trace(cleanMessage); + log.trace(cleanMessage, ex); + verify(mockLog, never()).trace(cleanMessage, ex); + } } From 2644f537be1d123a4c5db04877070d1a112963c4 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Fri, 8 Mar 2024 10:21:53 +0100 Subject: [PATCH 37/42] Rework --- .../UserIdConversionEndpointsTests.java | 253 ++++++------------ 1 file changed, 87 insertions(+), 166 deletions(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java index 63b56335269..af8f00ac83a 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java @@ -16,13 +16,10 @@ import static java.util.Collections.singletonList; import static java.util.UUID.randomUUID; import static java.util.stream.Collectors.toList; -import static junit.framework.Assert.assertEquals; -import static junit.framework.Assert.assertTrue; import static org.assertj.core.api.Assertions.assertThat; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.is; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.mockStatic; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatNoException; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.when; import java.util.ArrayList; @@ -36,16 +33,14 @@ import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning; import org.cloudfoundry.identity.uaa.scim.exception.ScimException; import org.cloudfoundry.identity.uaa.security.beans.SecurityContextAccessor; -import org.cloudfoundry.identity.uaa.zone.IdentityZone; -import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder; import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager; -import org.junit.After; -import org.junit.Assert; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.ExpectedException; -import org.mockito.MockedStatic; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.security.core.authority.AuthorityUtils; @@ -54,74 +49,48 @@ * @author Dave Syer * @author Luke Taylor */ -public class UserIdConversionEndpointsTests { - - @Rule - public ExpectedException expected = ExpectedException.none(); - +@ExtendWith(MockitoExtension.class) +class UserIdConversionEndpointsTests { private UserIdConversionEndpoints endpoints; + @Mock private SecurityContextAccessor mockSecurityContextAccessor; - - private final ScimUserEndpoints scimUserEndpoints = mock(ScimUserEndpoints.class); - - private final ScimUserProvisioning scimUserProvisioning = mock(ScimUserProvisioning.class); - - private final MockedStatic idzHolderMockedStatic; - private final IdentityZoneManager identityZoneManager = mock(IdentityZoneManager.class); + @Mock + private ScimUserEndpoints scimUserEndpoints; + @Mock + private ScimUserProvisioning scimUserProvisioning; + @Mock + private IdentityZoneManager identityZoneManager; @SuppressWarnings("rawtypes") private final Collection authorities = AuthorityUtils .commaSeparatedStringToAuthorityList("orgs.foo,uaa.user"); - public UserIdConversionEndpointsTests() { - this.idzHolderMockedStatic = mockStatic(IdentityZoneHolder.class); - } - @SuppressWarnings("unchecked") - @Before + @BeforeEach public void init() { - mockSecurityContextAccessor = mock(SecurityContextAccessor.class); endpoints = new UserIdConversionEndpoints(mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, identityZoneManager, true); - when(mockSecurityContextAccessor.getAuthorities()).thenReturn(authorities); - when(mockSecurityContextAccessor.getAuthenticationInfo()).thenReturn("mock object"); - when(scimUserEndpoints.getUserMaxCount()).thenReturn(10_000); - } - - @After - public void tearDown() throws Exception { - idzHolderMockedStatic.close(); + lenient().when(mockSecurityContextAccessor.getAuthorities()).thenReturn(authorities); + lenient().when(mockSecurityContextAccessor.getAuthenticationInfo()).thenReturn("mock object"); + lenient().when(scimUserEndpoints.getUserMaxCount()).thenReturn(10_000); } @Test - public void testHappyDay() { + void testHappyDay() { arrangeCurrentIdentityZone("uaa"); - endpoints.findUsers("userName eq \"marissa\"", "ascending", 0, 100, false); + assertThatNoException() + .isThrownBy(() -> endpoints.findUsers("userName eq \"marissa\"", "ascending", 0, 100, false)); } @Test - public void testSanitizeExceptionInFilter() { - expected.expect(ScimException.class); - expected.expectMessage(is("Invalid filter '<svg onload=alert(document.domain)>'")); - endpoints.findUsers("", "ascending", 0, 100, false); + void testSanitizeExceptionInFilter() { + assertThatExceptionOfType(ScimException.class) + .isThrownBy(() -> endpoints.findUsers("", "ascending", 0, 100, false)) + .withMessage("Invalid filter '<svg onload=alert(document.domain)>'"); } @Test - public void testBadFieldInFilter() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Invalid filter")); - endpoints.findUsers("emails.value eq \"foo@bar.org\"", "ascending", 0, 100, false); - } - - @Test - public void testBadFilterWithGroup() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Invalid filter")); - endpoints.findUsers("groups.display eq \"foo\"", "ascending", 0, 100, false); - } - - @Test - public void testGoodFilter_IncludeInactive() { + void testGoodFilter_IncludeInactive() { final String idzId = randomUUID().toString(); arrangeCurrentIdentityZone(idzId); @@ -148,7 +117,7 @@ public void testGoodFilter_IncludeInactive() { } @Test - public void testGoodFilter_OnlyActive() { + void testGoodFilter_OnlyActive() { final String idzId = randomUUID().toString(); arrangeCurrentIdentityZone(idzId); @@ -168,27 +137,14 @@ public void testGoodFilter_OnlyActive() { assertEndpointReturnsCorrectResult(filter, 10, expectedUsers, false); } - @Test - public void testGoodFilter1() { - testGoodFilter("(id eq \"foo\" or username eq \"bar\") and origin eq \"uaa\""); - } - - @Test - public void testGoodFilter2() { - testGoodFilter("origin eq \"uaa\" and (id eq \"foo\" or username eq \"bar\")"); - } - - @Test - public void testGoodFilter3() { - testGoodFilter("(id eq \"foo\" and username eq \"bar\") or id eq \"bar\""); - } - - @Test - public void testGoodFilter4() { - testGoodFilter("id eq \"bar\" and (id eq \"foo\" and username eq \"bar\")"); - } - - private void testGoodFilter(final String filter) { + @ParameterizedTest + @ValueSource(strings = { + "(id eq \"foo\" or username eq \"bar\") and origin eq \"uaa\"", + "origin eq \"uaa\" and (id eq \"foo\" or username eq \"bar\")", + "(id eq \"foo\" and username eq \"bar\") or id eq \"bar\"", + "id eq \"bar\" and (id eq \"foo\" and username eq \"bar\")" + }) + void testGoodFilter(final String filter) { arrangeCurrentIdentityZone("uaa"); final ResponseEntity response = endpoints.findUsers( filter, @@ -197,109 +153,74 @@ private void testGoodFilter(final String filter) { 100, false ); - assertEquals(HttpStatus.OK, response.getStatusCode()); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); } - @Test - public void testBadFilter1() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Wildcards are not allowed in filter.")); - endpoints.findUsers("id co \"foo\"", "ascending", 0, 100, false); - } - - @Test - public void testBadFilter2() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Invalid filter")); - endpoints.findUsers("id sq \"foo\"", "ascending", 0, 100, false); - } - - @Test - public void testBadFilter3() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Wildcards are not allowed in filter.")); - endpoints.findUsers("id sw \"foo\"", "ascending", 0, 100, false); + @ParameterizedTest + @ValueSource(strings = { + "id co \"foo\"", + "id sw \"foo\"", + "id pr", + "id eq \"foo\" or origin co \"uaa\"" + }) + void testBadFilter_WildcardsNotAllowed(final String filter) { + assertThatExceptionOfType(ScimException.class) + .isThrownBy(() -> endpoints.findUsers(filter, "ascending", 0, 100, false)) + .withMessage("Wildcards are not allowed in filter."); } - @Test - public void testBadFilter4() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Wildcards are not allowed in filter.")); - endpoints.findUsers("id pr", "ascending", 0, 100, false); + @ParameterizedTest + @ValueSource(strings = { + "id gt \"foo\"", + "id le \"foo\"", + "id lt \"foo\"" + }) + void testBadFilter_UnsupportedOperator(final String filter) { + assertThatExceptionOfType(ScimException.class) + .isThrownBy(() -> endpoints.findUsers(filter, "ascending", 0, 100, false)) + .withMessage("Invalid operator."); } - @Test - public void testBadFilter5() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Invalid operator.")); - endpoints.findUsers("id gt \"foo\"", "ascending", 0, 100, false); + @ParameterizedTest + @ValueSource(strings = { + "id sq \"foo\"" + }) + void testBadFilter_UnrecognizedOperator(final String filter) { + assertThatExceptionOfType(ScimException.class) + .isThrownBy(() -> endpoints.findUsers(filter, "ascending", 0, 100, false)) + .withMessageStartingWith("Invalid filter '"); } - @Test - public void testBadFilter6() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Invalid operator.")); - endpoints.findUsers("id gt \"foo\"", "ascending", 0, 100, false); + @ParameterizedTest + @ValueSource(strings = { + "origin eq \"uaa\"", + "emails.value eq \"foo@bar.org\"", + "origin eq \"uaa\" or origin eq \"bar\"", + "groups.display eq \"foo\"" + }) + void testBadFilter_DoesNotContainClauseWithIdOrUserName(final String filter) { + assertThatExceptionOfType(ScimException.class) + .isThrownBy(() -> endpoints.findUsers(filter, "ascending", 0, 100, false)) + .withMessage("Invalid filter attribute."); } @Test - public void testBadFilter7() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Invalid operator.")); - endpoints.findUsers("id lt \"foo\"", "ascending", 0, 100, false); - } - - @Test - public void testBadFilter8() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Invalid operator.")); - endpoints.findUsers("id le \"foo\"", "ascending", 0, 100, false); - } - - @Test - public void testBadFilter9() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Invalid filter")); - endpoints.findUsers("origin eq \"uaa\"", "ascending", 0, 100, false); - } - - @Test - public void testBadFilter10() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Wildcards are not allowed in filter.")); - - // illegal operator in right operand of root-level "or" -> all branches need to be checked even if left operands are valid - endpoints.findUsers("id eq \"foo\" or origin co \"uaa\"", "ascending", 0, 100, false); - } - - @Test - public void testBadFilter11() { - expected.expect(ScimException.class); - expected.expectMessage(containsString("Invalid filter")); - endpoints.findUsers("origin eq \"uaa\" or origin eq \"bar\"", "ascending", 0, 100, false); - } - - @Test - public void testDisabled() { - arrangeCurrentIdentityZone("uaa"); + void testDisabled() { endpoints = new UserIdConversionEndpoints(mockSecurityContextAccessor, scimUserEndpoints, scimUserProvisioning, identityZoneManager, false); ResponseEntity response = endpoints.findUsers("id eq \"foo\"", "ascending", 0, 100, false); - Assert.assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode()); - Assert.assertEquals("Illegal Operation: Endpoint not enabled.", response.getBody()); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(response.getBody()).isEqualTo("Illegal Operation: Endpoint not enabled."); } @Test - public void noActiveIdps_ReturnsEmptyResources() { + void noActiveIdps_ReturnsEmptyResources() { arrangeCurrentIdentityZone("uaa"); SearchResults searchResults = (SearchResults) endpoints.findUsers("username eq \"foo\"", "ascending", 0, 100, false).getBody(); - assertTrue(searchResults.getResources().isEmpty()); + assertThat(searchResults).isNotNull(); + assertThat(searchResults.getResources()).isEmpty(); } private void arrangeCurrentIdentityZone(final String idzId) { - final IdentityZone identityZone = new IdentityZone(); - identityZone.setId(idzId); - idzHolderMockedStatic.when(IdentityZoneHolder::get).thenReturn(identityZone); - idzHolderMockedStatic.when(IdentityZoneHolder::getCurrentZoneId).thenReturn(idzId); when(identityZoneManager.getCurrentIdentityZoneId()).thenReturn(idzId); } From b9348ef93d17c7050ebbda7d91ed9a1bee2b3369 Mon Sep 17 00:00:00 2001 From: Markus Strehle <11627201+strehle@users.noreply.github.com> Date: Fri, 8 Mar 2024 10:34:02 +0100 Subject: [PATCH 38/42] Fix first part of issue #2752 (#2758) By mistake the variable jwtclientAuthentication was used in PR for clientAuthentication. Documentation explain jwtClientAuthentication as entry --- .../oauth/OauthIDPWrapperFactoryBean.java | 23 +++++++++++-------- ...tityProviderDefinitionFactoryBeanTest.java | 23 +++++++++++++++++-- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIDPWrapperFactoryBean.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIDPWrapperFactoryBean.java index 198dd94286a..046c99fcf3e 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIDPWrapperFactoryBean.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIDPWrapperFactoryBean.java @@ -23,6 +23,7 @@ import java.net.MalformedURLException; import java.net.URL; +import java.util.Collections; import java.util.HashMap; import java.util.LinkedList; import java.util.List; @@ -88,26 +89,30 @@ private AbstractExternalOAuthIdentityProviderDefinition getExternalOIDCIdentityP idpDefinitionMap.get("passwordGrantEnabled") == null ? false : (boolean) idpDefinitionMap.get("passwordGrantEnabled")); oidcIdentityProviderDefinition.setSetForwardHeader(idpDefinitionMap.get("setForwardHeader") == null ? false : (boolean) idpDefinitionMap.get("passwordGrantEnabled")); oidcIdentityProviderDefinition.setPrompts((List) idpDefinitionMap.get("prompts")); - setJwtClientAuthentication("jwtclientAuthentication", idpDefinitionMap, oidcIdentityProviderDefinition); + setJwtClientAuthentication(idpDefinitionMap, oidcIdentityProviderDefinition); oauthIdpDefinitions.put(alias, oidcIdentityProviderDefinition); rawDef = oidcIdentityProviderDefinition; provider.setType(OriginKeys.OIDC10); return rawDef; } - private static void setJwtClientAuthentication(String entry, Map map, OIDCIdentityProviderDefinition definition) { - if (map.get(entry) != null) { - if (map.get(entry) instanceof Boolean) { - boolean jwtClientAuthentication = (Boolean) map.get(entry); - if (jwtClientAuthentication) { - definition.setJwtClientAuthentication(new HashMap<>()); + private static void setJwtClientAuthentication(Map map, OIDCIdentityProviderDefinition definition) { + Object jwtClientAuthDetails = getJwtClientAuthenticationDetails(map, List.of("jwtClientAuthentication", "jwtclientAuthentication")); + if (jwtClientAuthDetails != null) { + if (jwtClientAuthDetails instanceof Boolean boolValue) { + if (boolValue.booleanValue()) { + definition.setJwtClientAuthentication(Collections.emptyMap()); } - } else if (map.get(entry) instanceof HashMap) { - definition.setJwtClientAuthentication(map.get(entry)); + } else if (jwtClientAuthDetails instanceof Map) { + definition.setJwtClientAuthentication(jwtClientAuthDetails); } } } + private static Object getJwtClientAuthenticationDetails(Map uaaYamlMap, List entryInUaaYaml) { + return entryInUaaYaml.stream().filter(e -> uaaYamlMap.get(e) != null).findFirst().map(uaaYamlMap::get).orElse(null); + } + public static IdentityProviderWrapper getIdentityProviderWrapper(String origin, AbstractExternalOAuthIdentityProviderDefinition rawDef, IdentityProvider provider, boolean override) { provider.setOriginKey(origin); provider.setName("UAA Oauth Identity Provider["+provider.getOriginKey()+"]"); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIdentityProviderDefinitionFactoryBeanTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIdentityProviderDefinitionFactoryBeanTest.java index b0ed32d74d1..559433b59bd 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIdentityProviderDefinitionFactoryBeanTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIdentityProviderDefinitionFactoryBeanTest.java @@ -30,6 +30,7 @@ import static org.cloudfoundry.identity.uaa.util.UaaMapUtils.map; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -120,7 +121,7 @@ public void external_group_mapping_default_in_body() { @Test public void jwtClientAuthenticationTrue() { Map definitions = new HashMap<>(); - idpDefinitionMap.put("jwtclientAuthentication", new Boolean(true)); + idpDefinitionMap.put("jwtclientAuthentication", Boolean.valueOf (true)); idpDefinitionMap.put("type", OriginKeys.OIDC10); definitions.put("test", idpDefinitionMap); factoryBean = new OauthIDPWrapperFactoryBean(definitions); @@ -143,7 +144,7 @@ public void jwtClientAuthenticationNull() { @Test public void jwtClientAuthenticationInvalidType() { Map definitions = new HashMap<>(); - idpDefinitionMap.put("jwtclientAuthentication", new Integer(1)); + idpDefinitionMap.put("jwtclientAuthentication", Integer.valueOf(1)); idpDefinitionMap.put("type", OriginKeys.OIDC10); definitions.put("test", idpDefinitionMap); factoryBean = new OauthIDPWrapperFactoryBean(definitions); @@ -167,6 +168,24 @@ public void jwtClientAuthenticationWithCustomSetting() { assertEquals("issuer", (((Map)((OIDCIdentityProviderDefinition) factoryBean.getProviders().get(0).getProvider().getConfig()).getJwtClientAuthentication()).get("iss"))); } + @Test + public void jwtClientAuthenticationWith2EntriesButNewOneMustWin() { + // given: 2 similar entry because of issue #2752 + idpDefinitionMap.put("jwtclientAuthentication", Map.of("iss", "issuer")); + idpDefinitionMap.put("jwtClientAuthentication", Map.of("iss", "trueIssuer")); + idpDefinitionMap.put("type", OriginKeys.OIDC10); + Map definitions = new HashMap<>(); + definitions.put("test", idpDefinitionMap); + // when: load beans from uaa.yml + factoryBean = new OauthIDPWrapperFactoryBean(definitions); + factoryBean.setCommonProperties(idpDefinitionMap, providerDefinition); + // then + assertTrue(factoryBean.getProviders().get(0).getProvider().getConfig() instanceof OIDCIdentityProviderDefinition); + assertNotNull(((OIDCIdentityProviderDefinition) factoryBean.getProviders().get(0).getProvider().getConfig()).getJwtClientAuthentication()); + assertNotEquals("issuer", (((Map)((OIDCIdentityProviderDefinition) factoryBean.getProviders().get(0).getProvider().getConfig()).getJwtClientAuthentication()).get("iss"))); + assertEquals("trueIssuer", (((Map)((OIDCIdentityProviderDefinition) factoryBean.getProviders().get(0).getProvider().getConfig()).getJwtClientAuthentication()).get("iss"))); + } + @Test public void testNoDiscoveryUrl() { Map definitions = new HashMap<>(); From d0cf81f75753f2a2f83e51e347899b24e16f1599 Mon Sep 17 00:00:00 2001 From: Adrian Hoelzl Date: Fri, 8 Mar 2024 11:04:56 +0100 Subject: [PATCH 39/42] Rework --- .../UserIdConversionEndpointsTests.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java index af8f00ac83a..61cf3d8d227 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/UserIdConversionEndpointsTests.java @@ -247,18 +247,20 @@ private void assertEndpointReturnsCorrectResult( final boolean lastPageIncomplete = expectedUsers.size() % resultsPerPage != 0; final int expectedPages = expectedUsers.size() / resultsPerPage + (lastPageIncomplete ? 1 : 0); - final Function> fetchNextPage = (startIndex) -> endpoints.findUsers( - filter, "ascending", startIndex, resultsPerPage, includeInactive - ); + final Function>> fetchNextPage = (startIndex) -> { + final ResponseEntity response = endpoints.findUsers( + filter, "ascending", startIndex, resultsPerPage, includeInactive + ); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); + assertThat(response.getBody()).isNotNull().isInstanceOf(SearchResults.class); + return (SearchResults>) response.getBody(); + }; // collect all users in several pages final List> observedUsers = new ArrayList<>(); int currentStartIndex = 1; for (int i = 0; i < expectedPages; i++) { - final ResponseEntity response = fetchNextPage.apply(currentStartIndex); - assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); - assertThat(response.getBody()).isNotNull().isInstanceOf(SearchResults.class); - final SearchResults> responseBody = (SearchResults>) response.getBody(); + final SearchResults> responseBody = fetchNextPage.apply(currentStartIndex); assertThat(responseBody.getTotalResults()).isEqualTo(expectedUsers.size()); final int expectedNumberOfResultsInPage; @@ -276,10 +278,7 @@ private void assertEndpointReturnsCorrectResult( } // check next page -> should be empty - final ResponseEntity response = fetchNextPage.apply(currentStartIndex); - assertThat(response.getBody()).isNotNull(); - assertThat(response.getBody()).isNotNull().isInstanceOf(SearchResults.class); - final SearchResults> responseBody = (SearchResults>) response.getBody(); + final SearchResults> responseBody = fetchNextPage.apply(currentStartIndex);; assertThat(responseBody.getTotalResults()).isEqualTo(expectedUsers.size()); assertThat(responseBody.getResources()).isNotNull().isEmpty(); From 6efdd010b55402d665632baf8096ccae498c5a9e Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sat, 9 Mar 2024 13:38:19 +0100 Subject: [PATCH 40/42] build(deps): bump org.eclipse.jgit:org.eclipse.jgit (#2772) Bumps org.eclipse.jgit:org.eclipse.jgit from 6.8.0.202311291450-r to 6.9.0.202403050737-r. --- updated-dependencies: - dependency-name: org.eclipse.jgit:org.eclipse.jgit dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dependencies.gradle b/dependencies.gradle index b0dac4e28ea..a22745c7fe4 100644 --- a/dependencies.gradle +++ b/dependencies.gradle @@ -50,7 +50,7 @@ libraries.braveInstrumentationSpringWebmvc = "io.zipkin.brave:brave-instrumentat libraries.braveContextSlf4j = "io.zipkin.brave:brave-context-slf4j:${versions.braveVersion}" libraries.commonsIo = "commons-io:commons-io:2.15.1" libraries.dumbster = "dumbster:dumbster:1.6" -libraries.eclipseJgit = "org.eclipse.jgit:org.eclipse.jgit:6.8.0.202311291450-r" +libraries.eclipseJgit = "org.eclipse.jgit:org.eclipse.jgit:6.9.0.202403050737-r" libraries.flywayCore = "org.flywaydb:flyway-core" libraries.greenmail = "com.icegreen:greenmail:1.6.15" libraries.guava = "com.google.guava:guava:${versions.guavaVersion}" From b8826e1456b98ae179b85cab9e5261a64edc2d25 Mon Sep 17 00:00:00 2001 From: Markus Strehle <11627201+strehle@users.noreply.github.com> Date: Sat, 9 Mar 2024 17:01:13 +0100 Subject: [PATCH 41/42] Update rack in Gemfile.lock (#2773) --- uaa/slate/Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/uaa/slate/Gemfile.lock b/uaa/slate/Gemfile.lock index 18da0c5aa1d..b69cc8a2369 100644 --- a/uaa/slate/Gemfile.lock +++ b/uaa/slate/Gemfile.lock @@ -97,7 +97,7 @@ GEM parslet (2.0.0) public_suffix (5.0.4) racc (1.7.3) - rack (2.2.8) + rack (2.2.8.1) rb-fsevent (0.11.2) rb-inotify (0.10.1) ffi (~> 1.0) From 500228c40e4b64770751931728c08cbab65ddfdc Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 11 Mar 2024 12:32:25 +0100 Subject: [PATCH 42/42] build(deps): bump versions.jacksonVersion from 2.16.1 to 2.16.2 (#2774) Bumps `versions.jacksonVersion` from 2.16.1 to 2.16.2. Updates `com.fasterxml.jackson.core:jackson-annotations` from 2.16.1 to 2.16.2 - [Commits](https://github.com/FasterXML/jackson/commits) Updates `com.fasterxml.jackson.core:jackson-databind` from 2.16.1 to 2.16.2 - [Commits](https://github.com/FasterXML/jackson/commits) Updates `com.fasterxml.jackson.dataformat:jackson-dataformat-yaml` from 2.16.1 to 2.16.2 - [Commits](https://github.com/FasterXML/jackson-dataformats-text/compare/jackson-dataformats-text-2.16.1...jackson-dataformats-text-2.16.2) --- updated-dependencies: - dependency-name: com.fasterxml.jackson.core:jackson-annotations dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: com.fasterxml.jackson.core:jackson-databind dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: com.fasterxml.jackson.dataformat:jackson-dataformat-yaml dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dependencies.gradle b/dependencies.gradle index a22745c7fe4..40bfabe796b 100644 --- a/dependencies.gradle +++ b/dependencies.gradle @@ -17,7 +17,7 @@ versions.tomcatCargoVersion = "9.0.86" versions.guavaVersion = "33.0.0-jre" versions.seleniumVersion = "4.18.1" versions.braveVersion = "6.0.2" -versions.jacksonVersion = "2.16.1" +versions.jacksonVersion = "2.16.2" versions.jsonPathVersion = "2.9.0" // Versions we're overriding from the Spring Boot Bom (Dependabot does not issue PRs to bump these versions, so we need to manually bump them)