Skip to content

Commit

Permalink
Fixes #535
Browse files Browse the repository at this point in the history
  • Loading branch information
@oharsta
oharsta committed Oct 31, 2024
1 parent 410e6f9 commit 5a406cf
Show file tree
Hide file tree
Showing 7 changed files with 123 additions and 33 deletions.
6 changes: 0 additions & 6 deletions myconext-server/src/main/java/myconext/api/UserController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ public class UserController implements UserAuthentication {
private final List<VerifyIssuer> issuers;
//For now, hardcode the not known issuers from test
private final List<String> unknownIssuers = List.of("CURRNL2A");
private final int nudgeAppDays;

public UserController(UserRepository userRepository,
UserCredentialRepository userCredentialRepository,
Expand All @@ -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;
Expand All @@ -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> idinIssuers = objectMapper.readValue(issuersResource.getInputStream(), new TypeReference<>() {
});
Expand Down Expand Up @@ -372,9 +369,6 @@ public ResponseEntity<StatusResponse> 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);
Expand Down
60 changes: 47 additions & 13 deletions myconext-server/src/main/java/myconext/model/User.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -83,11 +84,10 @@ public class User implements Serializable, UserDetails {
private List<EduID> eduIDS = new ArrayList<>();

private long created;
private long updatedAt;
private long lastLogin;
@Setter
@Indexed
private String trackingUuid;
@Setter
private long lastSeenAppNudge;
@Transient
@JsonIgnore
Expand All @@ -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);
Expand Down Expand Up @@ -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();
Expand All @@ -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
Expand Down Expand Up @@ -322,16 +322,16 @@ public Map<String, EduID> 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()
Expand Down Expand Up @@ -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;
}
}
11 changes: 5 additions & 6 deletions myconext-server/src/main/java/myconext/security/GuestIdpAuthenticationRequestFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -109,6 +110,7 @@ public GuestIdpAuthenticationRequestFilter(String redirectUrl,
GeoLocation geoLocation,
int rememberMeMaxAge,
int nudgeAppDays,
int nudgeAppDelayDays,
int rememberMeQuestionAskedDays,
boolean secureCookie,
String magicLinkUrl,
Expand All @@ -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;
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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";
Expand Down
4 changes: 3 additions & 1 deletion myconext-server/src/main/java/myconext/security/SecurityConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -121,6 +122,7 @@ public SamlSecurity(@Value("${private_key_path}") Resource privateKeyPath,
geoLocation,
rememberMeMaxAge,
nudgeAppDays,
nudgeAppDelayDays,
rememberMeQuestionAskedDays,
secureCookie,
magicLinkUrl,
Expand Down
3 changes: 2 additions & 1 deletion myconext-server/src/main/resources/application.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 2 additions & 4 deletions myconext-server/src/test/java/myconext/api/UserControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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("[email protected]"));

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)
Expand Down
66 changes: 64 additions & 2 deletions myconext-server/src/test/java/myconext/model/UserTest.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -15,6 +16,7 @@
import java.util.regex.Pattern;

import static org.junit.Assert.*;
import static org.mockito.Mockito.mock;

public class UserTest {

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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());
Expand All @@ -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");
Expand All @@ -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);
Expand Down

0 comments on commit 5a406cf

Please sign in to comment.