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

microsoft: option for group UUIDs instead of name and group whitelist #1446

Merged
merged 1 commit into from
Jul 25, 2019

Conversation

maksd
Copy link
Contributor

@maksd maksd commented May 8, 2019

This PR add a useGroupUUID option to return groups UUIDs instead of names. And, it change the behavior of the groups option to not only allow user member of a particular group but also act as a whitelist for the groups return in the claim.

The main reason for using group UUIDs, is that group names are not unique in Microsoft Azure AD.

@ericchiang
Copy link
Contributor

Can we make this an enum instead of a boolean? Something like:

groupNameFormat: id # defaults to name

Also are these actually UUIDs? (https://en.wikipedia.org/wiki/Universally_unique_identifier) That's a pretty specific format and I only see "id" referenced in the API 😃

@maksd
Copy link
Contributor Author

maksd commented May 14, 2019

@ericchiang Yes I can change it for an enum. About the "uuids", I took it from the Microsoft Graph documentation for the directoryObject resource type :

Property Type Description
id String A Guid that is the unique identifier for the object; for example, 12345678-9abc-def0-1234-56789abcde. Key. Not nullable. Read-only.

But I will change it for "id" instead.

Copy link
Contributor

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

please squash your commits 😃

connector/microsoft/microsoft.go Outdated Show resolved Hide resolved
connector/microsoft/microsoft.go Show resolved Hide resolved
@maksd maksd force-pushed the microsoft-groups-uuid-whitelist branch from 552cbd8 to 78088c6 Compare May 28, 2019 01:18
@srenatus
Copy link
Contributor

Could you rebase this once more, please? I had merged a change refactoring the group filtering into a helper method that affected this PR, sorry. 😃

@maksd maksd force-pushed the microsoft-groups-uuid-whitelist branch from 78088c6 to 754c38a Compare July 25, 2019 00:39
@maksd
Copy link
Contributor Author

maksd commented Jul 25, 2019

Hi @srenatus, rebase done.

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.

Thank you 🎉 That's a nice contribution.

Just one nitpick. I'm afraid this connector has no tests, but it's OK if you don't want to be the person reducing that tech debt. If, however, you'd like to add some, that would be very welcome 😉

connector/microsoft/microsoft.go Outdated Show resolved Hide resolved
@maksd maksd force-pushed the microsoft-groups-uuid-whitelist branch from d684c93 to 4585850 Compare July 25, 2019 13:15
@maksd
Copy link
Contributor Author

maksd commented Jul 25, 2019

@srenatus nit fixed, thanks for the catch. About the tests, I would be happy to do it, but it will have to wait a bit, maybe in another PR, or unless someone else beat me to it. :)

@srenatus srenatus merged commit 6e98c04 into dexidp:master Jul 25, 2019
mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
…list

microsoft: option for group UUIDs instead of name and group whitelist
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