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.

Sign up for GitHub

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

Jump to bottom

Upgrade ScribeJava to latest version #5991

Closed
poikilotherm opened this issue Jul 4, 2019 · 6 comments · Fixed by #5997
Closed

Upgrade ScribeJava to latest version #5991

poikilotherm opened this issue Jul 4, 2019 · 6 comments · Fixed by #5997

Comments

@poikilotherm
Copy link
Contributor

This is an implementation task for the salvation of #5974

With the current OAuth2 providers, you can only connect to specific providers. For using an IDM/IAM, we need a more general approach, more like mentioned in #4383.

These days, OpenID Connect defines a common standard based on OAuth2, where at least some scopes and claims are standardized. ScribeJava offers some (very) limited support for using the standard, so let's try this...
@poikilotherm poikilotherm mentioned this issue Jul 4, 2019
Closed
5 tasks
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Jul 4, 2019
@poikilotherm
IQSS#5991 - Update from ScribeJava v3.1.0 to v6.6.3
13465b7
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Jul 5, 2019
@poikilotherm
IQSS#5991. Change BaseAPI to DefaultApi20, as upstream removed the ol…
9aa7fe8 
…d one.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Jul 5, 2019
@poikilotherm
IQSS#5991. Refactor OAuth login bean and provider superclass to be co…
1a4e449 
…mpatible with recent ScribeJava library. Refactored code structure a bit, too.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Jul 5, 2019
@poikilotherm
IQSS#5991. Refactor ORCID provider to use current ScribeJava lib. Ref…
26af35e 
…actor base class to avoid code duplication when generating the user record.
@poikilotherm poikilotherm mentioned this issue Jul 5, 2019
Merged
8 tasks
@poikilotherm
Copy link
Contributor Author

I just created PR #5997.
@pdurbin could you please add this to code review? Before I add more unit tests, I would like to know if this goes into the right direction.

Beware, this is not yet tested "in the wild" against Github/Google. That's the next step... 😉

@pdurbin
Copy link
Member

pdurbin commented Jul 8, 2019

With the current OAuth2 providers, you can only connect to specific providers.

Yes. This reminds me of this issue: "As a Dataverse Installation Administrator, I want to add an alternative OAuth provider as a plugin, so that I don't have to fork the core code" #4383

@poikilotherm will your current effort help with this? You might be interested in the slide below about federated login presented by @philippconzett during the 2019 community meeting: https://osf.io/cqsrj/
auth

A video of this talk is available: https://youtu.be/vAPpKuDQUDY?t=746

Before I add more unit tests

I don't see any tests yet in pull request #5997. 😄

@poikilotherm
Copy link
Contributor Author

Hey @pdurbin

Yes. This reminds me of this issue: "As a Dataverse Installation Administrator, I want to add an alternative OAuth provider as a plugin, so that I don't have to fork the core code" #4383

@poikilotherm will your current effort help with this? You might be interested in the slide below about federated login presented by @philippconzett during the 2019 community meeting: https://osf.io/cqsrj/

I referenced #4383 in my description already... 😉 To answer your question: it might help. People need to attach their IDM to the other services. This is definitly possible, like it has been done with Unity IDM to connect to a lot more providers than currently exist in Dataverse (see b2access as an example, offering Win Live and Facebook)

I don't see any tests yet in pull request #5997. smile

I wasn't clear enough... There are currently no unit tests at all for the business logic of the OAuth stuff. Before I add any unit tests I would like to know if you guys like what you see... (I prefer test driven design, but I really need to get things done now 😉 .)

This was referenced Sep 13, 2019
Merged
Merged
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Sep 23, 2019
@poikilotherm
Make scope checking after authorization multi-scope aware.
a51497f 
The list of authorized scopes doesn't have to be in the same order as
we send it. So better check the single scope names on their own.

Relates to IQSS#5991.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Sep 24, 2019
@poikilotherm
Remove scope attribute from OAuth2TokenData.
81b367b 
Since the introduction of OAuth2 the scope attribute
of all saved tokens has been "null", as there seemed to
be a bug with ScribeJava. Upgrading from v3.3.6 to v6.8.1
resulted in exceptions due to the scope being saved now,
but violating the 64 char limit.

As the persisted data has not been in use ever since
(the scope is always retrieved from the IdP implementation),
the attribute has been removed to save database space and
avoid the exception.

An appropriate SQL migration script for Flyway has been added.
Relates to IQSS#5991.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Sep 24, 2019
@poikilotherm
Refactor scope usage to make it optional.
3916966 
The GitHub auth provider had been implemented with no scope.
Thus only public information is used, the user needs to provide
his or her mail address on first login page.

Relates to IQSS#5991.
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Sep 25, 2019
@poikilotherm
Check for empty scope in response but non-empty scope in provider. Re…
5dc272c 
…lated to IQSS#5991.
@landreev
Copy link
Contributor

landreev commented Oct 18, 2019 •
edited
Loading

I'm seeing that the PR #5997 has just been merged. Why is the flyway script named V4.18.0.1__5991-update-scribejava.sql? - The current version is V4.17.
(not sure if it's worth the trouble renaming the script at this point; doing so would cause the usual problems on all the instances where develop has already been deployed... and since the script only has one "drop column" line, it may be safe to leave it as is... I can't think of a condition where it would case a problem - anyone?)

But let's be careful about keeping established numbering scheme going forward; which is to build on top of the current version in pom.xml.

@pdurbin
Copy link
Member

pdurbin commented Oct 18, 2019

@landreev d'oh! I do think we should rename the SQL script and I volunteer to do it. Sorry I missed this in code review. 😞 I just opened #6293 to fix it.

@poikilotherm poikilotherm changed the title Implementing a minimal OpenID Connect compliant OAuth2 Auth provider Upgrade ScribeJava to latest version Oct 31, 2019
@poikilotherm
Copy link
Contributor Author

Alright, lets close this as done. I'm opening new issues for the further work.

This was referenced Nov 11, 2019
Closed
Merged
@poikilotherm poikilotherm mentioned this issue Dec 4, 2019
Closed
@poikilotherm poikilotherm mentioned this issue Jan 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

5991 - Update ScribeJava to 6.6.3 and necessary refactoring poikilotherm/dataverse
3 participants
@pdurbin @landreev @poikilotherm