PR#312: Restrict waiver creation based on users/groups and testcase - waiverdb

waiverdb

#312 Restrict waiver creation based on users/groups and testcase

Merged 4 years ago by gnaponie. Opened 5 years ago by gnaponie.

gnaponie/waiverdb permission-mapping into master

Test proxied_by with access control

Giulia Naponiello • 4 years ago

4cb1f4d

Restrict waiver creation based on users/groups and testcase

Giulia Naponiello • 4 years ago

14b1001

tests/conftest.py

file modified

+20

		`@@ -81,3 +81,23 @@`
		`@pytest.fixture()`
		`def enable_cors(app, monkeypatch):`
		`monkeypatch.setitem(app.config, 'CORS_URL', 'https://bodhi.fedoraproject.org')`
		`+`
		`+`
		`+ @pytest.fixture()`
		`+ def enable_permission_mapping(app, monkeypatch):`
		`+ monkeypatch.setitem(app.config, 'PERMISSION_MAPPING',`
		`+ {`
		`+ "^testcase1.*": {"groups": ["factory-2-0"], "users": []}, # noqa`
		`+ "^testcase2.*": {"groups": [], "users": ["foo"]}, # noqa`
		`+ "^testcase4.*": {"groups": [], "users": []} # noqa`
		`+ })`
		`+`
		`+`
		`+ @pytest.fixture()`
		`+ def enable_ldap_host(app, monkeypatch):`
		`+ monkeypatch.setitem(app.config, 'LDAP_HOST', 'ldap://ldap.something.com')`
		`+`
		`+`
		`+ @pytest.fixture()`
		`+ def enable_ldap_base(app, monkeypatch):`
		`+ monkeypatch.setitem(app.config, 'LDAP_BASE', 'ou=Users,dc=something,dc=com')`

tests/test_access_control.py

file added

