8000 Allow override of baseUrl and apiUrl in GitHub identity provider by nngo · Pull Request #7021 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Allow override of baseUrl and apiUrl in GitHub identity provider #7021

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 6, 2022
Merged

Allow override of baseUrl and apiUrl in GitHub identity provider #7021

merged 4 commits into from
Apr 6, 2022

Conversation

nngo
Copy link
Contributor
@nngo nngo commented Apr 28, 2020 •
edited by stianst
Loading
  • This enhancement allows administrator to use the GitHub identity provider
    against a GitHub Enterprise server using an overridable/configurable fields for:
    • baseUrl (to set Authentication URL and token URL)
    • apiUrl (to set profileUrl and emailUrl (search email).
  • Add partial github-ext.html fragment to allow user to override the
    GitHub identity provider's base URL and api URL (this allows using
    a custom GitHub Enterprise server.
    • using generic identity-provider.base-url* and identity-provider.api-url*
      in admin-messages_en.properties and html fragment so that these
      could also be used for other identity providers.

keycloak-github-ent-1-collapsed
keycloak-github-ent-2-expanded

Closes #11144

@nngo nngo force-pushed the KEYCLOAK-13828-github-enterprise-intergration branch from 1cdc177 to 63694e5 Compare April 29, 2020 11:55
@stianst stianst added the missing/tests Tests are missing label Apr 30, 2020
@nngo nngo force-pushed the KEYCLOAK-13828-github-enterprise-intergration branch from 63694e5 to 2663784 Compare April 30, 2020 13:16
@nngo
Copy link
Contributor Author
nngo commented Apr 30, 2020

Hi @stianst, I pushed updated commit to add unit test for my suggested changes. Please review.

@nngo
Copy link
Contributor Author
nngo commented Apr 30, 2020

This PR is for https://issues.redhat.com/browse/KEYCLOAK-13828

@nngo
Copy link
Contributor Author
nngo commented May 11, 2020

@stianst Is there anything else I need to get this PR approved and merged?

@stianst
Copy link
Contributor
stianst commented May 19, 2020

At first glance this looks good, we just need to find someone to do a deeper review for this one

@nngo
Copy link
Contributor Author
nngo commented Jun 2, 2020

@stianst any traction on getting a deeper review of this PR? Maybe @EricWittmann could help? this change is pretty straight forward, but then I am biased.

@EricWittmann
Copy link
Contributor

Looks pretty good to me. It would be good to make the same changes for GitLab as well (along with any other identity providers that have "enteprise" versions). Not sure how much of deep dive is needed here - the test is reasonable.

@stianst stianst requested a review from vmuzikar June 15, 2020 11:25
@vmuzikar
Copy link
Contributor

Will try to review by the end of this week.

vmuzikar
vmuzikar requested changes Jun 23, 2020
View reviewed changes
Copy link
Contributor
@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

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

@nngo Thank you for your contribution. Added a few comments.

On top of that, I think we should also mention this new option in the docs.

@abstractj abstractj added kind/enhancement Categorizes a PR related to an enhancement impact/medium status/in-review labels Jun 23, 2020
@abstractj abstractj requested a review from vmuzikar July 8, 2020 12:39
@nngo nngo force-pushed the KEYCLOAK-13828-github-enterprise-intergration branch from 2663784 to 722323f Compare July 8, 2020 13:37
@vmuzikar
Copy link
Contributor

@nngo I'm sorry for delayed response. Added a few comments.

@nngo
KEYCLOAK-13828 Allow override of baseUrl & apiUrl in GitHub identity …
2aa5c0d 
…provider

* Add partial github-ext.html fragment to allow user to override the
  GitHub identity provider's base URL and api URL (this allows using
  a custom GitHub Enterprise server.
  * using generic identity-provider.base-url* and identity-provider.api-url*
    in admin-messages_en.properties so that this could also be used
    for other identity providers.
* add new DEFAUL_*_URL public constants
* Add GitHubIdentityProviderTest unit test
@nngo nngo force-pushed the KEYCLOAK-13828-github-enterprise-intergration branch from 722323f to 2aa5c0d Compare July 10, 2020 13:02
@abstractj
Copy link
Contributor

@nngo would you like to move forward with these changes? If yes, please iterate over @vmuzikar comments.

@rajivmulajker
Copy link

Hi Appreciate any feedback on when this functionality will be included. It's stopping us from deploying keycloak and would really appreciate if we have a timeline on when this can be released. Thank you.

@abstractj
Copy link
Contributor

@nngo we need to follow up on @vmuzikar to move forward with this PR. Are you still interested in moving forward?

@nngo
Copy link
Contributor Author
nngo commented Oct 1, 2021

yes, I believe I have already responded to all the comments. I can merge (or rebase) from the latest origin/master branch if you want.

vmuzikar
vmuzikar requested changes Oct 1, 2021
View reviewed changes
Copy link
Contributor
@vmuzikar vmuzikar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@nngo Thank you for your response!

From my PoV, in order to merge this we need to:

  1. Rebase.
  2. Fix the UI issue – probably remove the override section and put the new fields directly among other fields (see below).
  3. Remove the empty test method (see below).
  4. Add a simple social login test. Something like: add GitHub IdP with overridden URLs and check it really tries to access those URLs (maybe non-existing because we do not have access to GitHub Enterprise). Nothing fancy.

@abstractj abstractj requested a review from vmuzikar October 7, 2021 18:10
@abstractj
Copy link
Contributor

@nngo would you like to move forward with this change?

@nngo
Remove empty testGetProfileEndpointForValidation() unit test
81cc80f 
* it is tested in testGitHubIdentityProvider()
@nngo
Copy link
Contributor Author
nngo commented Jan 11, 2022

here's the updated screen shot for setting the GitHub Enterprise Base URL and API URL
Screenshot 2022-01-10 at 14-00-31 Keycloak Admin Console
t

@nehachopra27
Copy link
Contributor
nehachopra27 commented Mar 8, 2022 •
edited
Loading

The the GitHub Enterprise Base URL and API URL works fine and has been tested using trial version of Github Enterprise Server hosted on Google cloud (trial version). Since the access is not permanent, these tests can not be automated. The changes seems good to go.

KeycloakSetup

GithubEnterpriseLoginScreen

SuccesfulLogin

@stianst stianst mentioned this pull request Apr 5, 2022
Closed
29 tasks
vmuzikar
vmuzikar approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor
@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

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

@nngo Thank you for the update and sorry for late response. LGTM.
And thanks to @nehachopra27 for testing it!

@abstractj abstractj closed this Apr 5, 2022
@abstractj abstractj reopened this Apr 5, 2022
@abstractj
Copy link
Contributor

@vmuzikar @nngo retriggered the jobs to make sure they all pass, after that, we should be able to merge.

@abstractj abstractj added this to the 18.0.0 milestone Apr 5, 2022
@nngo
Copy link
Contributor Author
nngo commented Apr 5, 2022

@vmuzikar @nngo retriggered the jobs to make sure they all pass, after that, we should be able to merge.

thanks @abstractj looking forward to this getting into the next Keycloak release

@stianst stianst merged commit f11573e into keycloak:main Apr 6, 2022
@stianst stianst changed the title KEYCLOAK-13828 Allow override of baseUrl and apiUrl in GitHub identity provider Allow override of baseUrl and apiUrl in GitHub identity provider Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stianst stianst stianst approved these changes

@vmuzikar vmuzikar vmuzikar approved these changes

Assignees

@abstractj abstractj

Labels
impact/medium kind/enhancement Categorizes a PR related to an enhancement missing/tests Tests are missing
Projects
None yet
Milestone
18.0.0
Development

Successfully merging this pull request may close these issues.

Identity brokering support for GitHub Enterprise server
7 participants
@nngo @stianst @EricWittmann @vmuzikar @abstractj @rajivmulajker @nehachopra27
0