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

add preffered_username to idToken #1566

Merged
merged 2 commits into from
Oct 30, 2019
Merged

add preffered_username to idToken #1566

merged 2 commits into from
Oct 30, 2019

Conversation

bonifaido
Copy link
Member

@bonifaido bonifaido commented Oct 10, 2019

Signed-off-by: Nandor Kracser [email protected]

Implemented preferred_username in ID Token for:

  • GitHub
  • GitLab
  • LDAP

TODO:

  • update docs after this gets finalized

Fixes: #1076

@bonifaido bonifaido self-assigned this Oct 11, 2019
@bonifaido bonifaido marked this pull request as ready for review October 11, 2019 12:08
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.

Some notes after a quick glance 👀

connector/ldap/ldap.go Outdated Show resolved Hide resolved
server/handlers.go Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Haven't checked the storage part. The rest looks good to me, except one detail.

server/handlers.go Outdated Show resolved Hide resolved
@bonifaido
Copy link
Member Author

Changes are done (as requested).

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM; haven't really looked at storage changes though.

@ghost
Copy link

ghost commented Oct 23, 2019

I built a docker container of this branch and tested it (sqlite3 storage, LDAP provider). Works fine!

@bonifaido
Copy link
Member Author

Do you think we are okay to merge this @srenatus ?

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.

Not blocking this. Thanks for the work here, everyone. 😃

server/handlers.go Outdated Show resolved Hide resolved
Co-Authored-By: Felix Fontein <[email protected]>
@bonifaido bonifaido merged commit d5d3abc into master Oct 30, 2019
@bonifaido bonifaido deleted the preferred_username branch October 30, 2019 12:25
@ghost
Copy link

ghost commented Oct 30, 2019

Thanks a lot for implementing this! I'm already using it in production and this was something that was really missing (for me) :)

@bonifaido
Copy link
Member Author

Great to hear that, welcome! We also missed it, and will introduce it in prod in the coming days :)

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

Successfully merging this pull request may close these issues.

Github doesn't return login - Feature Request
3 participants