+156

		`@@ -0,0 +1,156 @@`
		`+ # SPDX-License-Identifier: GPL-2.0+`
		`+`
		`+ import json`
		`+ from base64 import b64encode`
		`+`
		`+ import mock`
		`+ import ldap`
		`+ import pytest`
		`+`
		`+`
		`+ @pytest.mark.usefixtures('enable_permission_mapping')`
		`+ @pytest.mark.usefixtures('enable_kerberos')`
		`+ @mock.patch.multiple("gssapi.SecurityContext", complete=True,`
		`+ __init__=mock.Mock(return_value=None),`
		`+ step=mock.Mock(return_value=b"STOKEN"),`
		`+ initiator_name="foo@EXAMPLE.ORG")`
		`+ @mock.patch.multiple("gssapi.Credentials",`
		`+ __init__=mock.Mock(return_value=None),`
		`+ __new__=mock.Mock(return_value=None))`
		`+ class TestAccessControl(object):`
		`+`
		`+ data = {`
		`+ 'subject_type': 'koji_build',`
		`+ 'subject_identifier': 'glibc-2.26-27.fc27',`
		`+ 'testcase': 'testcase1',`
		`+ 'product_version': 'fool-1',`
		`+ 'waived': True,`
		`+ 'comment': 'it broke',`
		`+ }`
		`+ headers = {'Authorization':`
		`+ 'Negotiate %s' % b64encode(b"CTOKEN").decode()}`
		`+`
		`+ def test_ldap_host_base_not_defined(self, client, session):`
		`+ r = client.post('/api/v1.0/waivers/', data=json.dumps(self.data),`
		`+ content_type='application/json', headers=self.headers)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 500`
		`+ assert res_data['message'] == ("LDAP_HOST and LDAP_BASE also need to be "`
		`+ "defined if PERMISSION_MAPPING is defined.")`
		`+`
		`+ @pytest.mark.usefixtures('enable_ldap_host')`
		`+ def test_ldap_host_defined_base_not(self, client, session):`
		`+ self.test_ldap_host_base_not_defined(client, session)`
		`+`
		`+ @pytest.mark.usefixtures('enable_ldap_base')`
		`+ def test_ldap_base_defined_host_not(self, client, session):`
		`+ self.test_ldap_host_base_not_defined(client, session)`
		`+`
		`+ @pytest.mark.usefixtures('enable_ldap_host')`
		`+ @pytest.mark.usefixtures('enable_ldap_base')`
		`+ @mock.patch('ldap.initialize', side_effect=ldap.LDAPError())`
		`+ def test_initialization_ldap_connection(self, mocked, client, session):`
		`+ r = client.post('/api/v1.0/waivers/', data=json.dumps(self.data),`
		`+ content_type='application/json', headers=self.headers)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 401`
		`+ assert res_data['message'] == "Some error occured initializing the LDAP connection."`
		`+`
		`+ @pytest.mark.usefixtures('enable_ldap_host')`
		`+ @pytest.mark.usefixtures('enable_ldap_base')`
		`+ @mock.patch('waiverdb.api_v1.WaiversResource.get_group_membership', return_value=([]))`
		`+ def test_user_not_found_in_ldap(self, mocked_conn, client, session, monkeypatch):`
		`+ monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab')`
		`+ r = client.post('/api/v1.0/waivers/', data=json.dumps(self.data),`
		`+ content_type='application/json', headers=self.headers)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 401`
		`+ assert res_data['message'] == "Couldn't find user foo in LDAP"`
		`+`
		`+ @pytest.mark.usefixtures('enable_ldap_host')`
		`+ @pytest.mark.usefixtures('enable_ldap_base')`
		`+ @mock.patch('waiverdb.api_v1.WaiversResource.get_group_membership',`
		`+ return_value=(['factory-2-0', 'something-else']))`
		`+ def test_group_has_permission(self, mocked_conn, client, session, monkeypatch):`
		`+ monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab')`
		`+ r = client.post('/api/v1.0/waivers/', data=json.dumps(self.data),`
		`+ content_type='application/json', headers=self.headers)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 201`
		`+ assert res_data['username'] == 'foo'`
		`+ assert res_data['subject'] == {'type': 'koji_build', 'item': 'glibc-2.26-27.fc27'}`
		`+ assert res_data['subject_type'] == 'koji_build'`
		`+ assert res_data['subject_identifier'] == 'glibc-2.26-27.fc27'`
		`+ assert res_data['testcase'] == 'testcase1'`
		`+ assert res_data['product_version'] == 'fool-1'`
		`+ assert res_data['waived'] is True`
		`+ assert res_data['comment'] == 'it broke'`
		`+`
		`+ @pytest.mark.usefixtures('enable_ldap_host')`
		`+ @pytest.mark.usefixtures('enable_ldap_base')`
		`+ def test_user_has_permission(self, client, session, monkeypatch):`
		`+ monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab')`
		`+ self.data['testcase'] = 'testcase2'`
		`+ r = client.post('/api/v1.0/waivers/', data=json.dumps(self.data),`
		`+ content_type='application/json', headers=self.headers)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 201`
		`+ assert res_data['username'] == 'foo'`
		`+ assert res_data['subject'] == {'type': 'koji_build', 'item': 'glibc-2.26-27.fc27'}`
		`+ assert res_data['subject_type'] == 'koji_build'`
		`+ assert res_data['subject_identifier'] == 'glibc-2.26-27.fc27'`
		`+ assert res_data['testcase'] == 'testcase2'`
		`+ assert res_data['product_version'] == 'fool-1'`
		`+ assert res_data['waived'] is True`
		`+ assert res_data['comment'] == 'it broke'`
		`+`
		`+ @pytest.mark.usefixtures('enable_ldap_host')`
		`+ @pytest.mark.usefixtures('enable_ldap_base')`
		`+ @mock.patch('waiverdb.api_v1.WaiversResource.get_group_membership',`
		`+ return_value=(['factory-2-0', 'something-else']))`
		`+ def test_both_user_group_no_permission(self, mocked_conn, client, session, monkeypatch):`
		`+ monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab')`
		`+ self.data['testcase'] = 'testcase3'`
		`+ r = client.post('/api/v1.0/waivers/', data=json.dumps(self.data),`
		`+ content_type='application/json', headers=self.headers)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 401`
		`+ assert res_data['message'] == ("You are not authorized to submit a waiver "`
		`+ "for the test case testcase3")`
		`+`
		`+ @pytest.mark.usefixtures('enable_ldap_host')`
		`+ @pytest.mark.usefixtures('enable_ldap_base')`
		`+ @mock.patch('waiverdb.auth.get_user', return_value=('bodhi', {}))`
		`+ @mock.patch('waiverdb.api_v1.WaiversResource.get_group_membership',`
		`+ return_value=(['factory-2-0', 'something-else']))`
		`+ def test_proxied_by_with_no_permission(self, mocked_conn, mock_get_user, client, session):`
		`+ self.data['testcase'] = 'testcase3'`
		`+ self.data['username'] = 'foo'`
		`+ r = client.post('/api/v1.0/waivers/', data=json.dumps(self.data),`
		`+ content_type='application/json', headers=self.headers)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 401`
		`+ assert res_data['message'] == ("You are not authorized to submit a waiver "`
		`+ "for the test case testcase3")`
		`+`
		`+ @pytest.mark.usefixtures('enable_ldap_host')`
		`+ @pytest.mark.usefixtures('enable_ldap_base')`
		`+ @mock.patch('waiverdb.auth.get_user', return_value=('bodhi', {}))`
		`+ @mock.patch('waiverdb.api_v1.WaiversResource.get_group_membership',`
		`+ return_value=(['factory-2-0', 'something-else']))`
		`+ def test_proxied_by_has_permission(self, mocked_conn, mock_get_user, client, session):`
		`+ self.data['testcase'] = 'testcase2'`
		`+ self.data['username'] = 'foo'`
		`+ r = client.post('/api/v1.0/waivers/', data=json.dumps(self.data),`
		`+ content_type='application/json', headers=self.headers)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 201`
		`+ assert res_data['username'] == 'foo'`
		`+ assert res_data['subject'] == {'type': 'koji_build', 'item': 'glibc-2.26-27.fc27'}`
		`+ assert res_data['subject_type'] == 'koji_build'`
		`+ assert res_data['subject_identifier'] == 'glibc-2.26-27.fc27'`
		`+ assert res_data['testcase'] == 'testcase2'`
		`+ assert res_data['product_version'] == 'fool-1'`
		`+ assert res_data['waived'] is True`
		`+ assert res_data['comment'] == 'it broke'`
		`+ assert res_data['proxied_by'] == 'bodhi'`

