Combine CERN HR logic with internal life-cycle

[enricovianello]

Updates on label logic:

• label hr.cern.cern_person_id contains CERN unique identifier of the user
• label hr.cern.ignored is used to exclude a user that has a cern_person_id from updating his personal data (given name, family name and optionally email) and his membership end-time from the data retrieved from HR database; this means, for example, that administrators have to manually manage his membership endTime to avoid suspension;
• label hr.cern.timestamp value was used to express the last time HR handler has run. Due to the fact it was updated each time the handler runs, also triggering an AccountLabelSetEvent, it was removed. We can rely on LOG to understand when the handler runs;
• label hr.cern.status contains the current status of contacting the API for that user; possible values are:
  • IGNORED used when label hr.cern.ignore is present;
  • ERROR in case of failed connections to HR API or null VOPerson record obtained;
  • NOT_FOUND in case HR API record is obtained but no experiment participation is found at all;
  • MEMBER when API returns a VOPerson record not null and the participation to the experiment is found and the expiration date is still valid (not expired);
  • EXPIRED when API returns a VOPerson record not null and the participation to the experiment is found but it has expired;
• label hr.cern.message contains more details about status value;
• label hr.cern.skip-email-synch, if set, is used to skip email update from HR record;
• label hr.cern.action has been removed.

New CERN-HR-lifecycle-handler logic doesn't suspend users. It relies on the other internal expired-accounts-handler logic. But it still restores users those have been suspended by the expired-accounts-handler because of the end of suspension grace period.

This means a grace period is now applied for CERN profile.

[enricovianello]

Schema that trues to describe the new logic
CERN HR lifecycle handler v2.pdf

[enricovianello]

@giacomini I'm not sure what's better between adding into the logic the delete of the unused action label (db is not involved is label doesn't exist) https://github.com/indigo-iam/iam/pull/844/files#diff-73664991d3c69420bd0bdde5aa47020a0e11cdb51168ffa015a974685bcce26fR213 and launching a script that deletes every action label for each user. Another solution could be remove the delete I added into this release and add a proper migration within next release, because next release will contains several db migrations.

[balciiberk]

@enricovianello thanks for quickly implementing this! And thanks for the explanatory schema! I think this logic cleanly solves both the grace period issue and "accounts getting suspended when they change institutes' issue. And the new status labels are much more descriptive than the previous ones.

[enricovianello]

@enricovianello thanks for quickly implementing this! And thanks for the explanatory schema! I think this logic cleanly solves both the grace period issue and "accounts getting suspended when they change institutes' issue. And the new status labels are much more descriptive than the previous ones.

Thanks @balciiberk :)
We were thinking about a small update to the logic in case no participation is found for the user. In the previous schema only an error is added to labels but endtime is not touched. This means that if a user changes experiment (for example), the end-time will remain and that user will be able to continue obtaining tokens, etc. Administrators can easily not recognize the ERROR status on labels so probably in this case (no participation record found for the user),the first time the user is not found on that experiment, his endtime should be updated to current time in order to "activate" the grace period. If things won't change within 7 days, that user will be suspended.

What do you think?

CERN HR lifecycle handler v2-updated.pdf

(Update) we could define another status "NOT_FOUND" to make it different from an API error

[balciiberk]

@enricovianello This makes sense in the case that the error is "null VOPerson record obtained". But when there is another error returned from hr-db-api, this can create problems. For example, if the hr-db-api is down for some reason, it would return error to all of the users and it would start the suspending grace period for all of the users.

If we separate these 2 kinds of errors and change the endTime on only the error "null VOPerson record obtained", and don't touch the endTime on any other error, it makes sense I think.

[enricovianello]

@enricovianello This makes sense in the case that the error is "null VOPerson record obtained". But when there is another error returned from hr-db-api, this can create problems. For example, if the hr-db-api is down for some reason, it would return error to all of the users and it would start the suspending grace period for all of the users.

In case of down we cannot reach the code that updates the endTime so this problem should not exist...

If we separate these 2 kinds of errors and change the endTime on only the error "null VOPerson record obtained", and don't touch the endTime on any other error, it makes sense I think.

..but keeping different statuses surely reduces the possible errors! I updated the logic:

CERN HR lifecycle handler v2.2.pdf

I highlight in this schema also the raised events. I added also the logic necessary to avoid event spamming when values are updated with always the same value. As you can see the cases when the endtime is updated are:

• the VOPerson record exists + experiment is found: value is the one retrieved from HR API
• the VOPerson record exists + experiment is not found: value is the start of the day (but only the first time to avoid infinite grace period)
  This means that with an API down nothing is touched.

I'm wondering if the update of timestamp label for every account each time the handler runs makes sense (it means hundreds of useless AccountLabelSetEvents).

[balciiberk]

In this case, logicwise everything looks good to me!

I'm wondering if the update of timestamp label for every account each time the handler runs makes sense (it means hundreds of useless AccountLabelSetEvents).

I agree. If this value isn't used somewhere else, I don't think we need to know every account's last updated time. Seeing that the lifecycle ran from the logs is enough I think

[enricovianello]

Schema with latest updates:
CERN HR lifecycle handler v2.3.pdf

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud