AUP exemption

[garaimanoj]

No description provided.

[rmiccoli]

In general, the PR looks good to me.

I have only some comments:

• would it be better to handle the IamAupSignatureUpdateError exception and print the error message? now only the status code 405 is shown
• if the user is set up as a service account after having already signed the AUP, does it make sense to set the AUP signature time to null? this should not even trigger the AUP expiry reminder emails

[enricovianello]

• would it be better to handle the IamAupSignatureUpdateError exception and print the error message? now only the status code 405 is shown

I agree, we could add an ExceptionHandler like this in the same controller this https://github.com/indigo-iam/iam/pull/811/files#diff-a8bc81ad573d8d00f0b71699cc5862722f1bc38abdd3c9cd03e206639a9f904aR205-R209

• if the user is set up as a service account after having already signed the AUP, does it make sense to set the AUP signature time to null? this should not even trigger the AUP expiry reminder emails

It makes sense to me. In my opinion when the administrator is prompted with the message of "service account creation" it should be aware of the fact that this account will lose the signatures and it will be exempted from them in the future. Again, as soon as the admin is going to switch again from a service account to a "normal" one, we could be very clear in the message by explaining that the user won't have any signature so he can decide to sign it on behalf in case.
I also prefer that the AUP reminder logic is updated with a filter that excludes service account when it looks for the accounts that have to receive the email.

[garaimanoj]

In my opinion when the administrator is prompted with the message of "service account creation" it should be aware of the fact that this account will lose the signatures and it will be exempted from them in the future. Again, as soon as the admin is going to switch again from a service account to a "normal" one, we could be very clear in the message by explaining that the user won't have any signature so he can decide to sign it on behalf in case.

We had a discussion regarding deletion of existing AUP signature. During that @enricovianello mentioned that if Admin by mistake set the account as service account then we will loose the AUP signature. So we can keep the signature as is and when account will be back as normal account we can have all the signature data as is.

I think checking the service account status check during AUP reminder is a good idea. If all agree I could add that additional check.

[rmiccoli]

When the account is set up as a service account, the signature of the aup is skipped (if it is defined), but what if it accidentally goes to the /iam/aup/sign endpoint and accepts the aup? the exception is thrown at this time, I don't know if the ExceptionHandler should be added here as well

iam/iam-login-service/src/main/java/it/infn/mw/iam/api/aup/AupSignaturePageController.java

Line 99 in bddf581

public ModelAndView signAup(HttpServletRequest request, HttpServletResponse response,

[garaimanoj]

When the account is set up as a service account, the signature of the aup is skipped (if it is defined), but what if it accidentally goes to the /iam/aup/sign endpoint and accepts the aup? the exception is thrown at this time, I don't know if the ExceptionHandler should be added here as well

iam/iam-login-service/src/main/java/it/infn/mw/iam/api/aup/AupSignaturePageController.java

Line 99 in bddf581

public ModelAndView signAup(HttpServletRequest request, HttpServletResponse response,

I am going to add the exception handler and a test.

[enricovianello]

If we add things to SCIM user that are not in the SCIM protocol we need to add them inside ScimIndigoUser extension object

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud