5991 - Update ScribeJava to 6.6.3 and necessary refactoring #5997
Conversation
…mpatible with recent ScribeJava library. Refactored code structure a bit, too.
…actor base class to avoid code duplication when generating the user record.
|
@poikilotherm as we're discussing at http://irclog.iq.harvard.edu/dataverse/2019-07-08#i_99911 I just left a comment at #5991 (comment) . I just took a quick look at the code and it's not immediately clear why the necessary refactoring is necessary. Can you please explain? I'm going to leave this in code review and try to get some "talk after" time after standup. |
|
The refactoring is necessary due to upstream changes. ScribeJava has moved some classes around, deprecated (and dropped) some things usable with v3.1.0. |
poikilotherm
changed the title
|
I just removed the "WIP" from the title as requested by @pdurbin on IRC. |
|
@poikilotherm - I assigned this to myself at standup and we'll put this on hold until I can review #5974 with the team. |
|
@sekmiller @pdurbin @kcondon and I talked about this post-standup in regards to testing. Testing should be focused on whether the OAuth methods (ORCID, Github, Google) still work. |
|
I am happy to do the heavy lifting with this and share my test results. I hoped for some technical discussion first about design and usability of the new code. |
|
Thanks @poikilotherm for the offer to help with the heavy lifting. @sekmiller is reviewing the code and will be able to provide any feedback. Is there anything specific about the approach that you'd like guidance on? |
|
@sekmiller mentioned at standup that he'd like someone with more familiarity with the auth code to take a look at what's done so far. @scolapasta I'll assign to you so you can either be that person or work with that person to keep this moving. Thank you! |
…xt". This is not usable on Glassfish 4.1. This reverts commit 55a2712.
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.
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.
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.
|
Alright, I tested this refactored code against GitHub, Google and ORCID sandbox. It all works flawless, affiliations are read from ORCID. Should be fine. Just those text strings left over. |
|
@poikilotherm thanks for keeping those strings in mind. When you're ready for this to go back into code review, please let us know! |
|
All set. Good night guys 😴 (Should we talk about adding the credentials for all the services to the repo? They are only usable with http://localhost:8080 anyway, so not very cool for abusing 😉 ) |
|
Thank you guys (@pdurbin, @scolapasta, @djbrooke, @kcondon et al) for merging. As always, it has been a pleasure 😄 |
closes #5991
This PR aims to provide a first, smaller step by updating the ScribeJava OAuth2 lib to a recent version.
Related Issues
Pull Request Checklist
Notable changes for QA (WIP, more to come):
scopeattribute fromOAuth2TokenDatain 81b367b. See commit comment for more information.TODOS