dexidp
Move unique functionality into getGroups to reduce calls to google
Overview
Removed the unique function and integrated into getGroups to reduce the number of calls to google.
What this PR does / why we need it
Depending on the groups a user belongs to the current code can make several extraneous calls to google. This can occur either when a user has several indirect memberships to a group or when the user has both an indirect and direct membership to a group:
# two extraneous calls to numbers
# email of user/group -> [groups email is a member of]
user -> [one, two, three]
one -> [numbers]
two -> [numbers]
three -> [numbers]
numbers -> []
# extraneous call to bar due to both direct and indirect membership
user -> [foo, bar]
foo -> [bar]
bar -> []
Implementing a standard map setup reduces calls to google down to number of groups + original user.
I'm not sure if google allows it, but it also eliminates a stack overflow if there was a circular membership:
user -> [one]
one -> [two]
two -> [three]
three -> [one]
Special notes for your reviewer
Does this PR introduce a user-facing change?
NONE
@nabokihms do I add a label or do you all? It wasn't mentioned in the PR template, but wasn't sure if you would get notified of the PR until it's there
Looks nice! Things that stop us from merging this PR:
1. A test. 2. [DCO](https://github.com/apps/dco).
I'll get on adding the test now. Had a production incident all day yesterday to handle but should be able to get to it the next couple of days.
For DCO, is that just you want the initial commit to have the Signed-off-by line like the 2nd commit currently has or is there something more to it that I missed from your link?
Do you mind if I add a few debug level log lines stating which email is being checked and later if the group exists? If yes, do you want me to use else or log and still use continue??
@ichbinfrog I reverted my google.go prior to me adding tests where I changed how some methods worked. After a mistake in merging master into my branch the tests now pass locally
@nabokihms I don't see a way to restart that one job and it appears to be unrelated to my change. Can you restart or merge anyways?
@nabokihms was there anything else you needed? I'd like to get this merged as soon as possible as I think I've seen some security releases and my company is currently running a custom build to fix this problem.
@sagikazarmark sorry if you're the wrong person. Saw you have lots of commits and modified my milestone. Who should I ask for review on this? Really need this feature and AFAIK all things are signed-off.
@snuggie12 sorry for the delay, and thank you for all your work and patience.
