-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fetch groups in a Google Connector #1185
Conversation
Just wanted to note I am presently testing this variation (plus #1180) and find it to be a valuable addition. What needs to happen to have the code mainlined? Happy to help in any way I can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly just nits.
you'll need to update the dependencies, we switched to Go modules since this PR was originally authored.
// Config holds configuration options for Google logins. | ||
type Config struct { | ||
ClientID string `json:"clientID"` | ||
ClientSecret string `json:"clientSecret"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can this be optionally read from a file? e.g. ClientSecretFile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this being done in any of the other connectors but could add this. I assume the ClientSecretFile
would just contain a single string? Or is there a format for files from Google containing the ClientSecret
?
c1b2a4f
to
84d6862
Compare
I've rebased and addressed a couple of your comments @ericchiang Took me a while to work out |
ping pretty please :) |
Still blocked on #1180, right? |
Still blocked on #1180 indeed, apologies all, will get back on to that once I'm recovered from KubeCon 😴 |
It would be awesome to have this feature in dex master! Really waiting for this to be merged! |
I'd like to test this w/ #1180 , any way we could see this rebased? |
b98860c
to
1227551
Compare
@jhohertz I've just rebased this branch, you should be able to test with 1180 with a little work |
Testing the rebased PR... This is working well along with #1180 What do we need to do to see this get merged? |
1227551
to
658a2cc
Compare
#1180 has been merged. @ericchiang @bonifaido Do you have some time to take a review? |
Just checking this. |
@srenatus @rithujohn191 -- Can you take a look at this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested locally, works great!
Fixes #866 #1001
This PR should not be used without #1180. Wanted to get the PR opened though in case anyone wants this feature urgently and can't wait until both PRs are merged.
I have created a separate Google connector that uses a service account to fetch all groups that a user is a member of.
I wasn't sure which documentation would need to be updated so I have added a document explaining the new parameters and how to set up the Google service account, but I reckon this will need extra documentation changes before it can be merged, let me know what it needs.