Conversation
….getUserRecord().
…abstracted provider. Added unit test, too.
…ed Faces context. Enhance readability.
|
Looks like upgrading to JUnit 5.5 makes test cases unhappy. Investigating. |
…ngs. IDEs might give us headaches and it ensures proper Maven configuration.
|
Ok, fixed as of 9f594b9 : reseting the |
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
@poikilotherm as we've been discussing in IRC, this looks good but our plan is for you to add OIDC support, probably in a new branch. We can leave this one around as a smaller branch for code reviewers to look at.
|
I got pretty far with OIDC yesterday. Already have it linked in the UI, building the URl for redirect works, too. Next up is retrieving user info. PR forthcoming, probably creating another small issue from the epic #5974 like we did here. |
|
@poikilotherm we discussed this a bit after standup today. We're willing to move this forward as refactoring, but to help us with testing, can you provide some guidance about areas of the code that had heavy changes and areas that you see as particularly complex and that have more risk? Thank you! |
|
@pdurbin asked me to add an update in a comment. Please see my notes in #6226 about this, too. 🤔 Based on what I changed, I suggest testing the following:
Futher comments:
|
|
Jesus, I just saw that unit test of mine fails now after pushing latest develop. Let me sort that out. |
|
Ok I cannot seem to fail this locally, but its flaky on Travis. This leads me to the impression that those pesky Consequence: be a good refactor, remove them and replace with proper |
…tem.currentTimeMillis(). Using CDI injection of clock via ClockUtil.
…y due to clock resolution of >1msec when using createState() and parseStateFromRequest().
|
Ok test flakiness fixed since db1e8e7. Reran the CI job on Travis 4 times (before I broke the tests pretty fast on first try) I'll add some notes to the testing scenarios above before sending to QA. @pdurbin please do a quick code review of my last changes and then push to QA at will... |
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
The code looks good and the API test suite is passing according to https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-6365/9/
|
@kcondon please see the "Based on what I changed, I suggest testing the following" note from @poikilotherm at #6365 (comment) |
|
Thanks @poikilotherm for adding the additional details! |
|
@poikilotherm I'm getting an exception in the UI and log when I attempt log in with Google. This works on develop. Here is the stack trace: [2019-11-22T15:57:25.126-0500] [glassfish 4.1] [WARNING] [] [javax.enterprise.resource.webcontainer.jsf.lifecycle] [tid: _ThreadID=54 ThreadName=jk-connector(2)] [timeMillis: 1574456245126] [levelValue: 900] [[ |
… the access token during code flow.
|
Ok @kcondon fixed as of 9a4b49f. Background: following https://developer.okta.com/blog/2018/04/10/oauth-authorization-code-grant-type#exchange-the-authorization-code-for-an-access-token, some providers (like Google) require the redirect_uri parameter also when requesting the access token (despite it wouldn't be necessary). |
|
@poikilotherm thanks for fixing that bug. Your commit makes sense. Thanks also for giving us a preview of the next pull request where you're adding the OIDC feature! This "diff" is looking good! poikilotherm/dataverse@6364-oauth2-abstract...poikilotherm:5974-oidc-impl |
|
@poikilotherm Thanks for the fix and the explanation. |
5 participants
This has been tested live with GitHub provider on local cluster. As the underlying infrastructure has not changed since #5991 and #6155, it shouldn't be necessary to test all providers again (but as always: up to you).
One might argue that
AbstractOAuth2AuthenticationProvidershould be refactored into an interface, too. This is left for later, as @djbrooke and @pdurbin do like the smallest scope possible.Please see also my comment below about what should be looked at for QA.
Related Issues
Pull Request Checklist