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

Implement the Locale claim for the OIDC connector #1535

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

justin-slowik
Copy link
Contributor

@justin-slowik justin-slowik commented Aug 28, 2019

the .well-known/openid-configuration return of Dex was displaying that "locale" was a supported claim when in fact it was not. My project requires locale, so I implemented the claim for the OIDC Connector and Storage methods.

This is my first deep dive into Go so please let me know if i missed anything

storage/sql/migrate.go Outdated Show resolved Hide resolved
@srenatus
Copy link
Contributor

💯 Thank you for contributing! I've not reviewed this thoroughly yet, just pointed out one thing amiss wrt. migrations. I'll try to review this soon.

@srenatus srenatus self-requested a review August 29, 2019 14:48
examples/config-dev.yaml Outdated Show resolved Hide resolved
@justin-slowik
Copy link
Contributor Author

Hi @srenatus , I was just curious if you've had a chance to take another look at this PR since I addressed your comments. Would love to get this in and then I could get working on zoneinfo next. Thanks!

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Went through this a bit more thoroughly this time. Thanks for your contribution, and sorry for dropping the ball a bit! 😃

connector/oidc/oidc.go Show resolved Hide resolved
storage/sql/migrate.go Outdated Show resolved Hide resolved
storage/sql/crud.go Outdated Show resolved Hide resolved
storage/conformance/conformance.go Show resolved Hide resolved
Modified Conformance unit tests to not have Locale in secondary use cases.   Removed the "not null default '' " from the migrate statements.
@justin-slowik
Copy link
Contributor Author

@srenatus in each of the conformance tests that utilize claims (testAuthCodeCRUD, testAuthRequestCRUD, and testRefreshTokenCRUD), I removed the locale claim from the secondary creates for each (a2 / refresh2 respectively) and verified that the tests still passed successfully.

Long term it may be worth making a test / tests that exercises all claims one at a time to be more complete, but i think thats out of the scope of this PR.

@justin-slowik justin-slowik changed the title Implement the Locale claim for the OIDC connector Implement the Locale/ZoneInfo claims for the OIDC connector Sep 19, 2019
@justin-slowik justin-slowik changed the title Implement the Locale/ZoneInfo claims for the OIDC connector Implement the Locale claim for the OIDC connector Sep 19, 2019
@justin-slowik
Copy link
Contributor Author

justin-slowik commented Sep 19, 2019

@srenatus Ignore my last comment/commit (since deleted and reverted). Accidentally pushed to the wrong branch. Anything else that needs to be done here?

@srenatus
Copy link
Contributor

@justin-slowik I'd hope @JoelSpeed or @bonifaido would pick up reviewing and merging this. I have no bandwidth left at the moment, I'm afraid. (I could merge this, but I cannot find time to deal with any fallout, so I cannot actually merge this 😅)

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

Successfully merging this pull request may close these issues.

3 participants