-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add Documentation for setting up upstream IAMs with Keycloak #419
Conversation
Signed-off-by: MohammedAbdi <mohammma@usc.edu>
Signed-off-by: MohammedAbdi <mohammma@usc.edu>
Signed-off-by: MohammedAbdi <mohammma@usc.edu>
Signed-off-by: MohammedAbdi <mohammma@usc.edu>
Signed-off-by: MohammedAbdi <mohammma@usc.edu>
Signed-off-by: MohammedAbdi <mohammma@usc.edu>
Signed-off-by: MohammedAbdi <mohammma@usc.edu>
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.
Left initial review. I followed the instructions for both Azure and Github and do not get the desired result. For Azure, it properly redirects me, but I cannot login with a user I need.
For Github, it appears to attempt a redirect, but gives some error encoded in the URL about a redirect mismatch:
...error%3Dredirect_uri_mismatch%26error_description%3DThe%2Bredirect_uri%2BMUST%2Bmatch%2Bthe%2Bregistered%2Bcallback%2BURL%2Bfor%2Bthis%2Bapplication.%26error_uri%3Dhttps%253A%252F%252Fdocs.github.com%252Fapps%252Fmanaging-oauth-apps%252Ftroubleshooting-authorization-request-errors%252F%2523redirect-uri-mismatch...
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 retested it, and I was able to get it to work - except Microsoft one requires you to select "Multitenant" so this detail needs to be added I think. I would add the details of a filled in form for each of keycloak and the upstream IDPs so that these details can be explained away
Also having gone through this I'm noticing the keycloak steps are pretty much always the same - maybe we can shorten the documentation. My suggestion would be to
- put the keycloak documentation at the top
a. State where the relevant Redirect URI is (with the screen shot)
b. Tell users to fill in the Client ID and Client Secret after obtaining them from the the desired IDP below - For each IDP create a collapsible section for readers to click into.
a. Talk about how to get to the relevant form to fill in Redirect URI
b. Show a filled in picture of the form with the important selections
While Keycloak is technically the last step, I think this might be clear enough.
> [!NOTE] For simplicity we will be creating a free account | ||
- Go to [Microsoft Azure](https://azure.microsoft.com/en-us/) and select `Try Azure for free` signup for an account. | ||
- Go to `App registrations` (you can search for it on top) | ||
- Click on `New Regestration` and configure the name and add a `Redirect URI` by selecting `Web` and paste the value of `Redirect URI` from keycloak and Register the application. |
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.
On this step I got it to work, but I also needed to select multitenant
option. Add this detail when screenshooting?
Signed-off-by: MohammedAbdi <mohammma@usc.edu>
Signed-off-by: MohammedAbdi <mohammma@usc.edu>
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.
This is very clean! So I have a minor nit comment for the lines that go:
> [!TIP]
I think do a sweep through because some of them haven't put the rest of the text on the next line in order to parse properly
I also do not see the TenantID field in Keycloak for Azure. Are you using the example in Tornjak repo or the helm charts? I'm using the example in Tornjak, so maybe it needs to be updated either with the right config. Probably can't remove the example in Tornjak repo to point to helm charts right now because that has the changes for the latest version of Tornjak to work with Keycloak. If you're using the helm charts, maybe make a note at the top about this should be sufficient?
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.
LGTM! Really great documentation!
Added documentation for setting up upstream IAMs with Keycloak
To do & enhancements