I think this is fine as long as we don't anticipate any new users to the system. That being said, with decommissioning on the horizon, I doubt it's worth worrying about such things.

For the sake of knowledge sharing, the email address claim is also referenced in config/initializers/ndr_authenticate.rb (config.saml_resource_locator = ...).

I think this is fine as long as we don't anticipate any new users to the system. That being said, with decommissioning on the horizon, I doubt it's worth worrying about such things.

For the sake of knowledge sharing, the email address claim is also referenced in config/initializers/ndr_authenticate.rb (config.saml_resource_locator = ...).

I've updated the PR: config/initializers/ndr_authenticate.rb now looks up users based on upn instead of emailaddress.

I've now reverted to the previously approved version, which I'll merge now. Here are Miles's additional comments:

To be able to handle new users, I think we need to map the UPN claim to the email column in the users table (via config/attribute-map.yml) so as to satisfy presence validations and NOT NULL constraints on the email attribute/column.
I’m not sure if devise-saml-authenticatable will allow mapping one claim to multiple columns (e.g. email and upn) :thinking_face:
The additional code in saml_resource_locator is/was a fallback for existing DMS users who were signing in with PHE creds for the first time, where a user record wouldn’t have been found by object_guid lookup (Devise.saml_default_user_key) and would otherwise have ended up creating a duplicate account for the individual without the email lookup fallback. This is probably unlikely at this point in time and I mentioned it for the sake of completeness.