waiverdb/api_v1.py

file modified

+47 -1

		`@@ -1,11 +1,14 @@`
		`# SPDX-License-Identifier: GPL-2.0+`

		`import datetime`
		`+ import re`
		`+ import logging`

		`import requests`
		`from flask import Blueprint, request, current_app`
		`from flask_restful import Resource, Api, reqparse, marshal_with, marshal`
		`- from werkzeug.exceptions import BadRequest, Forbidden, ServiceUnavailable`
		`+ from werkzeug.exceptions import (BadRequest, Forbidden, ServiceUnavailable,`
		`+ InternalServerError, Unauthorized, BadGateway)`
		`from sqlalchemy.sql.expression import func, and_, or_`

		`from waiverdb import __version__`
		`@@ -18,6 +21,7 @@`
		`api_v1 = (Blueprint('api_v1', __name__))`
		`api = Api(api_v1)`
		`requests_session = requests.Session()`
		`+ log = logging.getLogger(__name__)`


		`def valid_dict(value):`
		`@@ -321,6 +325,46 @@`

		`return result, 201, headers`

		`+ def get_group_membership(self, user):`
		`+ try:`
		`+ import ldap`
		`+ except ImportError:`
		`+ raise InternalServerError(('If PERMISSION_MAPPING is defined, '`
		`+ 'python-ldap needs to be installed.'))`
		`+ try:`
		`+ con = ldap.initialize(current_app.config['LDAP_HOST'])`
		`+ results = con.search_s(current_app.config['LDAP_BASE'], ldap.SCOPE_SUBTREE,`
		`+ f'(memberUid={user})', ['cn'])`
		`+ return [group[1]['cn'][0].decode('utf-8') for group in results]`
		`+ except ldap.LDAPError:`
		`+ log.exception('Some error occured initializing the LDAP connection.')`
		`+ raise Unauthorized('Some error occured initializing the LDAP connection.')`
		`+ except ldap.SERVER_DOWN:`
		`+ log.exception('The LDAP server is not reachable.')`
		`+ raise BadGateway('The LDAP server is not reachable.')`
		`+`
		`+ def verify_authorization(self, user, testcase):`
		`+ if not current_app.config['PERMISSION_MAPPING']:`
		`+ return True`
		`+ if not (current_app.config.get('LDAP_HOST') and current_app.config.get('LDAP_BASE')):`
		`+ raise InternalServerError(('LDAP_HOST and LDAP_BASE also need to be defined '`
		`+ 'if PERMISSION_MAPPING is defined.'))`
		`+ allowed_groups = []`
		`+ for testcase_pattern, permission in current_app.config['PERMISSION_MAPPING'].items():`
		`+ testcase_match = re.search(testcase_pattern, testcase)`
		`+ if testcase_match:`
		`+ # checking if the user is allowed`
		`+ if user in permission['users']:`
		`+ return True`
		`+ allowed_groups += permission['groups']`
		`+ group_membership = self.get_group_membership(user)`
		`+ if not group_membership:`
		`+ raise Unauthorized(f'Couldn\'t find user {user} in LDAP')`
		`+ if set(group_membership) & set(allowed_groups):`
		`+ return True`
		`+ raise Unauthorized(('You are not authorized to submit a waiver '`
		`+ f'for the test case {testcase}'))`
		`+`
		`def _create_waiver(self, args, user):`
		`proxied_by = None`
		`if args.get('username'):`
		`@@ -375,6 +419,8 @@`
		`if not args['testcase']:`
		`raise BadRequest({'testcase': 'Missing required parameter in the JSON body'})`

		`+ self.verify_authorization(user, args['testcase'])`
		`+`
		`# brew-build is an alias for koji_build`
		`if args['subject_type'] == 'brew-build':`
		`args['subject_type'] = 'koji_build'`

