-
Notifications
You must be signed in to change notification settings - Fork 1
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
Migrate to keycloak #413
Migrate to keycloak #413
Conversation
In order to do this we needed to add the ueberauth_oidc library which then involved updating a few other deps including Phoenix. But this should now enable a user with the correct role logging into the app via keycloak. It will reject any user who does not have the correct role.
ad5ec4c
to
ad53b4d
Compare
Posting here for prosperity: My understanding is roles are created per client (each application has a keycloak client) and if roles are named the same between clients there is no clash because the roles only come in during authentication with that particular client . Either way, I think we are good with the role name we decided ( |
…cture of extra in Fake keycloak strategy
@Adzz I made some changes here, mostly to fix some warnings that came up failing the build and test step and making sure it integrates well with Keycloak DEV (looks like it worked well for me). Last steps here are to fix up the tests happy to work on that today |
Hey no worry the tests are a nightmare so I can crack that out now, thanks for the contribution though! |
I'm still reviewing some other stuff in this PR, but it looks like its failing and I have one important thing that I think will need to be done to successfully work in deployed environments: can you add in And the error that happens if you do not do this: https://www.notion.so/mbta-downtown-crossing/Implementing-a-Keycloak-integration-ebf669208d464986b4989294891b1d8e?pvs=4#49283e6e8bc0478e965a416f8cc5e8d8 This happened while I was testing APA and suspect it would happen here too. |
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.
Looks good, I think we have to enable the one job before we merge though
lib/document_viewer/application.ex
Outdated
@@ -11,7 +11,8 @@ defmodule DocumentViewer.Application do | |||
DocumentViewerWeb.Telemetry, | |||
# Start the PubSub system | |||
{Phoenix.PubSub, name: DocumentViewer.PubSub}, | |||
Catalog.Supervisor, | |||
# commenting out so I can start the app or attempt to. | |||
# Catalog.Supervisor, |
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.
we need this enabled when we merge in
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.
oh yes great spot! will change
end) | ||
|
||
assert log =~ "Ueberauth error: failed" | ||
assert log =~ "Ueberauth error: Cross-Site Request Forgery attack" |
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.
hahah, I don't like it but I was having trouble with this yesterday anyhow.
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.
I think ultimately we are trying to test what happens when we get an error in ueberauth, which this does so I'm fine with it.
ad574f5
to
ad55b88
Compare
ad55b88
to
ad5ab68
Compare
in order to add
ueberauth_oidcc
I had to updatetelemetry_poller
which triggered updating phoenix.This then migrates to using keycloak and updates the tests for that use case.