Public ORCID login is available. #7025

felker13 · 2020-06-26T13:12:07Z

What this PR does / why we need it: This change will make it possible to use orcid public api for orcid authentication. Using member api is still possible, the chosen method depends on the userEndpoint parameter provided in the uploaded json. This feature is also mentioned in #6329.

Which issue(s) this PR closes: none

Special notes for your reviewer: none

Suggestions on how to test this: Use the new orcid-public.json with your ORCID public api credentials to create a new login option. Using the new option you should be able to log in to Dataverse using your ORCID credentials.

Does this PR introduce a user interface change? If mockups are available, please link/include them here: no

Is there a release notes update needed for this change?: This should be included in the release notes as a new feature.

Additional documentation: none

pdurbin

@felker13 this is very exciting! Would you be able to make any necessary changes to the OAuth section of the Installation guide?

In particular, if it's no longer (or never was) true that ORCID requires institutions to pay to use their login service, we should change this line:

"ORCID and Microsoft, on the other hand, do not have an automated system for requesting these credentials, and it is not free to use these authentication services."

I'm highlighting it in blue below:

It's from this page: http://guides.dataverse.org/en/4.20/installation/oauth2.html

Here's the source: https://github.com/IQSS/dataverse/blob/develop/doc/sphinx-guides/source/installation/oauth2.rst

Would you be able to make this change?

The code seems fine. I assume one chooses between public and non-public using the userEndpoint part of factoryData.

felker13 · 2020-06-26T22:53:07Z

When the user chooses ORCID authentication, he will be redirected to ORCID, where he must log in with his ORCID credentials. If this is successful, ORCID will return an access token. That token is saved into oauth2tokendata table during the registration process and will be used later to authenticate the user. The user data that dataverse can obtain from ORCID depends on the api type, but it is independent from the authentication. <https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> Mentes a vírusoktól. www.avast.com <https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2> qqmyers <notifications@github.com> ezt írta (időpont: 2020. jún. 26., P, 17:59):

…

Unless I'm missing something (which could be - I haven't played with ORCID in a while)- the public API allows you to read the user's public info, but you can read the profile of anyone you have the ID for, versus only someone that has successfully completed an Oauth2 login there. So the public /read-limited is great for getting the name of a list of creators or contacts you have ORCIDs for, but being able to retrieve the info doesn't prove the user is authenticated - which would be a problem if this call is involved in verifying the user is logged in. Is that the case? Or is there code elsewhere that does that? Or am I misunderstanding what the public ORCID API can do these days? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7025 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGI3Z5FAMBG3YBTRCRT4NZTRYTAWXANCNFSM4OJKFSSQ> .

coveralls · 2020-06-29T08:43:37Z

Coverage increased (+0.002%) to 19.655% when pulling 298bfab on dsd-sztaki-hu:orcid-public-api into 6daf219 on IQSS:develop.

pdurbin · 2020-06-29T15:20:36Z

@felker13 I made a pull request to change the docs at dsd-sztaki-hu#1

Please take a look. Thanks.

remove cost, link to ORCID APIs public, member IQSS#7025

felker13 · 2020-06-30T07:37:58Z

@pdurbin I agree with your suggestions, so I merged your changes. Thank you.

pdurbin

This looks good but I haven't tested it.

The idea is to add a second ORCID login option that uses's ORCID's public API. The older option that uses the member API should continue to work.

djbrooke · 2020-06-30T14:43:56Z

Hi @felker13 - can you include a screenshot of this? I wasn't aware that this may have impact to the UI. If it's in the way I understand it above, I'm hesitant to provide multiple options for ORCID login, as it may confuse people. Also, are there other applications that have these multiple options implemented in the same way? If so, can you point us to some of these platforms? Thank you!

pdurbin · 2020-06-30T14:49:00Z

@djbrooke there would not be multiple ORCID logins. What is being provided is more flexibility for the sysadmin who is configuring Dataverse. Basically, if you're institution is already a member of ORCID (like Harvard), you use the ORCID member API we've always supported. If your institution is not a member of ORCID, you use ORCID's public API.

djbrooke · 2020-06-30T14:54:27Z

@pdurbin thanks, so the decision about which login option to use would be made at the installation level, not at the researcher level. Makes sense, perfect.

@felker13 I am going to keep this assigned to you. Can you please update the top comment of the PR to match the pull request template format in other PRs (like in #7039)? It will help us test and get this merged. Thank you!

poikilotherm

Overall this is a very good idea for a very simple approach, following "convention over configuration".

A unit test case at OrcidOAuth2APTest is missing for this and should be done before merging to enhance code coverage. Happy to help if wanted.

poikilotherm · 2020-07-01T20:51:31Z

doc/sphinx-guides/source/installation/oauth2.rst


 URLs to help you request a Client ID and Client Secret from the providers supported by Dataverse are provided below.  For all of these providers, it's a good idea to request the Client ID and Client secret using a generic account, perhaps the one that's associated with the ``:SystemEmail`` you've configured for Dataverse, rather than your own personal Microsoft Azure AD, ORCID, GitHub, or Google account:

- ORCID: https://orcid.org/content/register-client-application-production-trusted-party
+- ORCID: https://orcid.org/content/register-client-application-0


Suggested change

- ORCID: https://orcid.org/content/register-client-application-0

- ORCID: https://orcid.org/content/register-client-application

Can you please update the top comment of the PR to match the pull request template format in other PRs

This done so I'm moving this to code review.

Interesting. I got the longer link with the "-0" at the end from https://orcid.org/organizations/integrators but I do think the shorter link looks nicer (and goes to the same place) so it would be a good change. @felker13 I don't have access to accept this change.

poikilotherm · 2020-07-01T21:07:31Z

src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/impl/OrcidOAuth2AP.java

+        String s = null;
+        if(userEndpoint != null){
+            s = userEndpoint.startsWith("https://pub") ? "/authenticate" : "/read-limited";
+        }
+        scope = Arrays.asList(s);


One could argue that this can be done more compact with only one if and without an extra variable allocation.

E. g. like

if(userEndpoint != null && userEndpoint.startsWith("https://pub")) { this.scope = Arrays.asList("/authenticate"); } else { this.scope = Arrays.asList("/read-limited"); }

Or you leave it to the code optimizer of the JVM... 😉

I'm fine with letting the JVM optimize it. 😄

pdurbin · 2020-07-02T13:51:41Z

@felker13 can you please create an issue for this pull request to close? It can be short. Just a title and a one line description is fine. Thanks!

pdurbin · 2020-07-02T14:53:20Z

A unit test case at OrcidOAuth2APTest is missing for this and should be done before merging to enhance code coverage. Happy to help if wanted.

@poikilotherm as we discussed at http://irclog.iq.harvard.edu/dataverse/2020-07-02#i_125994 you are welcome to write the extra unit tests either before or after this pull request is merged. Thanks for offering!

…nticate. IQSS#7025

poikilotherm · 2020-07-08T09:37:39Z

@pdurbin @felker13 I just created dsd-sztaki-hu#2, merging latest develop and unit tests for this.

pdurbin · 2020-07-08T15:34:48Z

@poikilotherm thanks. Is it possible to make the diff smaller? I assume to do so one would merge the latest from develop into dsd-sztaki-hu:orcid-public-api (this pull request).

poikilotherm · 2020-07-08T16:28:46Z

@pdurbin yeah, that should make the diff much smaller...

7025 orcid public

Public ORCID login is available.

c10b516

pdurbin requested a review from poikilotherm June 26, 2020 14:28

pdurbin reviewed Jun 26, 2020

View reviewed changes

Fix for NPE in test.

55c2c89

Oauth documentation is extended with ORCID public API option.

8da201b

pdurbin self-assigned this Jun 29, 2020

remove cost, link to ORCID APIs public, member IQSS#7025

9d7c843

pdurbin mentioned this pull request Jun 29, 2020

remove cost, link to ORCID APIs public, member #7025 dsd-sztaki-hu/dataverse#1

Merged

pdurbin removed their assignment Jun 29, 2020

pdurbin assigned felker13 Jun 29, 2020

Merge pull request #1 from IQSS/7025-orcid-docs

0bcd871

remove cost, link to ORCID APIs public, member IQSS#7025

pdurbin unassigned felker13 Jun 30, 2020

pdurbin approved these changes Jun 30, 2020

View reviewed changes

djbrooke assigned felker13 Jun 30, 2020

poikilotherm reviewed Jul 1, 2020

View reviewed changes

pdurbin unassigned felker13 Jul 2, 2020

pdurbin approved these changes Jul 7, 2020

View reviewed changes

poikilotherm added a commit to poikilotherm/dataverse that referenced this pull request Jul 8, 2020

Refactor userEndpoint checks for Public ORCID scope. IQSS#7025

0e75d27

poikilotherm added a commit to poikilotherm/dataverse that referenced this pull request Jul 8, 2020

Convert ORCID OAuth2 provider test from JUnit4 to JUnit5. IQSS#7025

fc9e633

poikilotherm added a commit to poikilotherm/dataverse that referenced this pull request Jul 8, 2020

Add test case for public ORCID API endpoint to result in scope /authe…

12fe798

…nticate. IQSS#7025

poikilotherm added 4 commits July 8, 2020 11:33

Merge branch 'develop' into orcid-public-api

8063067

Refactor userEndpoint checks for Public ORCID scope. IQSS#7025

5a98e64

Convert ORCID OAuth2 provider test from JUnit4 to JUnit5. IQSS#7025

9475838

Add test case for public ORCID API endpoint to result in scope /authe…

d588210

…nticate. IQSS#7025

kcondon self-assigned this Jul 9, 2020

Merge pull request #2 from poikilotherm/7025-orcid-public

298bfab

7025 orcid public

kcondon merged commit f5104d1 into IQSS:develop Jul 20, 2020

djbrooke added this to the Dataverse 5 milestone Jul 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public ORCID login is available. #7025

Public ORCID login is available. #7025

felker13 commented Jun 26, 2020 •

edited

Loading

pdurbin left a comment

felker13 commented Jun 26, 2020 via email

coveralls commented Jun 29, 2020 •

edited

Loading

pdurbin commented Jun 29, 2020

felker13 commented Jun 30, 2020

pdurbin left a comment

djbrooke commented Jun 30, 2020

pdurbin commented Jun 30, 2020

djbrooke commented Jun 30, 2020

poikilotherm left a comment •

edited

Loading

poikilotherm Jul 1, 2020

pdurbin Jul 2, 2020

pdurbin Jul 2, 2020

poikilotherm Jul 1, 2020

pdurbin Jul 2, 2020

pdurbin commented Jul 2, 2020

pdurbin commented Jul 2, 2020

poikilotherm commented Jul 8, 2020

pdurbin commented Jul 8, 2020

poikilotherm commented Jul 8, 2020

	- ORCID: https://orcid.org/content/register-client-application-0
	- ORCID: https://orcid.org/content/register-client-application

Public ORCID login is available. #7025

Public ORCID login is available. #7025

Conversation

felker13 commented Jun 26, 2020 • edited Loading

pdurbin left a comment

Choose a reason for hiding this comment

felker13 commented Jun 26, 2020 via email

coveralls commented Jun 29, 2020 • edited Loading

pdurbin commented Jun 29, 2020

felker13 commented Jun 30, 2020

pdurbin left a comment

Choose a reason for hiding this comment

djbrooke commented Jun 30, 2020

pdurbin commented Jun 30, 2020

djbrooke commented Jun 30, 2020

poikilotherm left a comment • edited Loading

Choose a reason for hiding this comment

poikilotherm Jul 1, 2020

Choose a reason for hiding this comment

pdurbin Jul 2, 2020

Choose a reason for hiding this comment

pdurbin Jul 2, 2020

Choose a reason for hiding this comment

poikilotherm Jul 1, 2020

Choose a reason for hiding this comment

pdurbin Jul 2, 2020

Choose a reason for hiding this comment

pdurbin commented Jul 2, 2020

pdurbin commented Jul 2, 2020

poikilotherm commented Jul 8, 2020

pdurbin commented Jul 8, 2020

poikilotherm commented Jul 8, 2020

felker13 commented Jun 26, 2020 •

edited

Loading

coveralls commented Jun 29, 2020 •

edited

Loading

poikilotherm left a comment •

edited

Loading