waiverdb/cli.py

file modified

+6 -5

		`@@ -278,11 +278,12 @@`
		`resp = requests.request(`
		`'POST', url, auth=auth, **common_request_arguments)`
		`if resp.status_code == 401:`
		`- raise click.ClickException('WaiverDB authentication using GSSAPI failed. '`
		`- 'Make sure you have a valid Kerberos ticket or '`
		`- 'that you correctly configured your Kerberos '`
		`- 'configuration file. Please check the doc for '`
		`- 'troubleshooting information.')`
		`+ msg = resp.json().get(`
		`+ 'message', ('WaiverDB authentication using GSSAPI failed. Make sure you have a '`
		`+ 'valid Kerberos ticket or that you correctly configured your Kerberos '`
		`+ 'configuration file. Please check the doc for troubleshooting '`
		`+ 'information.'))`
		`+ raise click.ClickException(msg)`
		`check_response(resp, result_ids)`
		`elif auth_method == 'dummy':`
		`resp = requests.request(`

waiverdb/config.py

file modified

		`@@ -27,6 +27,7 @@`
		`SQLALCHEMY_TRACK_MODIFICATIONS = True`
		`# A list of users are allowed to create waivers on behalf of other users.`
		`SUPERUSERS = []`
		`+ PERMISSION_MAPPING = {}`


		`class ProductionConfig(Config):`

gnaponie commented 5 years ago

Introduce access control based on the users/groups and the testcase.
Groups need to be defined in LDAP.
New configuration is required to enable this feature:
- PERMISSION_MAPPING: dictionary with keys regex applied to testcases
and as values dictionaries with "users" and "groups" allowed to
submit waivers for that matching testcase.
If not specified, the feature is not enabled.
- LDAP_HOST and LDAP_BASE: required to query the LDAP system.
If PERMISSION_MAPPING is specified, but these are not, it throws an
error.

gnaponie commented 5 years ago

PTAL @lucarval @mprahl

mprahl commented on line 30 of waiverdb/api_v1.py 5 years ago

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

