From 5a406cf274846d9db5cac646832e768cf2d91033 Mon Sep 17 00:00:00 2001 From: Okke Harsta Date: Thu, 31 Oct 2024 15:42:28 +0100 Subject: [PATCH] Fixes #535 --- .../java/myconext/api/UserController.java | 6 -- .../src/main/java/myconext/model/User.java | 60 +++++++++++++---- .../GuestIdpAuthenticationRequestFilter.java | 11 ++-- .../security/SecurityConfiguration.java | 4 +- .../src/main/resources/application.yml | 3 +- .../java/myconext/api/UserControllerTest.java | 6 +- .../test/java/myconext/model/UserTest.java | 66 ++++++++++++++++++- 7 files changed, 123 insertions(+), 33 deletions(-) diff --git a/myconext-server/src/main/java/myconext/api/UserController.java b/myconext-server/src/main/java/myconext/api/UserController.java index efa4ba5a..e5692589 100644 --- a/myconext-server/src/main/java/myconext/api/UserController.java +++ b/myconext-server/src/main/java/myconext/api/UserController.java @@ -100,7 +100,6 @@ public class UserController implements UserAuthentication { private final List issuers; //For now, hardcode the not known issuers from test private final List unknownIssuers = List.of("CURRNL2A"); - private final int nudgeAppDays; public UserController(UserRepository userRepository, UserCredentialRepository userCredentialRepository, @@ -124,7 +123,6 @@ public UserController(UserRepository userRepository, @Value("${rp_id}") String rpId, @Value("${feature.default_remember_me}") boolean featureDefaultRememberMe, @Value("${verify.issuers_path}") Resource issuersResource, - @Value("${nudge_eduid_app_days}") int nudgeAppDays, ServicesConfiguration servicesConfiguration) throws IOException { this.userRepository = userRepository; this.userCredentialRepository = userCredentialRepository; @@ -148,7 +146,6 @@ public UserController(UserRepository userRepository, this.relyingParty = relyingParty(rpId, rpOrigin); this.emailGuessingPreventor = new EmailGuessingPrevention(emailGuessingSleepMillis); this.featureDefaultRememberMe = featureDefaultRememberMe; - this.nudgeAppDays = nudgeAppDays; List idinIssuers = objectMapper.readValue(issuersResource.getInputStream(), new TypeReference<>() { }); @@ -372,9 +369,6 @@ public ResponseEntity createEduIDAccount(@Valid @RequestBody Cre createAccount.getRelyingPartClientId(), manage); user.setCreateFromInstitutionKey(institution.getHash()); - //Don't bother with the nudge app the next 24 hours - long nudgeAppMillis = System.currentTimeMillis() - (1000L * 60 * 60 * 24 * (nudgeAppDays - 1)); - user.setLastSeenAppNudge(nudgeAppMillis); user.validate(); userRepository.save(user); diff --git a/myconext-server/src/main/java/myconext/model/User.java b/myconext-server/src/main/java/myconext/model/User.java index 99b1f665..a7e83020 100644 --- a/myconext-server/src/main/java/myconext/model/User.java +++ b/myconext-server/src/main/java/myconext/model/User.java @@ -26,6 +26,7 @@ import org.springframework.util.StringUtils; import java.io.Serializable; +import java.time.Clock; import java.util.*; import java.util.stream.Collectors; @@ -83,11 +84,10 @@ public class User implements Serializable, UserDetails { private List eduIDS = new ArrayList<>(); private long created; - private long updatedAt; + private long lastLogin; @Setter @Indexed private String trackingUuid; - @Setter private long lastSeenAppNudge; @Transient @JsonIgnore @@ -109,7 +109,7 @@ public User(String uid, String email, String chosenName, String givenName, Strin this.chosenName = chosenName; this.givenName = givenName; this.familyName = familyName; - this.schacHomeOrganization = StringUtils.hasText(schacHomeOrganization) ? schacHomeOrganization.toLowerCase() : schacHomeOrganization ; + this.schacHomeOrganization = StringUtils.hasText(schacHomeOrganization) ? schacHomeOrganization.toLowerCase() : schacHomeOrganization; this.preferredLanguage = preferredLanguage; if (StringUtils.hasText(serviceProviderEntityId)) { this.computeEduIdForServiceProviderIfAbsent(serviceProviderEntityId, manage); @@ -185,7 +185,7 @@ public String computeEduIdForIdentityProviderProviderIfAbsent(RemoteProvider rem } private String doComputeEduIDIfAbsent(ServiceProvider serviceProvider, Manage manage) { - this.updatedAt = System.currentTimeMillis(); + this.lastLogin = System.currentTimeMillis(); serviceProvider.setLastLogin(new Date()); String institutionGuid = serviceProvider.getInstitutionGuid(); String entityId = serviceProvider.getEntityId(); @@ -195,11 +195,11 @@ private String doComputeEduIDIfAbsent(ServiceProvider serviceProvider, Manage ma //Ensure we don't match institutionGUID's that are both null boolean matchByInstitutionGUID = StringUtils.hasText(institutionGuid) && (institutionGuid.equals(eduID.getServiceInstutionGuid()) || - eduID.getServices().stream().anyMatch(sp -> institutionGuid.equals(sp.getInstitutionGuid()))); + eduID.getServices().stream().anyMatch(sp -> institutionGuid.equals(sp.getInstitutionGuid()))); //Ensure we don't match entityId's that are both null boolean matchByEntityId = StringUtils.hasText(entityId) && (entityId.equals(eduID.getServiceProviderEntityId()) || - eduID.getServices().stream().anyMatch(sp -> entityId.equals(sp.getEntityId()))); + eduID.getServices().stream().anyMatch(sp -> entityId.equals(sp.getEntityId()))); return matchByInstitutionGUID || matchByEntityId; }).findFirst(); //If there is an existing eduID then we add or update the service for this eduID, otherwise add new one @@ -322,16 +322,16 @@ public Map convertEduIdPerServiceProvider(ServicesConfiguration s } else { eduID.getServices().stream().filter(service -> !hideInOverview.contains(service.getEntityId())) .forEach(service -> { - String entityId = service.getEntityId(); - String key = StringUtils.hasText(entityId) ? entityId : service.getInstitutionGuid(); - //need to make copy otherwise the reference is the same and for mobile authentication we override the properties - result.put(key, eduID.copy(key )); - }); + String entityId = service.getEntityId(); + String key = StringUtils.hasText(entityId) ? entityId : service.getInstitutionGuid(); + //need to make copy otherwise the reference is the same and for mobile authentication we override the properties + result.put(key, eduID.copy(key)); + }); } }); //The mobile API expects the old format. For all keys we fill the obsolete attributes of an eduID if (this.isMobileAuthentication()) { - result.forEach((entityId, eduID)-> { + result.forEach((entityId, eduID) -> { if (!CollectionUtils.isEmpty(eduID.getServices())) { ServiceProvider serviceProvider = eduID.getServices().stream() .filter(sp -> entityId.equals(sp.getEntityId())).findFirst() @@ -385,4 +385,38 @@ public boolean reconcileLinkedAccounts() { } return false; } -} + + //Prevent @Getter lombok generation as we don't want any logic on this value anywhere else but here + private long getLastSeenAppNudge() { + return this.lastSeenAppNudge; + } + + @Transient + @JsonIgnore + public boolean nudgeToApp(int nudgeAppDays, int nudgeAppDelayDays) { + int oneDayMillis = 1000 * 60 * 60 * 24; + long nowMillis = System.currentTimeMillis(); + //this.created are seconds and not millis, will not change this for backward compatibility in Mobile GUI + long createdMillis = this.created * 1000; + long nudgeAppMillis = (long) nudgeAppDays * oneDayMillis; + + // First 24 hours we don't show the app nudge + boolean createdMoreThenOneDayAgo = createdMillis < (nowMillis - oneDayMillis); + // First subsequent login is before nudgeAppDays + boolean secondLoginBeforeNudgeAppDays = this.lastSeenAppNudge == 0L && + (nowMillis - createdMillis) < nudgeAppMillis; + // Nudge app delay time has reached + boolean delayReached = this.lastSeenAppNudge != 0L && + (nowMillis - this.lastSeenAppNudge) > ((long) oneDayMillis * nudgeAppDelayDays); + + // Corner case, user has postponed the second login for long time, but is now an active user + boolean cornerCase = createdMoreThenOneDayAgo && this.lastSeenAppNudge == 0L + && (nowMillis - this.lastLogin) < nudgeAppMillis; + + boolean decision = createdMoreThenOneDayAgo && (secondLoginBeforeNudgeAppDays || delayReached || cornerCase); + if (decision) { + this.lastSeenAppNudge = nowMillis; + } + return decision; + } +} \ No newline at end of file diff --git a/myconext-server/src/main/java/myconext/security/GuestIdpAuthenticationRequestFilter.java b/myconext-server/src/main/java/myconext/security/GuestIdpAuthenticationRequestFilter.java index af7c035c..6788d2d7 100644 --- a/myconext-server/src/main/java/myconext/security/GuestIdpAuthenticationRequestFilter.java +++ b/myconext-server/src/main/java/myconext/security/GuestIdpAuthenticationRequestFilter.java @@ -93,6 +93,7 @@ public class GuestIdpAuthenticationRequestFilter extends OncePerRequestFilter { private Manage manage; private final ExecutorService executor; private final int nudgeAppDays; + private final int nudgeAppDelayDays; private final int rememberMeQuestionAskedDays; private final long expiryNonValidatedDurationDays; private final long ssoMFADurationSeconds; @@ -109,6 +110,7 @@ public GuestIdpAuthenticationRequestFilter(String redirectUrl, GeoLocation geoLocation, int rememberMeMaxAge, int nudgeAppDays, + int nudgeAppDelayDays, int rememberMeQuestionAskedDays, boolean secureCookie, String magicLinkUrl, @@ -134,6 +136,7 @@ public GuestIdpAuthenticationRequestFilter(String redirectUrl, this.accountLinkingContextClassReferences = ACR.allAccountLinkingContextClassReferences(); this.rememberMeMaxAge = rememberMeMaxAge; this.nudgeAppDays = nudgeAppDays; + this.nudgeAppDelayDays = nudgeAppDelayDays; this.rememberMeQuestionAskedDays = rememberMeQuestionAskedDays; this.secureCookie = secureCookie; this.magicLinkUrl = magicLinkUrl; @@ -458,9 +461,6 @@ private boolean checkStepUp(HttpServletResponse response, HttpServletRequest req !hasValidatedName(user); if (user.isNewUser()) { user.setNewUser(false); - //ensure the user is the coming 24 hours is not nudged to the app - long nudgeAppMillis = System.currentTimeMillis() - (1000L * 60 * 60 * 24 * (nudgeAppDays - 1)); - user.setLastSeenAppNudge(nudgeAppMillis); userRepository.save(user); logWithContext(user, "add", "account", LOG, "Saving user after new registration and magic link"); @@ -495,11 +495,10 @@ private boolean checkStepUp(HttpServletResponse response, HttpServletRequest req return false; } else if (!samlAuthenticationRequest.isPasswordOrWebAuthnFlow() && !samlAuthenticationRequest.isTiqrFlow() && !user.loginOptions().contains(LoginOptions.APP.getValue()) && - user.getLastSeenAppNudge() < (System.currentTimeMillis() - (1000L * 60 * 60 * 24 * nudgeAppDays))) { - //Nudge user to use the app - user.setLastSeenAppNudge(System.currentTimeMillis()); + user.nudgeToApp(nudgeAppDays, nudgeAppDelayDays)) { userRepository.save(user); + //Nudge user to use the app String url = this.redirectUrl + "/confirm?h=" + hash + "&redirect=" + URLEncoder.encode(this.magicLinkUrl, charSet) + "&new=false"; diff --git a/myconext-server/src/main/java/myconext/security/SecurityConfiguration.java b/myconext-server/src/main/java/myconext/security/SecurityConfiguration.java index f7933fe4..42a0eae4 100644 --- a/myconext-server/src/main/java/myconext/security/SecurityConfiguration.java +++ b/myconext-server/src/main/java/myconext/security/SecurityConfiguration.java @@ -69,7 +69,8 @@ public SamlSecurity(@Value("${private_key_path}") Resource privateKeyPath, @Value("${saml_metadata_base_path}") String samlMetadataBasePath, @Value("${idp_redirect_url}") String redirectUrl, @Value("${remember_me_max_age_seconds}") int rememberMeMaxAge, - @Value("${nudge_eduid_app_days}") int nudgeAppDays, + @Value("${nudge_eduid_app_login_days}") int nudgeAppDays, + @Value("${nudge_eduid_app_pause_days}") int nudgeAppDelayDays, @Value("${remember_me_question_asked_days}") int rememberMeQuestionAskedDays, @Value("${secure_cookie}") boolean secureCookie, @Value("${email.magic-link-url}") String magicLinkUrl, @@ -121,6 +122,7 @@ public SamlSecurity(@Value("${private_key_path}") Resource privateKeyPath, geoLocation, rememberMeMaxAge, nudgeAppDays, + nudgeAppDelayDays, rememberMeQuestionAskedDays, secureCookie, magicLinkUrl, diff --git a/myconext-server/src/main/resources/application.yml b/myconext-server/src/main/resources/application.yml index 87cf068d..ab902fe6 100644 --- a/myconext-server/src/main/resources/application.yml +++ b/myconext-server/src/main/resources/application.yml @@ -106,7 +106,8 @@ tiqr_hash_secret: 43234502-2AAC-4E53-AA32-C7B909F71442 remember_me_max_age_seconds: 15_768_000 sso_mfa_duration_seconds: 900 -nudge_eduid_app_days: 7 +nudge_eduid_app_login_days: 5 +nudge_eduid_app_pause_days: 7 remember_me_question_asked_days: 30 email_guessing_sleep_millis: 500 email_spam_threshold_seconds: 15 diff --git a/myconext-server/src/test/java/myconext/api/UserControllerTest.java b/myconext-server/src/test/java/myconext/api/UserControllerTest.java index c0615886..92b47f09 100644 --- a/myconext-server/src/test/java/myconext/api/UserControllerTest.java +++ b/myconext-server/src/test/java/myconext/api/UserControllerTest.java @@ -99,16 +99,14 @@ public void newUserProvisioned() throws IOException { MagicLinkResponse magicLinkResponse = magicLinkRequest(user, HttpMethod.POST); User userFromDB = userRepository.findUserByEmail(user.getEmail()).get(); - assertEquals(0L, userFromDB.getLastSeenAppNudge()); + assertEquals(0L, ReflectionTestUtils.getField(userFromDB, "lastSeenAppNudge")); assertEquals(user.getGivenName(), userFromDB.getGivenName()); String samlResponse = samlResponse(magicLinkResponse); assertTrue(samlResponse.contains("new@example.com")); User userFromAfter = userRepository.findUserByEmail(user.getEmail()).get(); - long appNudgeDiff = System.currentTimeMillis() - userFromAfter.getLastSeenAppNudge(); - int days = (int) Math.floor((double) appNudgeDiff / (1000 * 60 * 60 * 24)); - assertEquals(6, days); + assertEquals(0L, ReflectionTestUtils.getField(userFromAfter, "lastSeenAppNudge")); when() .get("/myconext/api/idp/resend_magic_link_request?id=" + magicLinkResponse.authenticationRequestId) diff --git a/myconext-server/src/test/java/myconext/model/UserTest.java b/myconext-server/src/test/java/myconext/model/UserTest.java index 3422b35a..e6c78a4e 100644 --- a/myconext-server/src/test/java/myconext/model/UserTest.java +++ b/myconext-server/src/test/java/myconext/model/UserTest.java @@ -1,6 +1,7 @@ package myconext.model; import com.fasterxml.jackson.databind.ObjectMapper; +import lombok.SneakyThrows; import myconext.exceptions.WeakPasswordException; import myconext.manage.Manage; import myconext.manage.MockManage; @@ -15,6 +16,7 @@ import java.util.regex.Pattern; import static org.junit.Assert.*; +import static org.mockito.Mockito.mock; public class UserTest { @@ -87,7 +89,7 @@ public void encryptPassword() { @Test public void bugFixForUpdateExistingEduIDAndNotAddNewService() { User user = new User(); - Manage mockManage = Mockito.mock(Manage.class); + Manage mockManage = mock(Manage.class); String entityId = "https://sp_one"; String institutionGuid = UUID.randomUUID().toString(); ServiceProvider serviceProvider = new ServiceProvider(new RemoteProvider( @@ -134,17 +136,20 @@ public void reconcileLinkedAccounts() { User user = user("sp_entity_id"); LinkedAccount linkedAccount = new LinkedAccount(); linkedAccount.setCreatedAt(Date.from(Instant.ofEpochMilli(423439200000L))); + assertFalse(linkedAccount.isPreferred()); LinkedAccount otherLinkedAccount = new LinkedAccount(); otherLinkedAccount.setCreatedAt(Date.from(Instant.ofEpochMilli(1307052000000L))); ReflectionTestUtils.setField(otherLinkedAccount, "givenName", "Pol"); ReflectionTestUtils.setField(otherLinkedAccount, "familyName", "Kropto"); + assertFalse(otherLinkedAccount.isPreferred()); user.getLinkedAccounts().add(linkedAccount); user.getLinkedAccounts().add(otherLinkedAccount); ExternalLinkedAccount externalLinkedAccount = new ExternalLinkedAccount(); externalLinkedAccount.setCreatedAt(Date.from(Instant.ofEpochMilli(807052000000L))); + assertFalse(externalLinkedAccount.isPreferred()); user.getExternalLinkedAccounts().add(externalLinkedAccount); assertTrue(user.reconcileLinkedAccounts()); @@ -158,7 +163,7 @@ public void reconcileLinkedAccounts() { @Test public void hideServicesInEduID() { User user = new User(); - Manage mockManage = Mockito.mock(Manage.class); + Manage mockManage = mock(Manage.class); String entityId = "https://sp_one"; ServiceProvider serviceProvider = new ServiceProvider(new RemoteProvider( entityId, entityId.concat("Name"), entityId.concat("NameNl"), null, "logoURL"), "homeURL"); @@ -173,6 +178,63 @@ public void hideServicesInEduID() { assertEquals(0, eduIdPerServiceProviderFiltered.size()); } + @SneakyThrows + @Test + public void nudgeToAppActiveUser() { + int nudgeAppDays = 4; + int nudgeAppDelayDays = 7; + long nowMillis = System.currentTimeMillis(); + User user = user("SP"); + //User has logged in within 24 hours of creation, do not nudge to app + assertFalse(user.nudgeToApp(nudgeAppDays, nudgeAppDelayDays)); + assertEquals(0L, ReflectionTestUtils.getField(user, "lastSeenAppNudge")); + + //User has logged in after 24 hours of creation and before nudgeAppDays + ReflectionTestUtils.setField(user, "created", Instant.now().minus(3, ChronoUnit.DAYS).toEpochMilli() / 1000); + assertTrue(user.nudgeToApp(nudgeAppDays, nudgeAppDelayDays)); + //lastSeenAppNudge is set to now + long lastSeenAppNudge = (long) ReflectionTestUtils.getField(user, "lastSeenAppNudge"); + assertTrue(lastSeenAppNudge >= nowMillis); + + //User was nudged to app, on login again user is not nudged again + assertFalse(user.nudgeToApp(nudgeAppDays, nudgeAppDelayDays)); + long lastSeenAppNudgeIdempotent = (long) ReflectionTestUtils.getField(user, "lastSeenAppNudge"); + assertEquals(lastSeenAppNudge, lastSeenAppNudgeIdempotent); + + //User logs in again after long period of inactivity and is nudged again to app + ReflectionTestUtils.setField(user, "lastSeenAppNudge", Instant.now().minus(10, ChronoUnit.DAYS).toEpochMilli() / 1000); + //Need to sleep despite the new lastSeenAppNudge, can occur in same millisecond + Thread.sleep(3); + assertTrue(user.nudgeToApp(nudgeAppDays, nudgeAppDelayDays)); + long newAppNudgeIdempotent = (long) ReflectionTestUtils.getField(user, "lastSeenAppNudge"); + assertTrue(newAppNudgeIdempotent > lastSeenAppNudge); + + assertFalse(user.nudgeToApp(nudgeAppDays, nudgeAppDelayDays)); + } + + @Test + public void nudgeToAppLongInactivity() { + int nudgeAppDays = 4; + int nudgeAppDelayDays = 7; + long nowMillis = System.currentTimeMillis(); + User user = user("SP"); + + //User has logged after nudge to app + long sixDaysAgoMillis = Instant.now().minus(6, ChronoUnit.DAYS).toEpochMilli() / 1000; + ReflectionTestUtils.setField(user, "created", sixDaysAgoMillis); + ReflectionTestUtils.setField(user, "lastLogin", sixDaysAgoMillis); + assertFalse(user.nudgeToApp(nudgeAppDays, nudgeAppDelayDays)); + assertEquals(0L, ReflectionTestUtils.getField(user, "lastSeenAppNudge")); + + //User logs in again very shortly after the second login + user.computeEduIdForServiceProviderIfAbsent("SP", mock(Manage.class)); + assertTrue(user.nudgeToApp(nudgeAppDays, nudgeAppDelayDays)); + assertTrue((long) ReflectionTestUtils.getField(user, "lastSeenAppNudge") >= nowMillis); + + //But the user is not spammed with nudges after login now + assertFalse(user.nudgeToApp(nudgeAppDays, nudgeAppDelayDays)); + } + private User user(String serviceProviderEntityId) { return new User("uid", "email", "John", "John", "Doe", "schac", "en", serviceProviderEntityId, manage);