#342 Refactoring: Move authorization to separate submodule
Merged a year ago by lholecek. Opened a year ago by lholecek.
lholecek/waiverdb split-authorization  into  master

file modified
+6 -6
@@ -58,11 +58,11 @@ 

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

+         assert res_data['message'] == "Some error occurred 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=([]))

+     @mock.patch('waiverdb.authorization.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),
@@ -73,7 +73,7 @@ 

  

      @pytest.mark.usefixtures('enable_ldap_host')

      @pytest.mark.usefixtures('enable_ldap_base')

-     @mock.patch('waiverdb.api_v1.WaiversResource.get_group_membership',

+     @mock.patch('waiverdb.authorization.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')
@@ -110,7 +110,7 @@ 

  

      @pytest.mark.usefixtures('enable_ldap_host')

      @pytest.mark.usefixtures('enable_ldap_base')

-     @mock.patch('waiverdb.api_v1.WaiversResource.get_group_membership',

+     @mock.patch('waiverdb.authorization.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')
@@ -125,7 +125,7 @@ 

      @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',

+     @mock.patch('waiverdb.authorization.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'
@@ -140,7 +140,7 @@ 

      @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',

+     @mock.patch('waiverdb.authorization.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'

file modified
+14 -42
@@ -1,17 +1,20 @@ 

  # 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,

-                                  InternalServerError, Unauthorized, BadGateway)

+ from werkzeug.exceptions import (

+     BadRequest,

+     Forbidden,

+     ServiceUnavailable,

+ )

  from sqlalchemy.sql.expression import func, and_, or_

  

  from waiverdb import __version__

+ from waiverdb.authorization import verify_authorization

  from waiverdb.models import db

  from waiverdb.models.waivers import Waiver, subject_dict_to_type_identifier

  from waiverdb.utils import json_collection, jsonp
@@ -325,45 +328,14 @@ 

  

          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']:

+     def _verify_authorization(self, user, testcase):

+         permission_mapping = current_app.config.get('PERMISSION_MAPPING')

+         if not 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}'))

+ 

+         ldap_host = current_app.config.get('LDAP_HOST')

+         ldap_base = current_app.config.get('LDAP_BASE')

+         return verify_authorization(user, testcase, permission_mapping, ldap_host, ldap_base)

  

      def _create_waiver(self, args, user):

          proxied_by = None
@@ -419,7 +391,7 @@ 

          if not args['testcase']:

              raise BadRequest({'testcase': 'Missing required parameter in the JSON body'})

  

-         self.verify_authorization(user, args['testcase'])

+         self._verify_authorization(user, args['testcase'])

  

          # brew-build is an alias for koji_build

          if args['subject_type'] == 'brew-build':

@@ -0,0 +1,56 @@ 

+ # SPDX-License-Identifier: GPL-2.0+

+ 

+ import logging

+ import re

+ 

+ from werkzeug.exceptions import (

+     BadGateway,

+     InternalServerError,

+     Unauthorized,

+ )

+ 

+ log = logging.getLogger(__name__)

+ 

+ 

+ def get_group_membership(user, ldap_host, ldap_base):

+     try:

+         import ldap

+     except ImportError:

+         raise InternalServerError(('If PERMISSION_MAPPING is defined, '

+                                    'python-ldap needs to be installed.'))

+ 

+     try:

+         con = ldap.initialize(ldap_host)

+         results = con.search_s(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 occurred initializing the LDAP connection.')

+         raise Unauthorized('Some error occurred 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(user, testcase, permission_mapping, ldap_host, ldap_base):

+     if not (ldap_host and 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 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 = get_group_membership(user, ldap_host, ldap_base)

+     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}'))

I need this to be able to check the permissions with a script.

Optional: 2 commits might be better: one for moving to the separate submodule and one for the small fixes (typos and stylish new lines and stuff like that).

Beside that: +1 this looks good.

rebased onto 0df3b68

a year ago

Optional: 2 commits might be better: one for moving to the separate submodule and one for the small fixes (typos and stylish new lines and stuff like that).

You're right. I was too lazy to split it after already amending my commit. :)

Fixed now.

Looks good. Feel free to merge it when the CI finishes.

Commit cf5e7ef fixes this pull-request

Pull-Request has been merged by lholecek

a year ago

Pull-Request has been merged by lholecek

a year ago