Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TTO-231 multivalue env email #187

Merged
merged 5 commits into from
Jun 19, 2024
Merged

TTO-231 multivalue env email #187

merged 5 commits into from
Jun 19, 2024

Conversation

moseshll
Copy link
Collaborator

@moseshll moseshll commented Jun 18, 2024

This is in response to an unusual behavior noted with University of Kentucky's use of Shib when their users are authenticated for CRMS access. CRMS determines who is who (matching its users against both the crms.users table as well as ht.ht_users) by consulting $ENV{REMOTE_USER} and falling back to $ENV{email}. uky.edu was providing email values like [email protected];[email protected] possibly due to some misconfiguration. I agreed with A&E (after a long slack thread referenced in the ticket) that we should just try to accommodate multiple values as we would fields like eppn. This we have an additional loop over possible multiple values of email where previously we have been assuming it held a single value.

The commits:

  • A testable utility routine that parses the ENV{email} value and applies needed transformations.
  • Modify the caller subroutine SetupUser to use the new utility. The is the meat of the branch.
  • Tidy SetupUser.
  • Fix deferencing bug not detected prior to integration test under dev-2

Reviewer:

  • Would like a scan of the modified SetupUser in the area of the ENV{email} check -- there is some cleanup work to be done where the code unnecessarily refers to ht_repository and that is the subject of a lower-priority GitHub issue to be addressed later.
  • Would like to have a quick review of the added tests. Should extract_env_email take an optional string parameter defaulting to ENV{email} so we don't have to set and restore ENV in the tests?

@moseshll moseshll requested a review from mwarin June 19, 2024 18:05
Copy link

@mwarin mwarin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scan of the modified SetupUser

Looks good to me. I mean, yes to the cleanup, but I see nothing worth mentioning.

quick review of the added tests

I think I addressed that in my comments.

APPROVE.

t/CRMS.t Show resolved Hide resolved
cgi/CRMS.pm Show resolved Hide resolved
@moseshll moseshll merged commit 83a5c70 into main Jun 19, 2024
1 check passed
@moseshll moseshll deleted the TTO-231_multivalue_ENV_email branch June 19, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants