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

Add ORCID iD Authenticator #655

Closed
wants to merge 5 commits into from
Closed

Add ORCID iD Authenticator #655

wants to merge 5 commits into from

Conversation

matthewwiese
Copy link
Contributor

This PR adds an ORCID iD authenticator class inheriting from OAuthenticator, based on the implementation of the GitHubOAuthenticator class.

The tests are rudimentary - I'm happy to implement suggestions from maintainers.

This ought to resolve #284

@welcome
Copy link

welcome bot commented Jul 28, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@manics
Copy link
Member

manics commented Jul 28, 2023

Hi! We'd like to avoid adding more single service authenticators due to the maintenance and testing burden. Is there anything in here that can't be done by configuring the GenericAuthenticator?

@matthewwiese
Copy link
Contributor Author

matthewwiese commented Jul 28, 2023

Hi! We'd like to avoid adding more single service authenticators due to the maintenance and testing burden. Is there anything in here that can't be done by configuring the GenericAuthenticator?

Nope!

Should a code snippet using GenericAuthenticator be added to the docs? As it stands, #284 remains open without a usable example, and ORCID is popular enough that I imagine you'll be getting questions related to it in the future.

Edit: There is the normalize_username() override I did have to add, as by default it would lowercase 0000-0002-9079-593X to 0000-0002-9079-593x which is not strictly-speaking correct for ORCID iDs.

@manics
Copy link
Member

manics commented Jul 28, 2023

A documentation PR would be great!

Is it necessary to keep the upper case X? These are JupyterHub usernames so the requirement is a one-to-one mapping from the ORCID username to the JupyterHub username. Are ORCID usernames case sensitive?

In general you can't rely on going from the JupyterHub username back to the original username as downstream authenticators/spawners may need to manipulate it in other ways

@matthewwiese
Copy link
Contributor Author

Closing this PR.

A documentation PR would be great!

I can do this. @manics: to what page(s) should this be added?

Are ORCID usernames case sensitive?

At least according to my reading of this page, the capital "X" is necessary for recognizing the decimal value of 10 for the checksum.

In general you can't rely on going from the JupyterHub username back to the original username as downstream authenticators/spawners may need to manipulate it in other ways

That's good to know, thank you. If at some point my use case needs the username to map 1-to-1 then I will use the custom authenticator, otherwise GenericOAuthenticator works just fine.

@manics
Copy link
Member

manics commented Aug 2, 2023

@matthewwiese There's a page with example configurations:

But there's always room for improving the docs, so let us know if you think there's a better way to present this.

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.

Add documentation for ORCID-based authentication
2 participants