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

fix: move oidc scope logic to oidc provider #1989

Conversation

tuunit
Copy link
Member

@tuunit tuunit commented Jan 26, 2023

Description and Motivation

The OIDC provider shouldn't be treated as anything specially where as all the other providers have there internal scope logic defined in the specific provider files the scope logic for oidc and in extension the keycloak-oidc provider is located in the providers.go.

To align the OIDC and Keycloak OIDC provider I moved the logic to the oidc.go provider file.

How Has This Been Tested?

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@tuunit tuunit requested a review from a team as a code owner January 26, 2023 21:18
@tuunit tuunit changed the title fix: add default scope to keycloak oidc provider [WIP] fix: add default scope to keycloak oidc provider Jan 26, 2023
@tuunit tuunit changed the title [WIP] fix: add default scope to keycloak oidc provider [WIP] fix: move oidc scope logic to oidc provider Jan 26, 2023
@tuunit
Copy link
Member Author

tuunit commented Jan 26, 2023

@Kyserbyte I tested your issue with the scope missing for the KeycloakOIDC provider and couldn't produce it. As the KeycloakOIDC provider is a extension of the OIDC provider it should have the same default scope of "openid email profile". Nevertheless, I think it is a good idea to move this logic from the generic provider.go to the OIDC provider.

}
}
if providerConfig.OIDCConfig.UserIDClaim == "" {
providerConfig.OIDCConfig.UserIDClaim = "email"
Copy link
Member Author

@tuunit tuunit Jan 26, 2023

Choose a reason for hiding this comment

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

As this struct instance is not a reference and not used after this line it was essentially just dead code.

@tuunit tuunit force-pushed the feature/add-default-scope-to-keycloak-oidc branch from 8d90b60 to a907c7d Compare February 13, 2023 20:18
@tuunit tuunit changed the title [WIP] fix: move oidc scope logic to oidc provider fix: move oidc scope logic to oidc provider Feb 13, 2023
@github-actions
Copy link
Contributor

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

@github-actions github-actions bot added the Stale label Apr 15, 2023
@github-actions github-actions bot closed this Apr 23, 2023
@JoelSpeed JoelSpeed reopened this Apr 24, 2023
@JoelSpeed JoelSpeed removed the Stale label Apr 24, 2023
@github-actions
Copy link
Contributor

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

@github-actions github-actions bot added the Stale label Jul 12, 2023
@github-actions github-actions bot closed this Jul 20, 2023
@tuunit
Copy link
Member Author

tuunit commented Aug 24, 2023

@JoelSpeed please reopen :)

@JoelSpeed
Copy link
Member

The branch has since been force pushed, so I can't currently reopen right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants