-
Notifications
You must be signed in to change notification settings - Fork 73
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
global: add Keycloak contrib #217
global: add Keycloak contrib #217
Conversation
Hey @max-moser did anyone say they were going to follow-up on this PR? I am not familiar enough (yet!) to give it a fair review, but I think it would be super valuable. I will ping @ntarocco to see if he can (or knows who should) take a look. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, I am really sorry if it took very long time to review it. I hope it was not really blocking you.
Looks very good to me! Thanks a lot for this contribution, it is great! 👏
This can also replace the cern_openid
contrib. at some point, we will just need to "hook in" a way to perform some other actions after login and logout.
I have added a few minor comments, mostly cosmetic if you want to have a look.
If you then need to use this in a future release of invenio-oauthclient
, we can probably backport it to a previous version too, let us know.
Hey @ntarocco, thanks for the kind words and feedback! 🙂 I'll incorporate your feedback into the PR and refactor it a bit after the holidays. |
* Add support for using OAuth via Keycloak. * Add unit tests for the Keycloak integration. httpretty is used to mock a Keycloak server which returns actual data produced by a test instance of Keycloak, to increase meaningfulness of the unit tests.
* some tests were failing because the signature used for test tokens was expired
7f70318
to
208cb1d
Compare
Tests are:Passing with Postgresql
Failing with Mysql
Stacktrace:
Failing test run alone
|
The failing tests seem to be due to an accumulation of open connections - when I update the call to
|
With all tests enabled (respectively, none ignored), one of the tests fails:
[...]
[...]
|
No description provided.