Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

[KEYCLOAK-11499] Add idp-hint as a configuration option #495

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

moremagic
Copy link

Add idp-hint as a configuration option, so that gatekeeper can set it's own suggested idp.

@moremagic moremagic changed the title KEYCLOAK-11499 [KEYCLOAK-11499] Add idp-hint as a configuration option Dec 10, 2019
@abstractj abstractj self-assigned this Jan 11, 2020
Copy link

@abstractj abstractj left a comment

Choose a reason for hiding this comment

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

@moremagic I believe this change makes sense, wdyt @stianst?. We need two things here:

  1. Tests for the new parameter
  2. Update our documentation: https://github.com/keycloak/keycloak-documentation/blob/9194cbe2c4f443ddb4ab012cc9902fc3759bdc6a/securing_apps/topics/oidc/keycloak-gatekeeper.adoc

Please let me know if you would like to proceed, otherwise we may need to schedule the full implementation for the next releases. If you have any questions, feel free to ask.

@msuret
Copy link
Contributor

msuret commented Jan 22, 2020

It would be useful if idp-hint could also be dynamically deduced from the host, e.g. accessing the app from tenant1.myapp.com would set idp_hint to tenant1. What do you think ? I can work on this.

@abstractj
Copy link

It would be useful if idp-hint could also be dynamically deduced from the host, e.g. accessing the app from tenant1.myapp.com would set idp_hint to tenant1. What do you think ? I can work on this.

Thanks for volunteering to work on this @msuret. Before start working on anything, I'd suggest starting a thread on the dev mailing list, so others can join and provide some feedback on this.
Also, I didn't have the chance to look at other adapters, but we need to make sure that Gatekeeper has the same behavior as other Keycloak adapters.

// idp_hint config
idpHint := r.config.IdpHint
if idpHint != "" {
authURL += "&kc_idp_hint=" + idpHint

Choose a reason for hiding this comment

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

Could there sometimes be other url parameters already in authURL? I am thinking "&" vs "?"
If that is never a possibility please disregard, otherwise I suggest adding some pre-inspection of authURL before appending.

@rickyschechter
Copy link

Thank you @abstractj and @moremagic ! The change looks simple enough.

Line (91) I added a comment. I agree that tests are missing still.

Could you please point me at where I can download the built artifact? I could probably test it manually and report based on my use/findings.

@abstractj
Copy link

@moremagic would you like to follow up on @rickyschechter's comments? Do you have any plans to introduce tests?

@abstractj abstractj added the incomplete Not complete implementation or work in progress label Jun 19, 2020
@abstractj abstractj removed their assignment Aug 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
incomplete Not complete implementation or work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants