Fix behaviour

in paricular, when there are less than 5 days (configurable value) left before the AUP expires, the user sees a warning when logging in and can decide whether to skip signing or to re-sign
indigo-iam · Aug 31, 2023 · cc136c4 · cc136c4
1 parent e19cd1d
commit cc136c4
Show file tree

Hide file tree

Showing 7 changed files with 42 additions and 43 deletions.
diff --git a/iam-login-service/src/main/java/it/infn/mw/iam/api/aup/AupSignaturePageController.java b/iam-login-service/src/main/java/it/infn/mw/iam/api/aup/AupSignaturePageController.java
@@ -22,7 +22,6 @@
 
 import java.util.Date;
 import java.util.Optional;
-import java.util.function.Supplier;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -31,10 +30,8 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.security.access.prepost.PreAuthorize;
-import org.springframework.security.web.WebAttributes;
 import org.springframework.security.web.savedrequest.SavedRequest;
 import org.springframework.stereotype.Controller;
-import org.springframework.ui.Model;
 import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RequestMethod;
 import org.springframework.web.servlet.ModelAndView;
@@ -74,16 +71,16 @@ public AupSignaturePageController(IamAupRepository aupRepo,
 
   @PreAuthorize("hasRole('USER')")
   @RequestMapping(value = "/iam/aup/sign", method = { RequestMethod.GET })
-  public ModelAndView signAupPage(HttpSession session) {
+  public ModelAndView signAupPage() {
     ModelAndView view;
 
     Optional<IamAup> aup = repo.findDefaultAup();
+    IamAccount account = accountUtils.getAuthenticatedUserAccount().orElseThrow(
+        () -> new IllegalStateException("No iam account found for authenticated user"));
 
     if (aup.isPresent()) {
-      view = new ModelAndView("iam/signAup");
 
-      IamAccount account = accountUtils.getAuthenticatedUserAccount().orElseThrow(
-          () -> new IllegalStateException("No iam account found for authenticated user"));
+      view = new ModelAndView("iam/signAup");
 
       view.addObject("daysLeftToExpirySignature", service.getRemainingDaysSignatureExpiration(account));
       view.addObject("aup", aup.get());
@@ -136,4 +133,14 @@ public ModelAndView signAup(HttpServletRequest request, HttpServletResponse resp
 
     return new ModelAndView("redirect:/dashboard");
   }
+
+  @PreAuthorize("hasRole('USER')")
+  @RequestMapping(method = RequestMethod.GET, value = "/iam/aup/skip-sign")
+  public ModelAndView skipSignAup(HttpServletRequest request, HttpServletResponse response,
+      HttpSession session) {
+
+    return new ModelAndView("redirect:/dashboard");
+  }
+
+
 }
diff --git a/iam-login-service/src/main/java/it/infn/mw/iam/authn/EnforceAupSignatureSuccessHandler.java b/iam-login-service/src/main/java/it/infn/mw/iam/authn/EnforceAupSignatureSuccessHandler.java
@@ -27,6 +27,7 @@
 import javax.servlet.http.HttpSession;
 
 import org.mitre.openid.connect.web.AuthenticationTimeStamper;
+import org.springframework.beans.factory.annotation.Value;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.oauth2.provider.OAuth2Authentication;
 import org.springframework.security.web.authentication.AuthenticationSuccessHandler;
@@ -40,6 +41,9 @@
 @SuppressWarnings("deprecation")
 public class EnforceAupSignatureSuccessHandler implements AuthenticationSuccessHandler {
 
+  @Value("${iam.aup.advance-notice}")
+  private int EXPIRY_NOTICE_DAYS = 5;
+
   private final AuthenticationSuccessHandler delegate;
   private final AUPSignatureCheckService service;
   private final AccountUtils accountUtils;
@@ -100,7 +104,7 @@ public void onAuthenticationSuccess(HttpServletRequest request, HttpServletRespo
     Optional<IamAccount> authenticatedAccount = lookupAuthenticatedUser(auth);
 
     if (!authenticatedAccount.isPresent()
-        || !(service.getRemainingDaysSignatureExpiration(authenticatedAccount.get()) <= 0)) {
+        || !(service.getRemainingDaysSignatureExpiration(authenticatedAccount.get()) < EXPIRY_NOTICE_DAYS && service.getRemainingDaysSignatureExpiration(authenticatedAccount.get()) != 0)) {
       delegate.onAuthenticationSuccess(request, response, auth);
 
     } else {

diff --git a/iam-login-service/src/main/java/it/infn/mw/iam/core/web/aup/EnforceAupFilter.java b/iam-login-service/src/main/java/it/infn/mw/iam/core/web/aup/EnforceAupFilter.java
@@ -27,12 +27,10 @@
 import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import org.springframework.beans.factory.annotation.Value;
 
 import it.infn.mw.iam.api.account.AccountUtils;
 import it.infn.mw.iam.api.aup.error.AupNotFoundError;
@@ -44,9 +42,6 @@
 
 public class EnforceAupFilter implements Filter {
 
-  @Value("${iam.aup.advance-notice}")
-  private int EXPIRY_NOTICE_DAYS = 30;
-
   public static final Logger LOG = LoggerFactory.getLogger(EnforceAupFilter.class);
 
   public static final String AUP_API_PATH = "/iam/aup";
@@ -84,7 +79,6 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
       throws IOException, ServletException {
 
     HttpServletRequest req = (HttpServletRequest) request;
-    HttpServletResponse res = (HttpServletResponse) response;
 
     HttpSession session = req.getSession(false);
 
@@ -108,20 +102,6 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
         chain.doFilter(request, response);
         return;
       }
-      if (!res.isCommitted() && aup.isPresent()) {
-        res.sendRedirect(AUP_SIGN_PATH);
-      }
-      return;
-    }
-
-    int remainingDays = signatureCheckService.getRemainingDaysSignatureExpiration(authenticatedUser.get());
-    if ((remainingDays <= EXPIRY_NOTICE_DAYS) && !sessionOlderThanAupCreation(session) && !res.isCommitted()) {
-
-      session.setAttribute(SIGNATURE_REMAINING_DAYS, remainingDays);
-      session.setAttribute(REQUESTING_SIGNATURE, true);
-      res.sendRedirect(AUP_SIGN_PATH);
-      return;
-
     }
 
     chain.doFilter(request, response);

diff --git a/iam-login-service/src/main/webapp/WEB-INF/views/iam/signAup.jsp b/iam-login-service/src/main/webapp/WEB-INF/views/iam/signAup.jsp
@@ -19,13 +19,20 @@
 <%@ taglib prefix="t" tagdir="/WEB-INF/tags/iam"%>
 <t:page title="Sign Acceptable Usage Policy">
   <c:if test="${daysLeftToExpirySignature > 0}">
-      <form id="skip-sign-aup-form" class="sign-aup-form form" action="/iam/aup/skip-sign" method="post">
-      <h2 class="text-center">You still have ${daysLeftToExpirySignature} days to sign</h2>
-      <div>
-        <input id="skip-sign-aup-btn" class="btn btn-success" type="submit" value="Skip the sign AUP">
+      <form id="resign-aup-form" class="sign-aup-form form" action="/iam/aup/sign" method="post">
+      <h2 class="text-center">You still have ${daysLeftToExpirySignature} days to sign</h2>&nbsp
+      <div style="text-align:center;">
+        <input id="resign-aup-btn" class="btn btn-success" type="submit" value="Re-sign the AUP">
+      </div>
+    </form>
+    <h4 class="text-center">Or</h4>
+    <form id="skip-sign-aup-form" class="sign-aup-form form" action="/iam/aup/skip-sign" method="get">
+      <div style="text-align:center;">
+        <input id="skip-sign-aup-btn" class="btn btn-success" type="submit" value="Skip AUP signature">
       </div>
     </form>
   </c:if>
+  <c:if test="${daysLeftToExpirySignature <= 0}">
   <h2 class="text-center">Sign Acceptable Usage Policy</h2>
   <form id="sign-aup-form" class="sign-aup-form form" action="/iam/aup/sign" method="post">
     <c:if test="${aup.text != null}">
@@ -45,4 +52,5 @@
       <input id="sign-aup-btn" class="btn btn-success" type="submit" value="I agree with the AUP terms">
     </div>
   </form>
+  </c:if>
 </t:page>
diff --git a/...-service/src/test/java/it/infn/mw/iam/test/api/aup/AupSignatureCheckIntegrationTests.java b/...-service/src/test/java/it/infn/mw/iam/test/api/aup/AupSignatureCheckIntegrationTests.java
@@ -93,7 +93,7 @@ public void noAupDefinedMeansSignatureNotRequired() {
     IamAccount testAccount = accountRepo.findByUsername("test")
       .orElseThrow(() -> new AssertionError("Expected test account not found"));
 
-    assertThat(service.needsAupSignature(testAccount), is(false));
+    assertThat(service.getRemainingDaysSignatureExpiration(testAccount), is(Integer.MAX_VALUE));
   }
 
   @Test
@@ -116,12 +116,12 @@ public void aupDefinedSignatureChecksTest() throws JsonProcessingException, Exce
 
     mockTimeProvider.setTime(now.getTime() + TimeUnit.MINUTES.toMillis(5));
 
-    assertThat(service.needsAupSignature(testAccount), is(true));
+    assertThat(service.getRemainingDaysSignatureExpiration(testAccount), is(-Integer.MAX_VALUE));
 
     signatureRepo.createSignatureForAccount(testAccount,
         new Date(mockTimeProvider.currentTimeMillis()));
 
-    assertThat(service.needsAupSignature(testAccount), is(false));
+    assertThat(service.getRemainingDaysSignatureExpiration(testAccount), is(365));
 
     mockTimeProvider.setTime(now.getTime() + TimeUnit.MINUTES.toMillis(10));
 
@@ -133,22 +133,22 @@ public void aupDefinedSignatureChecksTest() throws JsonProcessingException, Exce
           patch("/iam/aup").contentType(APPLICATION_JSON).content(mapper.writeValueAsString(aup)))
       .andExpect(status().isOk());
 
-    assertThat(service.needsAupSignature(testAccount), is(false));
+    assertThat(service.getRemainingDaysSignatureExpiration(testAccount), is(364));
 
     mvc.perform(post("/iam/aup/touch")).andExpect(status().isOk());
 
-    assertThat(service.needsAupSignature(testAccount), is(true));
+    assertThat(service.getRemainingDaysSignatureExpiration(testAccount), is(364));
 
     mockTimeProvider.setTime(now.getTime() + TimeUnit.MINUTES.toMillis(20));
 
     signatureRepo.createSignatureForAccount(testAccount,
         new Date(mockTimeProvider.currentTimeMillis()));
 
-    assertThat(service.needsAupSignature(testAccount), is(false));
+    assertThat(service.getRemainingDaysSignatureExpiration(testAccount), is(365));
 
     mockTimeProvider.setTime(now.getTime() + TimeUnit.DAYS.toMillis(366));
 
-    assertThat(service.needsAupSignature(testAccount), is(true));
+    assertThat(service.getRemainingDaysSignatureExpiration(testAccount), is(0));
 
   }
 

diff --git a/...ice/src/test/java/it/infn/mw/iam/test/api/aup/EnforceAupSignatureSuccessHandlerTests.java b/...ice/src/test/java/it/infn/mw/iam/test/api/aup/EnforceAupSignatureSuccessHandlerTests.java
@@ -94,7 +94,7 @@ public void before() {
   public void userIsRedirectedToSignAupPageWhenNeeded() throws IOException, ServletException {
     // when(accountUtils.getAuthenticatedUserAccount()).thenReturn(Optional.of(account));
     when(accountUtils.getAuthenticatedUserAccount(Mockito.any())).thenReturn(Optional.of(account));
-    when(signatureCheckService.needsAupSignature(Mockito.any())).thenReturn(true);
+    when(signatureCheckService.getRemainingDaysSignatureExpiration(Mockito.any())).thenReturn(-1);
 
     handler.onAuthenticationSuccess(request, response, auth);
     verify(session).setAttribute(Mockito.eq(AuthenticationTimeStamper.AUTH_TIMESTAMP), Mockito.any());
@@ -107,7 +107,7 @@ public void userIsRedirectedToSignAupPageWhenNeeded() throws IOException, Servle
   public void delegateIsCalledIfNoSignatureIsNeeded()throws IOException, ServletException {
     // when(accountUtils.getAuthenticatedUserAccount()).thenReturn(Optional.of(account));
     when(accountUtils.getAuthenticatedUserAccount(Mockito.any())).thenReturn(Optional.of(account));
-    when(signatureCheckService.needsAupSignature(Mockito.any())).thenReturn(false); 
+    when(signatureCheckService.getRemainingDaysSignatureExpiration(Mockito.any())).thenReturn(30);
 
     handler.onAuthenticationSuccess(request, response, auth);
     verify(session).setAttribute(Mockito.eq(AuthenticationTimeStamper.AUTH_TIMESTAMP), Mockito.any());
@@ -124,7 +124,7 @@ public void testOAuthAuthenticationIsUnderstood() throws IOException, ServletExc
 
     // when(accountUtils.getAuthenticatedUserAccount()).thenReturn(Optional.of(account));
     when(accountUtils.getAuthenticatedUserAccount(Mockito.any())).thenReturn(Optional.of(account));
-    when(signatureCheckService.needsAupSignature(Mockito.any())).thenReturn(false);
+    when(signatureCheckService.getRemainingDaysSignatureExpiration(Mockito.any())).thenReturn(30);
 
     handler.onAuthenticationSuccess(request, response, oauth);
     verify(session).setAttribute(Mockito.eq(AuthenticationTimeStamper.AUTH_TIMESTAMP), Mockito.any());

diff --git a/iam-voms-aa/src/main/java/it/infn/mw/voms/api/VOMSController.java b/iam-voms-aa/src/main/java/it/infn/mw/voms/api/VOMSController.java
@@ -114,7 +114,7 @@ public String generateAC(@RequestHeader(name = "User-Agent", required = false) S
       }
     } else {
       IamAccount user = context.getIamAccount();
-      if (signatureCheckService.getRemainingDays(user) <= 0) {
+      if (signatureCheckService.getRemainingDaysSignatureExpiration(user) <= 0) {
         VOMSErrorMessage em = VOMSErrorMessage.faildToSignAup(user.getUsername());
         return responseBuilder.createErrorResponse(em);
       }