PR for #4759 - Updating IDP user info spam in the logs

stroom-core-shared/src/main/java/stroom/security/shared/User.java

@@ -385,6 +385,11 @@ public Builder displayName(final String displayName) {
             return this;
         }
 
+        public Builder fullName(final String fullName) {
+            this.fullName = fullName;
+            return this;
+        }
+
         /**
          * If value is true marks this {@link User} as a named user-group.
          */

stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/JwtUtil.java

@@ -90,7 +90,7 @@ public static String getUniqueIdentity(final OpenIdConfiguration openIdConfigura
         final String id = JwtUtil.getClaimValue(jwtClaims, uniqueIdentityClaim)
                 .orElseThrow(() -> new RuntimeException(LogUtil.message(
                         "Expecting claims to contain configured uniqueIdentityClaim '{}' " +
-                                "but it is not there, jwtClaims: {}",
+                        "but it is not there, jwtClaims: {}",
                         uniqueIdentityClaim,
                         jwtClaims)));
 
@@ -104,7 +104,7 @@ public static String getUniqueIdentity(final OpenIdConfiguration openIdConfigura
      * Maps to the 'name' column in stroom_user table.
      */
     public static Optional<String> getUserDisplayName(final OpenIdConfiguration openIdConfiguration,
-                                                  final JwtClaims jwtClaims) {
+                                                      final JwtClaims jwtClaims) {
         Objects.requireNonNull(openIdConfiguration);
         Objects.requireNonNull(jwtClaims);
         final String userDisplayNameClaim = openIdConfiguration.getUserDisplayNameClaim();

stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/StroomUserIdentityFactory.java

@@ -159,7 +159,7 @@ protected Optional<UserIdentity> mapAuthFlowIdentity(final JwtContext jwtContext
 
         return optUser
                 .flatMap(user -> {
-                    final User effectiveUser = updateUserInfo(user, jwtClaims);
+                    final User effectiveUser = updateUserInfo(subjectId, user, jwtClaims);
                     final UserIdentity userIdentity = createAuthFlowUserIdentity(
                             jwtClaims, request, tokenResponse, effectiveUser);
                     return Optional.of(userIdentity);
@@ -194,10 +194,15 @@ public void logoutUser(final UserIdentity userIdentity) {
      * them logging in though.
      * Each time we map their identity we check the cached info is up-to-date and if so update it.
      */
-    private User updateUserInfo(final User user, final JwtClaims jwtClaims) {
+    private User updateUserInfo(final String subjectId, final User user, final JwtClaims jwtClaims) {
         AtomicReference<User> userRef = new AtomicReference<>(user);
+
+        // We must default the displayName in the same way as happens when the DB record is created
+        // (in stroom.security.impl.UserServiceImpl.getOrCreateUser)
+        // else it will always detect a mismatch.
         final String displayName = JwtUtil.getUserDisplayName(openIdConfigProvider.get(), jwtClaims)
-                .orElse(null);
+                .filter(str -> !str.isBlank())
+                .orElse(subjectId);
 
         // Hopefully this one is enough of a standard to always be there.
         final String fullName = JwtUtil.getClaimValue(jwtClaims, OpenId.CLAIM__NAME)
@@ -325,8 +330,11 @@ private UserIdentity createAuthFlowUserIdentity(final JwtClaims jwtClaims,
         return userIdentity;
     }
 
-    private Optional<UserIdentity> getApiUserIdentity(final JwtContext jwtContext,
-                                                      final HttpServletRequest request) {
+    /**
+     * Pkg private for testing
+     */
+    Optional<UserIdentity> getApiUserIdentity(final JwtContext jwtContext,
+                                              final HttpServletRequest request) {
         LOGGER.debug(() -> "Getting API user identity for uri: " + request.getRequestURI());
 
         try {
@@ -348,7 +356,7 @@ private Optional<UserIdentity> getApiUserIdentity(final JwtContext jwtContext,
                 User user = getOrCreateUserBySubjectId(subjectId).orElseThrow(() ->
                         new AuthenticationException("Unable to find user with id: " + subjectId
                                                     + "(displayName: " + optDisplayName + ")"));
-                user = updateUserInfo(user, jwtClaims);
+                user = updateUserInfo(subjectId, user, jwtClaims);
                 userUuid = user.getUuid();
             }
 

stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/UserServiceImpl.java

@@ -99,11 +99,10 @@ public User getOrCreateUser(final UserDesc userDesc, final Consumer<User> onCrea
         return optional.orElseGet(() -> {
             final User user = new User();
             AuditUtil.stamp(securityContext, user);
-            user.setSubjectId(userDesc.getSubjectId());
+            final String subjectId = userDesc.getSubjectId();
+            user.setSubjectId(subjectId);
             // Make sure we set a display name even if it is the same as the subject id.
-            user.setDisplayName(NullSafe.isBlankString(userDesc.getDisplayName())
-                    ? userDesc.getSubjectId()
-                    : userDesc.getDisplayName());
+            user.setDisplayName(NullSafe.nonBlankStringElse(userDesc.getDisplayName(), subjectId));
             user.setFullName(userDesc.getFullName());
             user.setGroup(false);
             user.setEnabled(true);

stroom-security/stroom-security-impl/src/test/java/stroom/security/impl/TestStroomUserIdentityFactory.java

@@ -0,0 +1,90 @@
+package stroom.security.impl;
+
+import stroom.cache.impl.CacheManagerImpl;
+import stroom.security.mock.MockSecurityContext;
+import stroom.security.openid.api.IdpType;
+import stroom.security.openid.api.OpenId;
+import stroom.security.openid.api.OpenIdConfiguration;
+import stroom.security.shared.User;
+import stroom.util.entityevent.EntityEventBus;
+import stroom.util.logging.LogUtil;
+
+import jakarta.servlet.http.HttpServletRequest;
+import org.jose4j.jwt.JwtClaims;
+import org.jose4j.jwt.MalformedClaimException;
+import org.jose4j.jwt.consumer.JwtContext;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class TestStroomUserIdentityFactory {
+
+    public static final String USER_123_SUBJECT = "user123";
+    @Mock
+    private UserService mockUserService;
+    @Mock
+    private OpenIdConfiguration mockOpenIdConfiguration;
+    @Mock
+    private EntityEventBus mockEntityEventBus;
+    @Mock
+    private JwtClaims mockJwtClaims;
+    @Mock
+    private JwtContext mockJwtContext;
+    @Mock
+    private HttpServletRequest mockHttpServletRequest;
+
+    @Test
+    void mapApiIdentity() throws MalformedClaimException {
+        final String displayNameClaim = "disp";
+        final User user = User.builder()
+                .subjectId(USER_123_SUBJECT)
+                .displayName(USER_123_SUBJECT)
+                .fullName(null)
+                .build();
+
+        Mockito.when(mockOpenIdConfiguration.getIdentityProviderType())
+                .thenReturn(IdpType.EXTERNAL_IDP);
+        Mockito.when(mockOpenIdConfiguration.getUniqueIdentityClaim())
+                .thenReturn(OpenId.CLAIM__SUBJECT);
+        Mockito.when(mockOpenIdConfiguration.getUserDisplayNameClaim())
+                .thenReturn(displayNameClaim);
+        Mockito.when(mockJwtClaims.getClaimValue(Mockito.anyString(), Mockito.any()))
+                .thenAnswer(invocation -> {
+                    final String claim = invocation.getArgument(0, String.class);
+                    return switch (claim) {
+                        case OpenId.CLAIM__SUBJECT -> USER_123_SUBJECT;
+                        case OpenId.CLAIM__NAME -> null;
+                        case displayNameClaim -> null;
+                        default -> throw new IllegalArgumentException(LogUtil.message("'{}' not expected", claim));
+                    };
+                });
+        Mockito.when(mockJwtContext.getJwtClaims())
+                .thenReturn(mockJwtClaims);
+        Mockito.when(mockUserService.getOrCreateUser(Mockito.eq(USER_123_SUBJECT)))
+                .thenReturn(user);
+
+        final StroomUserIdentityFactory stroomUserIdentityFactory = new StroomUserIdentityFactory(
+                null,
+                () -> mockOpenIdConfiguration,
+                null,
+                null,
+                null,
+                null,
+                () -> mockUserService,
+                new MockSecurityContext(),
+                null,
+                mockEntityEventBus,
+                null,
+                null,
+                AuthorisationConfig::new,
+                new CacheManagerImpl());
+
+        stroomUserIdentityFactory.getApiUserIdentity(mockJwtContext, mockHttpServletRequest);
+
+        Mockito.verify(mockUserService, Mockito.never())
+                .update(Mockito.any());
+    }
+}

stroom-security/stroom-security-impl/src/test/java/stroom/security/impl/TestUserServiceImpl.java

@@ -0,0 +1,92 @@
+package stroom.security.impl;
+
+import stroom.security.mock.MockSecurityContext;
+import stroom.security.shared.User;
+import stroom.util.shared.UserDesc;
+
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import java.util.concurrent.atomic.AtomicReference;
+
+@ExtendWith(MockitoExtension.class)
+class TestUserServiceImpl {
+
+    public static final String SUBJECT_ID = "user123";
+    public static final String DISPLAY_NAME = "jbloggs";
+    public static final String FULL_NAME = "Joe Bloggs";
+    @Mock
+    private UserDao mockUserDao;
+
+    @Test
+    void testGetOrCreateUser_new() {
+        final UserServiceImpl userService = new UserServiceImpl(
+                new MockSecurityContext(),
+                mockUserDao,
+                null,
+                null,
+                null,
+                null,
+                null,
+                null,
+                null);
+
+        final UserDesc userDesc = UserDesc.builder(SUBJECT_ID)
+                .build();
+
+        final AtomicReference<User> onCreateConsumer = new AtomicReference<>();
+
+        Mockito.when(mockUserDao.tryCreate(Mockito.any(User.class), Mockito.any()))
+                .thenAnswer(invocation -> {
+                    return invocation.getArgument(0);
+                });
+
+        final User user = userService.getOrCreateUser(userDesc, onCreateConsumer::set);
+
+        Assertions.assertThat(user.getSubjectId())
+                .isEqualTo(SUBJECT_ID);
+        Assertions.assertThat(user.getDisplayName())
+                .isEqualTo(SUBJECT_ID);  // No display name so use sub
+        Assertions.assertThat(user.getFullName())
+                .isNull();
+    }
+
+    @Test
+    void testGetOrCreateUser_new2() {
+        final UserServiceImpl userService = new UserServiceImpl(
+                new MockSecurityContext(),
+                mockUserDao,
+                null,
+                null,
+                null,
+                null,
+                null,
+                null,
+                null);
+
+        final UserDesc userDesc = UserDesc.builder(SUBJECT_ID)
+                .displayName(DISPLAY_NAME)
+                .fullName(FULL_NAME)
+                .build();
+
+        final AtomicReference<User> onCreateConsumer = new AtomicReference<>();
+
+        Mockito.when(mockUserDao.tryCreate(Mockito.any(User.class), Mockito.any()))
+                .thenAnswer(invocation -> {
+                    return invocation.getArgument(0);
+                });
+
+        final User user = userService.getOrCreateUser(userDesc, onCreateConsumer::set);
+
+        Assertions.assertThat(user.getSubjectId())
+                .isEqualTo(SUBJECT_ID);
+        Assertions.assertThat(user.getDisplayName())
+                .isEqualTo(DISPLAY_NAME);  // No display name so use sub
+        Assertions.assertThat(user.getFullName())
+                .isEqualTo(FULL_NAME);
+    }
+}

unreleased_changes/20250204_162439_448__4759.md

@@ -0,0 +1,24 @@
+* Issue **#4759** : Fix `Updating IDP user info...` appearing repeatedly in the logs.
+
+
+```sh
+# ********************************************************************************
+# Issue title: `Updating IDP user info` spam in the logs
+# Issue link:  https://github.com/gchq/stroom/issues/4759
+# ********************************************************************************
+
+# ONLY the top line will be included as a change entry in the CHANGELOG.
+# The entry should be in GitHub flavour markdown and should be written on a SINGLE
+# line with no hard breaks. You can have multiple change files for a single GitHub issue.
+# The  entry should be written in the imperative mood, i.e. 'Fix nasty bug' rather than
+# 'Fixed nasty bug'.
+#
+# Examples of acceptable entries are:
+#
+#
+# * Issue **123** : Fix bug with an associated GitHub issue in this repository
+#
+# * Issue **namespace/other-repo#456** : Fix bug with an associated GitHub issue in another repository
+#
+# * Fix bug with no associated GitHub issue.
+```