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

Keycloak auth adapter #6376

Merged
merged 6 commits into from
Aug 31, 2020
Merged

Keycloak auth adapter #6376

merged 6 commits into from
Aug 31, 2020

Conversation

rhuanbarreto
Copy link
Contributor

Implemented a keycloak authentication adapter. I'm using it already for more than 6 months normally as a custom module. Keycloak runs connecting to 2 LDAP servers and provide a centralized ID management system with roles and groups mapping to LDAP groups.

@rhuanbarreto
Copy link
Contributor Author

I also left a PR in the docs repository for the documentation.

@codecov
Copy link

codecov bot commented Jan 28, 2020

Codecov Report

Merging #6376 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6376      +/-   ##
==========================================
- Coverage   93.82%   93.80%   -0.02%     
==========================================
  Files         168      169       +1     
  Lines       12190    12220      +30     
==========================================
+ Hits        11437    11463      +26     
- Misses        753      757       +4     
Impacted Files Coverage Δ
src/Adapters/Auth/index.js 93.54% <100.00%> (+0.10%) ⬆️
src/Adapters/Auth/keycloak.js 100.00% <100.00%> (ø)
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.92% <0.00%> (-0.89%) ⬇️
src/RestWrite.js 93.82% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4245a8f...964fd44. Read the comment docs.

@rhuanbarreto
Copy link
Contributor Author

@dplewis @flovilmart @davimacedo Can you merge?

@dplewis
Copy link
Member

dplewis commented Feb 8, 2020

@rhuanbarreto Sorry for the late reply. Can you add test cases? Everything looks good just want to make sure we have coverage.

@rhuanbarreto
Copy link
Contributor Author

The test cases are bound to have a keycloak running. Should I add something like this in the test suite?

@davimacedo
Copy link
Member

You can try to mock it. Or maybe use a "test endpoint". I am not sure they have one. You can refer to these tests: https://github.com/parse-community/parse-server/blob/master/spec/AuthenticationAdapters.spec.js

@rhuanbarreto
Copy link
Contributor Author

I made the change to increase coverage and on Travis the errors are not related to my code changes.

Can you please check @davimacedo @dplewis ?

@davimacedo
Copy link
Member

Could you please add more tests? Something like these ones.

@rhuanbarreto
Copy link
Contributor Author

Sorry. I don't have time anymore for adding these tests. If it's not enough, you can close the PR.

@dplewis
Copy link
Member

dplewis commented Mar 22, 2020

@rhuanbarreto I added a few tests cases. Can you review this?

@cbaker6 cbaker6 mentioned this pull request Apr 3, 2020
@rhuanbarreto
Copy link
Contributor Author

Tests are fine! You can merge.

@rhuanbarreto
Copy link
Contributor Author

@davimacedo @dplewis are you able to merge?

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@davimacedo davimacedo merged commit 6e36411 into parse-community:master Aug 31, 2020
@rhuanbarreto rhuanbarreto deleted the keycloak-auth-adapter branch August 31, 2020 10:01
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.

3 participants