Move unique functionality into getGroups to reduce calls to google

[snuggie12]

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

[snuggie12]

@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

[nabokihms]

Thanks for opening this PR. I left some questions. Could you please have a look?

P.S. Don't forget to sign your commits.

[nabokihms]

Is it possible to add a test that the number of API calls is reduced?

[snuggie12]

Yeah, I would like to add a test. Would I need to setup a mock for that? Is there an example?

Locally I have logs which demonstrate this. I could add that to the code instead.

[nabokihms]

Oh, there are no tests for Google connector at all...

You can use this code as an example to write a simple test.

[snuggie12]

The example looked easy enough in that you could send to whatever URL you want but the google connector is using a library and so I couldn't figure out how to do it in a similar fashion.

I attempted both interfaces and embedded structs but couldn't get around the issue of getGroups always calling its version of getGroupsList. In the end I settled on getGroups taking a function as a parameter and that worked.

Hopefully that is okay, because otherwise I fear I might need a lot of hand holding to get something working (not a developer by trade.)

[ichbinfrog]

@snuggie12, I've implemented some unit testing for your PR on my fork: https://github.com/ichbinfrog/dex/blob/reduce-admin-api-calls/connector/google/google_test.go.

Feel free to pull / take inspiration from the test file and reuse it since it also adds the required instrumentation for the amount of apiCalls.

TLDR: the basic gist of mocking google APIs is to inject a few client options which will force it to point to a local http server whose behavior you can control:

conn.adminSrv, err = admin.NewService(
   context.Background(), 
   option.WithoutAuthentication(), 
   option.WithEndpoint(ts.URL),
)

[snuggie12]

@snuggie12, I've implemented some unit testing for your PR on my fork: https://github.com/ichbinfrog/dex/blob/reduce-admin-api-calls/connector/google/google_test.go.

Feel free to pull / take inspiration from the test file and reuse it since it also adds the required instrumentation for the amount of apiCalls.

TLDR: the basic gist of mocking google APIs is to inject a few client options which will force it to point to a local http server whose behavior you can control:

conn.adminSrv, err = admin.NewService(
   context.Background(), 
   option.WithoutAuthentication(), 
   option.WithEndpoint(ts.URL),
)

@ichbinfrog thanks for this. I've been busy moving where I live but I'll def take a look soon.

[nabokihms]

NIT: I'd like to reduce the indentation level here, e.g.,

if _, exists := checkedGroups[group.Email]; exists {
    continue
}
...
if !fetchTransitiveGroupMembership {
    continue
}
...    

Nested conditions wrapped in two for loops are a little bit harder to read.

[unaryops]

Perhaps use some tools in the build pipeline that do static analysis of cognitive and cyclomatic complexity.

[nabokihms]

Oh, there are no tests for Google connector at all...

You can use this code as an example to write a simple test.

[nabokihms]

Looks nice! Things that stop us from merging this PR:

1. A test.
2. DCO.

[snuggie12]

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??

[snuggie12]

@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

[snuggie12]

@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?

[snuggie12]

@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.

[snuggie12]

@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.

[nabokihms]

@snuggie12 sorry for the delay, and thank you for all your work and patience.