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

[OpenID] Add OpenID Connect support to LORIS #8255

Merged
merged 13 commits into from
Nov 1, 2023
Merged

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Dec 6, 2022

Brief summary of changes

This adds a new module, oidc, to add support for OpenID Connect to LORIS. This allows LORIS to be configured to login with third party identity providers (ie. "Login with Google", "Login with Facebook", etc).

If a user authenticates with an OIDC provider that's been configured the behaviour will depend on whether there is a user with the same email address that exists in LORIS. If a user exists, it will be logged in. If there is no user, they will be redirected to the Request Account page with the known information that came back from OpenID pre-filled.

Testing instructions (if applicable)

You will need to get credentials from an OpenID provider, the exact details of how to register your app will vary based on who you use. (I used auth0 for dev and testing since it allowed localhost URLs for testing, other providers only allow allow https:// on a top level domain to be used to register an app.)

After registering with your openid identity provider, you need to manually insert into the openid_connect_providers with the values that came from registering your app (except Name, which is internal to LORIS and used for displaying the "Login with.." text on the login page.) See description in the module README.

@driusan driusan added Release: Add to release notes PR whose changes should be highlighted in the release notes Category: Feature PR or issue that aims to introduce a new feature State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed labels Dec 6, 2022
@driusan driusan force-pushed the OpenIDConnect branch 2 times, most recently from 0e7b216 to 928b189 Compare December 6, 2022 18:48
@driusan driusan removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Dec 6, 2022
@laemtl laemtl modified the milestone: 25.0.0 Mar 14, 2023
@ridz1208 ridz1208 removed this from the 25.0.0 milestone Mar 21, 2023
@christinerogers
Copy link
Contributor

per Loris meeting Oct.10 - shouldn't be a big task, mostly review

Copy link
Contributor

@jeffersoncasimir jeffersoncasimir left a comment

Choose a reason for hiding this comment

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

I have tested this PR with 3 providers: Google, Globus and ORCID. Total success was only achieved with Google but I managed to authenticate with all of them. Here are my observations.

email_verified:

In all 3 cases, the request was not accepted when email_verified was in the scope parameter. It should probably be removed.

  • Google: For the 3 providers I tested, only Google had email_verified in their claims_supported array. It was included in the response without it being present in the scope. Worked as expected once removed.

  • Globus: Returns the email but does not return email_verified. This causes an issue here but otherwise works.

  • ORCID: Does not have email scope so cannot be immediately associated to an account. This provider would require an extra API call or some DB accommodations.

JWK::parseKeySet():

This only worked out the box for Google.

  • Globus: This error is thrown: "kid" empty, unable to lookup correct key. I was able to get it to work by manually generating the key using JWK::createPemFromModulusAndExponent and passing that to JWT::decode.

  • ORCID: This error is thrown: JWK must contain an "alg" parameter. I got it to work by passing $defaultAlg = 'RS256' to JWK::parseKeySet. This value can be obtained from the discovery config.

Notes:

  • The code suggestion I made is because ORCID returns "token_type":"bearer" all lowercase.
  • Perhaps inserting the module should be part of the patch.
  • client_id and client_secret in the form_params was not required for any of the providers I used.
  • I will gladly test this with other providers, if requested. Let me know which one(s).

modules/oidc/php/callback.class.inc Show resolved Hide resolved
@driusan
Copy link
Collaborator Author

driusan commented Oct 31, 2023

@jeffersoncasimir Thanks for the review. When I was writing it I used auth0, but you don't need to test it with that.

I'm not sure what's actionable and what's not in your review.

With Globus, is it a question of the software not supporting the email_verified claim, or a question of your email address not being verified with them? If we trust unverified email addresses claims I think it opens up security problems we don't want to deal with and I'm not sure how to move forward.

For the JWK problems, is this a problem in our code, or a problem with the library we're using? Would upgrading the version of php-jwt used by LORIS potentially fix them? How do we move forward with this?

@jeffersoncasimir
Copy link
Contributor

With Globus, is it a question of the software not supporting the email_verified claim, or a question of your email address not being verified with them? If we trust unverified email addresses claims I think it opens up security problems we don't want to deal with and I'm not sure how to move forward.

Globus does not return email_verified as is, so we cannot securely use this provider. There might be a way to determine if the account came from a verified Google account but it would require adapting the module. I think there is no actionable here and we should stick with the more secure option.

For the JWK problems, is this a problem in our code, or a problem with the library we're using? Would upgrading the version of php-jwt used by LORIS potentially fix them? How do we move forward with this?

I am using the latest version of the library (firebase/php-jwt v6.9.0) and we are relying on its keys parsing function to determine which algorithm to use. That function throws an error because the providers' keys are each missing a (different) field, which is not actually needed to generate the key. I place the blame somewhere between the library for being too strict and the providers omitting a field, and perhaps a more lenient library or determining the algorithm from discovery and deliberately generating the right type of key would work more broadly.

We cannot really use these providers right now though (no email_verified field) so maybe I can test it on other actual potential candidates to determine if any further actions should be taken. For now, the only provider that was appropriate (Google) worked great once the email_verified field was omitted from scope. It would also be more secure to remove the client_secret from the form_params since it gets sent encoded in auth,

This adds support for the OpenID Connect protocol in LORIS, allowing
us to include "Login with ...." links on the login page and delegate
to third parties which implement the protocol such as Google or Facebook
or Globus. For writing this, I used auth0 (because they allow http://localhost
URLs for development.)

When a user clicks the "Login with ...." link on the login page, they
should be redirected to the third party to authenticate the request. If
they authorize it, LORIS will check if a user exists with the email address
returned. If it already exists, they will be logged in. If not, they will
be redirected to the request account page with the known information (name
and email address) populated. (We can not determine their site or project
or radiologist/examiner status from the third party.)
@driusan
Copy link
Collaborator Author

driusan commented Oct 31, 2023

@jeffersoncasimir I rebased and removed the client_id/secret

@driusan
Copy link
Collaborator Author

driusan commented Oct 31, 2023

I also just removed email_verified from the scope.

Since I'm not sure if they were required by the implementation I was using when developing and I don't think I have it setup anymore, could you test quickly with auth0 and confirm that it still works with Google?

I think we should try and get Globus (used by some projects with overrides without using OIDC) and ORCID (highly demanded) working, but we can make issues for those and iterate on this.

Copy link
Contributor

@jeffersoncasimir jeffersoncasimir left a comment

Choose a reason for hiding this comment

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

I just pulled the changes and it works as expected and without modifications for both Google and auth0. The key situation persists for Globus and ORCID.

@driusan driusan merged commit a31025d into aces:main Nov 1, 2023
19 checks passed
@ridz1208 ridz1208 added this to the 26.0.0 milestone Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature Release: Add to release notes PR whose changes should be highlighted in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants