PTAL @lucarval @mprahl

You probably want to except on ldap.SERVER_DOWN to raise a bad gateway exception.

The problem with this query is that it does not include the posixGroup objects the user is a member of. It does seem to include the Rover groups though.

If you only need to care about the posixGroup membership, then you can use the following query:

results = con.search_s('ou=Groups,dc=example,dc=com', ldap.SCOPE_SUBTREE, f'(memberUid={user})', ['cn'])
group_cns = [group[1]['cn'] for group in results]

If you need the Rover groups and the traditional posixGroups, then you may have to perform both queries.

@mikeb, do you have any suggestions?

I'd prefer if you did:

if not current_app.config['PERMISSION_MAPPING']:
    return True
......

None is not required here since by default, get will return None if the key isn't present.

It's a bit simpler if you do:

if not (current_app.config.get('LDAP_HOST') and current_app.config.get('LDAP_BASE')):

Can you break from the loop after the first match? Is there a reason not to?

Instead of keeping track of the allowed_users list, you could just check:

if user in permission['users']:
    return True

The extra set of parentheses aren't needed. Also, it'd be nice if you were consistent with the quotes type you used. Mixing between single quotes and double quotes is a bit annoying.

How about?

group = item.split(',')[0][len('cn='):]

The f is not necessary in this string.

Perhaps a better name like verify_authorization or assert_authorized since you aren't actually returning anything, and you raise exceptions when they aren't authorized.

How about a better name like get_group_membership? You could also parse the results and just return a set of the common names of the groups here instead of in get_permissions.

Where are you mocking python-ldap? Is this unit test actually trying to connect to some LDAP server and failing?

No, that's the point of the test: test that without a valid LDAP server it will fail connecting.
The "monkeypatch" input is an error. I just forgot to remove it.

Not sure. Let's way for @mikeb or @lucarval.

I want allowed_groups and allowed_users to include all possible allowed groups and users.
The user might not be listed in the first match. I might forbid a user that is allowed but is listed after the first match.

oh... my beautiful regexp....

@mprahl I think I've addressed all the comments. Could you have another look?

You should mock the ldap library so that it raises an exception rather than have it try to reach out to ldap.something.com. I don't like the idea of the unit tests trying to connect to some random server on the internet.

How about logging the exception?

How about logging the exception?

Optional: The LDAP server seems to be unreachable. => The LDAP server is not reachable

Optional: This is a good candidate for a set

Perhaps the return value should be formatted to be a list/set of item[0].decode('utf-8').
Then the for loop can be replaced with:

if set(result) & set(allowed_groups):
    return True

Optional: A better variable name such as group_membership is appreciated for readability

Do you mean the return value of get_group_membership?

Yes

@mprahl all comments should be addressed. Thanks for the reviews.

Optional: It'd be better to mock the ldap.initialize method to raise the exception

:thumbsup:

Is this error propagated to client? Why re-throw this at all?

Any way to make the noqa comment clearer? No idea why those are used most of the time.

Because that log I think is a bit nicer than the traceback.

Flake8 doesn't handle correctly f-strings... I don't think putting the error code would be clearer in this case.

Flake8 doesn't handle correctly f-strings... I don't think putting the error code would be clearer in this case.

So everytime you use fstring, you also add one noqa? I'm not sure if using newer python features is worth it. :) Hmm, maybe get rid of flake8 and use pylint.

+1 But it looks like there should be a library for authentication that works nicely with flake and LDAP.

So everytime you use fstring, you also add one noqa? I'm not sure if using newer python features is worth it. :) Hmm, maybe get rid of flake8 and use pylint.

It doesn't seem to happen everytime. I'm just confident they will fix it soon :)
I like the f-string feature and I think everyone will start supporting it.

@gnaponie can you disable the flake8 rule globally that causes the issue?

Can you make this log.exception so that the LDAP exception is retained in the logs?

Can you make this log.exception so that the LDAP exception is retained in the logs?

I like the f-string feature and I think everyone will start supporting it.

Yeah, it's really nice feature, but I think there are couple of downsides, like you cannot use them in multi-line strings.

@gnaponie can you disable the flake8 rule globally that causes the issue?

It is "E999 SyntaxError: invalid syntax ", I don't think we should disable that globally.

The problem with this query is that it does not include the posixGroup objects the user is a member of. It does seem to include the Rover groups though.
If you only need to care about the posixGroup membership, then you can use the following query:
results = con.search_s('ou=Groups,dc=example,dc=com', ldap.SCOPE_SUBTREE, f'(memberUid={user})', ['cn'])
group_cns = [group[1]['cn'] for group in results]

If you need the Rover groups and the traditional posixGroups, then you may have to perform both queries.
@mikeb, do you have any suggestions?

@gnaponie what did you decide on this comment?

@gnaponie what did you decide on this comment?

I've decided for making the query on posixGroup. I've discussed a bit with @jkaluza about it, and it seems that that should be the best solution for us.

:thumbsup:

mmm but in this case everyone who needs to run the test needs to install ldap. Do we care? Not sure. It shouldn't in the requirement though I think.

Mocking 'waiverdb.api_v1.WaiversResource.get_group_membership.ldap' or 'ldap' fails if you don't have it installed?

@mprahl @lholecek comments addressed.

Optional: set this message to a variable so that you don't have to duplicate the string. While, you're at it ldap => LDAP :smile:

Optional: If you move this before the allowed_groups += permission['groups'] line, then you can save an unnecessary list concatenation

Optional: It'd be more intuitive if this was a set to begin with instead of a list

with => for the

@gnaponie I left some optional comments, but feel free to ignore them if you disagree.

Regardless, :thumbsup:

@mprahl I changed the ones I shared :)
Can we merge it?

Added another commit to test "proxied_by" and proceed with this KR. I know we are never gonna close it....

+1 (I didn't know about proxied_by parameter.)

Commit 03964df8 fixes this pull-request