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.

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

Allow preferred_username claim to be set for Crowd connector #1684

Merged
merged 7 commits into from
Apr 23, 2020
Merged

Allow preferred_username claim to be set for Crowd connector #1684

merged 7 commits into from
Apr 23, 2020

Conversation

mvdkleijn
Copy link
Contributor

What this is
In the current implementation, the preferred_username claim remains empty. Some clients might require that claim and Dex supports it, if set from the connector's side.

This PR allows the connector to optionally support the preferred_username claim. It achieves this by allowing a user to choose to fill the preferred_username claim by one of the available Crowd fields. Specifically: key, name or email. Invalid options cause the claim to remain empty.

Breaking change: No. Existing configuration should not notice this change.
Optional: Yes.

Notes:

  • Added test for identityFromCrowdUser function which was not extensively tested before
  • Added entry for the Crowd connector in the readme
  • Updated documentation for the connector
  • I'm fairly new to Go, so I welcome constructive feedback

@mvdkleijn
Copy link
Contributor Author

Hi @srenatus @bonifaido @sagikazarmark - I was wondering if I could get some feedback on what this still needs or possibly just have this approved... It's for the recently added Crowd connector. 😃

Passes the checks and latest changes from master were merged in.

@nabokihms
Copy link
Member

@mvdkleijn thanks a lot for your work. This is wonderful! It would be great to get it merged.

@mvdkleijn
Copy link
Contributor Author

oof... a minute or two for the code change and 10-15 minutes just to get the PR updated. Bad timing for stability issues in Github. :)

@mvdkleijn
Copy link
Contributor Author

The build / checks failed due to stability issues with Github. Can they be re-triggered manually or do I need to push a dummy commit?

@bonifaido
Copy link
Member

bonifaido commented Apr 23, 2020 •
edited
Loading

just triggered it again, github is having a hard time these days

@mvdkleijn
Copy link
Contributor Author

Thanks for the retrigger @bonifaido Sadly Github is still having issues. I'll check back later. This PR should be good to merge anyway. I don't believe this needs anything else and the change is backwards compatible.

bonifaido
bonifaido approved these changes Apr 23, 2020
View reviewed changes
Copy link
Member

@bonifaido bonifaido left a comment

Choose a reason for hiding this comment

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

Just managed to retrigger again, now it is green again, it LGTM!

@bonifaido bonifaido merged commit 0a85a97 into dexidp:master Apr 23, 2020
@mvdkleijn mvdkleijn deleted the feature-preferred-username-field branch April 23, 2020 19:06
@mvdkleijn mvdkleijn mentioned this pull request Jul 30, 2020
Merged
elffjs pushed a commit to DIMO-Network/dex that referenced this pull request Jun 27, 2022
@mvdkleijn @elffjs
Allow preferred_username claim to be set for Crowd connector (dexidp#…
d0e000b 
…1684)

* Add atlassiancrowd connector to list in readme

* Add TestIdentityFromCrowdUser

* Set preferred_username claim when configured

* Add preferredUsernameField option to docs

* Log warning when mapping invalid crowd field
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabokihms nabokihms nabokihms left review comments

@bonifaido bonifaido bonifaido approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mvdkleijn @nabokihms @bonifaido