ORCiD authentication - #4145
Conversation
chenejac
left a comment
There was a problem hiding this comment.
@bkampe thanks for this. You might be right about security warning, the issue might be in the Vitro codebase. However, we will know that once we merge your Vitro pull request on the main branch. Meanwhile, let's try to clean up code here. I left one comment about gson dependency. Please try to address that.
brianjlowe
left a comment
There was a problem hiding this comment.
Thank you for this PR. I've noted some lines where minor changes should be made. The main thing to do at this stage is to remove any unused code, template files, etc. related to the DOI claiming and profile creation functionality that is not used. This should greatly simplify the PR and any remaining review.
brianjlowe
left a comment
There was a problem hiding this comment.
Many thanks for the updated PR(s). Looks like it's almost ready. It appears that just a few more methods and inner classes can now be removed since the orcidBio value is not being used; see inline comments.
…idAuthController.java Co-authored-by: Brian Lowe <brian@ontocale.com>
brianjlowe
left a comment
There was a problem hiding this comment.
Thanks for the revisions. I've now tested this on a public server and confirmed that the authentication does work when the ORCID iD is used directly in the networkId matching property, but please see my inline comment related to this. I've also noted a few additional things things that need some attention.
… get linked user account
brianjlowe
left a comment
There was a problem hiding this comment.
Thanks for these updates. I've re-tested the various scenarios and the functionality is working as expected. I think we're down to just two little improvements that will make this more robust with dirtier data, and then we should be ready to merge:
- In getPersonUriByOrcidUri() the first person URI with a matching vivo:orcidId property is returned. This may lead to failed logins if there are duplicate persons in the triple store, because the first person returned may not be the one with a matching property associated with a user account. I would suggest incorporating the matching property from the self editing configuration into the query in getPersonUriByOrcidUri() so that only persons associated with user accounts are returned. If there are multiple of these, then taking the first seems fine. I would, however, also suggest adding "ORDER BY ?person" to that query so that the first one returned is always the same given the same data.
- On line 160, https protocol is assumed for the ORCID URI. This is a good assumption for any recent data, but I think it would be nice to allow for the possibility that older http:// ORCID URIs might exist in some VIVOs and thus check for both protocols.
Otherwise, looking good. Thanks again.
brianjlowe
left a comment
There was a problem hiding this comment.
The logic looks good to me; there are just a few opportunities now to simplify the code a bit, and then this should be good to go.
| } | ||
|
|
||
| private String getPersonUriByOrcidId(VitroRequest vreq, String orcidId) { | ||
| final List<String> personUris = new ArrayList<String>(); |
There was a problem hiding this comment.
This could be String personUri instead of an ArrayList, since you're only retrieving a limit of one value.
There was a problem hiding this comment.
But it's used in the inner class final protected void processQuerySolution(QuerySolution qs). I think a simple string variable could not be accessed from there? But it could be simplified to a one element string array String[] personUri = new String[1];
There was a problem hiding this comment.
Thanks, you're right; I overlooked that. In that case, I would just leave it as is. I deleted my related comment below.
brianjlowe
left a comment
There was a problem hiding this comment.
Looks great. Many thanks for all your work on this.
chenejac
left a comment
There was a problem hiding this comment.
@bkampe please check my two comments.
Also, I think we might discuss UI design of the feature. At the moment, when orcid.authEnabled is false, the login widget is the following one
When orcid.authEnabled is true, the base login widget is the following one
And it can be extended
The another option might be just to add Log in via ORCID button next to the green Log in button (right of that) at the first picture.
At the moment, this is linked with Vitro code, but I think we should also discuss why Log in via ORCID UI is there, and expecially taking into account it uses labels defined in VIVO code base.
| clientId = configProperties.getProperty(CONFIGURATION_AUTH_ORCID_CLIENT_ID); | ||
| clientSecret = configProperties.getProperty(CONFIGURATION_AUTH_ORCID_CLIENT_PASSWORD); |
There was a problem hiding this comment.
Can we use here OrcidClientContext properties? - OrcidClientContext.Setting.CLIENT_ID, OrcidClientContext.Setting.CLIENT_SECRET
| ConfigurationProperties configProperties = ConfigurationProperties.getInstance(); | ||
| clientId = configProperties.getProperty(CONFIGURATION_AUTH_ORCID_CLIENT_ID); | ||
| clientSecret = configProperties.getProperty(CONFIGURATION_AUTH_ORCID_CLIENT_PASSWORD); | ||
| callbackUrl = configProperties.getProperty(CONFIGURATION_AUTH_ORCID_CALLBACK_URL) + "orcidAuth/callback"; |
There was a problem hiding this comment.
I think in ORCID push data PR the hardcoded callback specified in OrcidClientContext (OrcidClientContext.Setting.CALLBACK_PATH) has been tried to be reused. It is allowed to have more than one callbackUrl/redirectUrl in ORCID registration, but not sure what might be a good practice in our case taking into account maintainance and configuration? @bkampe Can you please investigate a little bit and write here in the comment your arguments? Here is an LLM response about advantages of single and multi callback URLs:
Advantages of Using a Single Callback URL
Simpler ORCID Configuration
Only a single Redirect URI needs to be registered in the ORCID Developer Tools.
Reduces the risk of configuration errors during migrations, deployments, or domain changes.Easier Maintenance
All OAuth responses are handled by a single component or endpoint.
Provides a centralized location for logging, auditing, monitoring, and debugging.Alignment with Standard OAuth Practices
OAuth is designed to support multiple authorization flows through the use of the state parameter.
Different operations (e.g., login, ORCID verification, data synchronization) can be distinguished and routed appropriately based on the value of the state parameter, while sharing the same callback URL.
Advantages of Using Multiple Callback URLs
Clear Separation of Concerns
Each ORCID-related flow (login, verification, data push) has its own dedicated endpoint.
Business logic is naturally isolated per use case, which can make the code easier to understand in larger systems.Simplified Request Handling
No need to inspect or route logic based on the state parameter for basic flow separation.
Each callback directly maps to a single, well-defined purpose.Improved Security Isolation (in some architectures)
Different callback endpoints can be secured independently.
Useful when different flows require different validation rules, scopes, or permission checks.Easier Integration with Modular or Microservice Architectures
Each service or module can expose its own callback endpoint.
Reduces coupling between authentication flows and downstream processing logic.
Sign-in using ORCID credentials.
VIVO GitHub issue: (ORCID authentication (login) within VIVO #4071)
What does this pull request do?
This pull request adds the posibility to sign-in using ORCID credentials.
What's new?
Added java class OrcidAuthController.java which contains main functionality to log in with ORCID credentials. With a click on "Login via ORCID" the user gets redirected to the ORCID login page. After entering the crendentials ORCID redirects to a predefined callback-URL.
How should this be tested?
To test this feature you need to
auth.orcid.clientId = APP-0FHD0100VVK2E4FPauth.orcid.clientPassword = bed12525-xxxx-xxxx-xxxx-xxxxxxxxxxxxxauth.orcid.callbackUrl = http://kampe.ddns.net:62808/orcid_auth_war/orcidAuth/callbackAdditional Notes:
For registering an API client and setting callback URLs see https://info.orcid.org/documentation/api-tutorials/api-tutorial-get-and-authenticated-orcid-id/
Interested parties
@chenejac @hauschke