mprahl commented on line 28 of waiverdb/api_v1.py 5 years ago

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?

mprahl commented on line 34 of waiverdb/api_v1.py 5 years ago

I'd prefer if you did:

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

......

mprahl commented on line 35 of waiverdb/api_v1.py 5 years ago

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

mprahl commented on line 35 of waiverdb/api_v1.py 5 years ago

It's a bit simpler if you do:

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

mprahl commented on line 43 of waiverdb/api_v1.py 5 years ago

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

mprahl commented on line 45 of waiverdb/api_v1.py 5 years ago

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

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

mprahl commented on line 51 of waiverdb/api_v1.py 5 years ago

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.

mprahl commented on line 54 of waiverdb/api_v1.py 5 years ago

How about?

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

mprahl commented on line 58 of waiverdb/api_v1.py 5 years ago

The f is not necessary in this string.

mprahl commented on line 33 of waiverdb/api_v1.py 5 years ago

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.

mprahl commented on line 20 of waiverdb/api_v1.py 5 years ago

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.

mprahl commented on line 56 of tests/test_access_control.py 5 years ago

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

gnaponie commented on line 56 of tests/test_access_control.py 5 years ago

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.

gnaponie commented on line 28 of waiverdb/api_v1.py 5 years ago

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

gnaponie commented on line 43 of waiverdb/api_v1.py 5 years ago

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.

gnaponie commented on line 54 of waiverdb/api_v1.py 5 years ago

oh... my beautiful regexp....

rebased onto 0db12004da5dce4c660668d23b258e3c1167b16f

4 years ago

gnaponie commented 4 years ago

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

mprahl commented on line 51 of tests/test_access_control.py 4 years ago

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.

mprahl commented on line 32 of waiverdb/api_v1.py 4 years ago

How about logging the exception?

mprahl commented on line 34 of waiverdb/api_v1.py 4 years ago

How about logging the exception?

mprahl commented on line 34 of waiverdb/api_v1.py 4 years ago

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

mprahl commented on line 42 of waiverdb/api_v1.py 4 years ago

Optional: This is a good candidate for a set

mprahl commented on line 54 of waiverdb/api_v1.py 4 years ago

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

mprahl commented on line 50 of waiverdb/api_v1.py 4 years ago

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

gnaponie commented on line 54 of waiverdb/api_v1.py 4 years ago

Do you mean the return value of get_group_membership?

mprahl commented on line 54 of waiverdb/api_v1.py 4 years ago

Yes

rebased onto 9e0cfceb1d4af6bc506dfa63d7d38a306acd28ef

4 years ago

gnaponie commented 4 years ago

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

mprahl commented on line 52 of tests/test_access_control.py 4 years ago

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

mprahl commented 4 years ago

:thumbsup:

lholecek commented on line 17 of waiverdb/cli.py 4 years ago

msg = resp.json().get('message', UNAUTHORIZED_ERROR_MESSAGE)

lholecek commented on line 34 of waiverdb/api_v1.py 4 years ago

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

lholecek commented on line 38 of waiverdb/api_v1.py 4 years ago

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

gnaponie commented on line 34 of waiverdb/api_v1.py 4 years ago

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

gnaponie commented on line 38 of waiverdb/api_v1.py 4 years ago

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

lholecek commented 4 years ago

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.

lholecek commented 4 years ago

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

gnaponie commented 4 years ago

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.

mprahl commented 4 years ago

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

mprahl commented on line 41 of waiverdb/api_v1.py 4 years ago

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

mprahl commented on line 44 of waiverdb/api_v1.py 4 years ago

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

lholecek commented 4 years ago

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 commented 4 years ago

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

mprahl commented 4 years ago

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?

Edited 4 years ago by mprahl

gnaponie commented 4 years ago

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

mprahl commented 4 years ago

:thumbsup:

gnaponie commented on line 52 of tests/test_access_control.py 4 years ago

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.

mprahl commented on line 52 of tests/test_access_control.py 4 years ago

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

rebased onto 01caf9c640c253a81ff9993f8a4001b8841542cb

4 years ago

gnaponie commented 4 years ago

@mprahl @lholecek comments addressed.