odcs

Clone

#37 Allow adding users in configured groups
Merged 6 years ago by qwan. Opened 6 years ago by qwan.
qwan/odcs users-in-config-group  into  master

file modified
+14 -12 
@@ -39,18 +39,20 @@ 
 

      PDC_INSECURE = False
 

      PDC_DEVELOP = True
 

  

-     # Used to authorize authenticated users.
 

-     # Each of them is a string representing a group name. So far, ODCS
 

-     # supports OpenIDC and Kerberos authentication depending on the
 

-     # concrete deployment environment. So, for
 

-     # OpenIDC authentication, they are FAS group names.
 

-     # Kerberos authentication, they are LDAP group names.
 

-     # If not allow anyone to perform actions, keep empty list here.
 

-     ALLOWED_GROUPS = []
 

- 

-     # Users in ADMIN_GROUPS will be treated as admin.
 

-     # The groups info is retrieved from backend authentication system.
 

-     ADMIN_GROUPS = []
 

+     # Users are required to be in allowed_clients to generate composes,
 

+     # you can add group names or usernames (it can be normal user or host
 

+     # principal) into ALLOWED_CLIENTS. The group names are from ldap for
 

+     # kerberos users or FAS for openidc users.
 

+     ALLOWED_CLIENTS = {
 

+         'groups': [],
 

+         'users': [],
 

+     }
 

+ 

+     # Users in ADMINS are granted with admin permission.
 

+     ADMINS = {
 

+         'groups': [],
 

+         'users': [],
 

+     }
 

  

      # Select which authentication backend to work with. There are 3 choices
 

      # noauth: no authentication is enabled. Useful for development particularly.

file modified
+19 -19 
@@ -156,24 +156,24 @@ 
 

          raise ValueError('Unknown backend name {0}.'.format(backend))
 

  

  

- def admin_required(f):
 

-     """Check if current user is in admin groups"""
 

-     @wraps(f)
 

-     def wrapper(*args, **kwargs):
 

-         if conf.authorize_disabled:
 

-             return f(*args, **kwargs)
 

-         if bool(set(flask.g.groups) & set(conf.admin_groups)):
 

-             return f(*args, **kwargs)
 

-         abort(401, 'User {0} is not in admin groups {1}.'.format(
 

-             flask.g.user.username, str(conf.admin_groups)))
 

-     return wrapper
 

- 

+ def requires_role(role):
 

+     """Check if user is in the configured role.
 

  

- def user_in_allowed_groups():
 

-     """Check if current user is in allowed groups
 

- 

-     :return: True if current user is in allowed groups, otherwise False is
 

-         returned.
 

-     :rtype: bool
 

+     :param str role: role name, supported roles: 'allowed_clients', 'admins'.
 

      """
 

-     return bool(set(flask.g.groups) & set(conf.allowed_groups))
 

+     valid_roles = ['allowed_clients', 'admins']
 

+     if role not in valid_roles:
 

+         raise ValueError("Unknown role <%s> specified, supported roles: %s." % (role, str(valid_roles)))
 

+ 

+     def wrapper(f):
 

+         @wraps(f)
 

+         def wrapped(*args, **kwargs):
 

+             groups = getattr(conf, role).get('groups', [])
 

+             users = getattr(conf, role).get('users', [])
 

+             in_groups = bool(set(flask.g.groups) & set(groups))
 

+             in_users = flask.g.user.username in users
 

+             if in_groups or in_users:
 

+                 return f(*args, **kwargs)
 

+             abort(401, 'User %s is not in role %s.' % (flask.g.user.username, role))
 

+         return wrapped
 

+     return wrapper

file modified
+8 -8 
@@ -162,14 +162,14 @@ 
 

              'type': str,
 

              'default': '',
 

              'desc': "Group base to query user's groups from LDAP server."},
 

-         'allowed_groups': {
 

-             'type': list,
 

-             'default': [],
 

-             'desc': "List of group names to authorize user's request."},
 

-         'admin_groups': {
 

-             'type': list,
 

-             'default': [],
 

-             'desc': "List of group names that users in these groups will be treated as admin"},
 

+         'allowed_clients': {
 

+             'type': dict,
 

+             'default': {'groups': [], 'users': []},
 

+             'desc': "Groups and users that are allowed to generate composes."},
 

+         'admins': {
 

+             'type': dict,
 

+             'default': {'groups': [], 'users': []},
 

+             'desc': "Admin groups and users."},
 

          'auth_backend': {
 

              'type': str,
 

              'default': 'noauth',

file modified
+4 -19 
@@ -24,10 +24,8 @@ 
 

  import datetime
 

  import json
 

  

- import flask
 

- 

  from flask.views import MethodView
 

- from flask import request, jsonify, abort
 

+ from flask import request, jsonify
 

  from flask_login import login_required
 

  

  from odcs import app, db, log, conf
 
@@ -35,20 +33,7 @@ 
 

  from odcs.models import Compose, COMPOSE_RESULTS, COMPOSE_FLAGS, COMPOSE_STATES
 

  from odcs.pungi import PungiSourceType
 

  from odcs.api_utils import pagination_metadata, filter_composes
 

- from odcs.auth import user_in_allowed_groups as _user_in_allowed_groups
 

- from odcs.auth import admin_required
 

- 

- 

- def user_in_allowed_groups(func):
 

-     """Only allow user who is in allowed groups to call endpoint"""
 

-     def _decorator(*args, **kwargs):
 

-         if conf.authorize_disabled:
 

-             return func(*args, **kwargs)
 

-         if _user_in_allowed_groups():
 

-             return func(*args, **kwargs)
 

-         abort(401, 'User {0} is not in allowed groups.'.format(
 

-             flask.g.user.username))
 

-     return _decorator
 

+ from odcs.auth import requires_role
 

  

  

  api_v1 = {
 
@@ -100,7 +85,7 @@ 
 

                  raise NotFound('No such compose found.')
 

  

      @login_required
 

-     @user_in_allowed_groups
 

+     @requires_role('allowed_clients')
 

      def post(self):
 

          owner = "Unknown"  # TODO
 

  
@@ -188,7 +173,7 @@ 
 

          return jsonify(compose.json()), 200
 

  

      @login_required
 

-     @admin_required
 

+     @requires_role('admins')
 

      def delete(self, id):
 

          compose = Compose.query.filter_by(id=id).first()
 

          if compose:

file modified
-31 
@@ -246,34 +246,3 @@ 
 

          self.assertRaises(ValueError, init_auth, self.login_manager, 'xxx')
 

          self.assertRaises(ValueError, init_auth, self.login_manager, '')
 

          self.assertRaises(ValueError, init_auth, self.login_manager, None)
 

- 

- 

- class TestUserInAllowedGroups(unittest.TestCase):
 

-     """Test user_in_allowed_groups"""
 

- 

-     @patch.object(odcs.auth.conf, 'allowed_groups', new=[])
 

-     def test_not_allow_when_allowed_groups_is_empty(self):
 

-         with app.test_request_context():
 

-             flask.g.groups = []
 

-             self.assertFalse(odcs.auth.user_in_allowed_groups())
 

- 

-             flask.g.groups = ['testers']
 

-             self.assertFalse(odcs.auth.user_in_allowed_groups())
 

- 

-     @patch.object(odcs.auth.conf, 'allowed_groups', new=['admins', 'testers'])
 

-     def test_not_allow_when_not_in_allowed_groups(self):
 

-         with app.test_request_context():
 

-             flask.g.groups = ['common']
 

-             self.assertFalse(odcs.auth.user_in_allowed_groups())
 

- 

-     @patch.object(odcs.auth.conf, 'allowed_groups', new=['admins', 'testers'])
 

-     def test_allow_when_in_allowed_groups(self):
 

-         with app.test_request_context():
 

-             flask.g.groups = ['common', 'testers']
 

-             self.assertTrue(odcs.auth.user_in_allowed_groups())
 

- 

-             flask.g.groups = ['common', 'testers', 'admins']
 

-             self.assertTrue(odcs.auth.user_in_allowed_groups())
 

- 

-             flask.g.groups = ['testers', 'admins']
 

-             self.assertTrue(odcs.auth.user_in_allowed_groups())

file modified
+54 -65 
@@ -28,15 +28,13 @@ 
 

  import flask
 

  

  from freezegun import freeze_time
 

- from mock import Mock, patch
 

- from werkzeug.exceptions import Unauthorized
 

+ from mock import patch
 

  

  import odcs.auth
 

  

  from odcs import db, app, login_manager
 

  from odcs.models import Compose, COMPOSE_STATES, COMPOSE_RESULTS, User
 

  from odcs.pungi import PungiSourceType
 

- from odcs.views import user_in_allowed_groups
 

  

  

  @login_manager.user_loader
 
@@ -48,14 +46,18 @@ 
 

      maxDiff = None
 

  

      def setUp(self):
 

-         self.patch_allowed_groups = patch.object(odcs.auth.conf,
 

-                                                  'allowed_groups',
 

-                                                  new=['tester'])
 

-         self.patch_admin_groups = patch.object(odcs.auth.conf,
 

-                                                'admin_groups',
 

-                                                new=['admin'])
 

-         self.patch_allowed_groups.start()
 

-         self.patch_admin_groups.start()
 

+         patched_allowed_clients = {'groups': ['composer'],
 

+                                    'users': ['dev']}
 

+         patched_admins = {'groups': ['admin'],
 

+                           'users': ['root']}
 

+         self.patch_allowed_clients = patch.object(odcs.auth.conf,
 

+                                                   'allowed_clients',
 

+                                                   new=patched_allowed_clients)
 

+         self.patch_admins = patch.object(odcs.auth.conf,
 

+                                          'admins',
 

+                                          new=patched_admins)
 

+         self.patch_allowed_clients.start()
 

+         self.patch_admins.start()
 

  

          self.client = app.test_client()
 

          db.session.remove()
 
@@ -81,8 +83,8 @@ 
 

          db.drop_all()
 

          db.session.commit()
 

  

-         self.patch_allowed_groups.stop()
 

-         self.patch_admin_groups.stop()
 

+         self.patch_allowed_clients.stop()
 

+         self.patch_admins.stop()
 

  

      @contextlib.contextmanager
 

      def test_request_context(self, user=None, groups=None, **kwargs):
 
@@ -98,13 +100,15 @@ 
 

                          flask.g.groups = groups
 

                      else:
 

                          flask.g.groups = [groups]
 

+                 else:
 

+                     flask.g.groups = []
 

                  with self.client.session_transaction() as sess:
 

                      sess['user_id'] = user
 

                      sess['_fresh'] = True
 

              yield
 

  

      def test_submit_build(self):
 

-         with self.test_request_context(user='tester', groups=['tester']):
 

+         with self.test_request_context(user='dev'):
 

              rv = self.client.post('/odcs/1/composes/', data=json.dumps(
 

                  {'source_type': 'module', 'source': 'testmodule-master'}))
 

              data = json.loads(rv.data.decode('utf8'))
 
@@ -124,7 +128,7 @@ 
 

          self.assertEqual(c.state, COMPOSE_STATES["wait"])
 

  

      def test_submit_build_nodeps(self):
 

-         with self.test_request_context(user='tester', groups=['tester']):
 

+         with self.test_request_context(user='dev'):
 

              rv = self.client.post('/odcs/1/composes/', data=json.dumps(
 

                  {'source_type': 'tag', 'source': 'f26', 'packages': ['ed'],
 

                   'flags': ['no_deps']}))
 
@@ -141,7 +145,7 @@ 
 

          self.c1.reused_id = 1
 

          db.session.commit()
 

  

-         with self.test_request_context(user='tester', groups=['tester']):
 

+         with self.test_request_context(user='dev'):
 

              rv = self.client.post('/odcs/1/composes/', data=json.dumps({'id': 1}))
 

              data = json.loads(rv.data.decode('utf8'))
 

  
@@ -158,7 +162,7 @@ 
 

          self.c1.reused_id = 1
 

          db.session.commit()
 

  

-         with self.test_request_context(user='tester', groups=['tester']):
 

+         with self.test_request_context(user='dev'):
 

              rv = self.client.post('/odcs/1/composes/', data=json.dumps({'id': 1}))
 

              data = json.loads(rv.data.decode('utf8'))
 

  
@@ -171,14 +175,14 @@ 
 

          self.assertEqual(c.reused_id, None)
 

  

      def test_submit_build_resurrection_no_removed(self):
 

-         with self.test_request_context(user='tester', groups=['tester']):
 

+         with self.test_request_context(user='dev'):
 

              rv = self.client.post('/odcs/1/composes/', data=json.dumps({'id': 1}))
 

              data = json.loads(rv.data.decode('utf8'))
 

  

          self.assertEqual(data['message'], 'No expired or failed compose with id 1')
 

  

      def test_submit_build_resurrection_not_found(self):
 

-         with self.test_request_context(user='tester', groups=['tester']):
 

+         with self.test_request_context(user='dev'):
 

              rv = self.client.post('/odcs/1/composes/', data=json.dumps({'id': 100}))
 

              data = json.loads(rv.data.decode('utf8'))
 

  
@@ -236,7 +240,7 @@ 
 

  

              self.assertEqual(len(Compose.composes_to_expire()), 0)
 

  

-             with self.test_request_context(user='tester', groups=['tester', 'admin']):
 

+             with self.test_request_context(user='root'):
 

                  resp = self.client.delete("/odcs/1/composes/%s" % c3.id)
 

                  data = json.loads(resp.data.decode('utf8'))
 

  
@@ -264,7 +268,7 @@ 
 

                  db.session.commit()
 

                  compose_id = new_c.id
 

  

-                 with self.test_request_context(user='tester', groups=['tester', 'admin']):
 

+                 with self.test_request_context(user='root'):
 

                      resp = self.client.delete("/odcs/1/composes/%s" % compose_id)
 

                      data = json.loads(resp.data.decode('utf8'))
 

  
@@ -275,7 +279,7 @@ 
 

                  self.assertEqual(data['error'], 'Bad Request')
 

  

      def test_delete_non_exist_compose(self):
 

-         with self.test_request_context(user='tester', groups=['tester', 'admin']):
 

+         with self.test_request_context(user='root'):
 

              resp = self.client.delete("/odcs/1/composes/999999")
 

              data = json.loads(resp.data.decode('utf8'))
 

  
@@ -285,57 +289,42 @@ 
 

          self.assertEqual(data['error'], 'Not Found')
 

  

      def test_delete_compose_with_non_admin_user(self):
 

-         with self.test_request_context(user='tester', groups=['tester']):
 

+         with self.test_request_context(user='dev'):
 

              resp = self.client.delete("/odcs/1/composes/%s" % self.c1.id)
 

  

          self.assertEqual(resp.status, '401 UNAUTHORIZED')
 

          self.assertEqual(resp.status_code, 401)
 

  

+     def test_can_not_create_compose_with_non_composer_user(self):
 

+         with self.test_request_context(user='qa'):
 

+             resp = self.client.post('/odcs/1/composes/', data=json.dumps(
 

+                 {'source_type': 'module', 'source': 'testmodule-master'}))
 

  

- class TestUserInAllowedGroupsDecorator(unittest.TestCase):
 

-     """Test decorator user_in_allowed_groups"""
 

- 

-     def setUp(self):
 

-         self.mock_func = Mock()
 

-         self.decorated_func = user_in_allowed_groups(self.mock_func)
 

- 

-         self.patch_allowed_groups = patch.object(odcs.auth.conf,
 

-                                                  'allowed_groups',
 

-                                                  new=['testers'])
 

-         self.patch_allowed_groups.start()
 

- 

-     def tearDown(self):
 

-         self.patch_allowed_groups.stop()
 

- 

-     def test_401_if_not_in_allowed_groups(self):
 

-         with app.test_request_context():
 

-             flask.g.groups = ['another_group']
 

-             flask.g.user = User(id=1, username='tester')
 

- 

-             self.assertRaises(Unauthorized, self.decorated_func, 1, 2, 3)
 

-             self.mock_func.assert_not_called()
 

- 

-     def test_authorized_if_in_allowed_groups(self):
 

-         with app.test_request_context():
 

-             flask.g.groups = ['testers']
 

-             flask.g.user = User(id=1, username='tester')
 

+         self.assertEqual(resp.status, '401 UNAUTHORIZED')
 

+         self.assertEqual(resp.status_code, 401)
 

  

-             self.decorated_func(1, 2, 3)
 

-             self.mock_func.assert_called_once_with(1, 2, 3)
 

+     def test_can_create_compose_with_user_in_configured_groups(self):
 

+         with self.test_request_context(user='another_user', groups=['composer']):
 

+             resp = self.client.post('/odcs/1/composes/', data=json.dumps(
 

+                 {'source_type': 'module', 'source': 'testmodule-rawhide'}))
 

+         db.session.expire_all()
 

  

-     @patch.object(odcs.views.conf, 'authorize_disabled', new=True)
 

-     def test_no_authorize_when_disable_authorize(self):
 

-         with app.test_request_context():
 

-             flask.g.groups = ['testers']
 

-             flask.g.user = User(id=1, username='tester')
 

+         self.assertEqual(resp.status, '200 OK')
 

+         self.assertEqual(resp.status_code, 200)
 

+         c = db.session.query(Compose).filter(Compose.source == 'testmodule-rawhide').one()
 

+         self.assertEqual(c.state, COMPOSE_STATES["wait"])
 

  

-             self.decorated_func(1, 2, 3)
 

-             self.mock_func.assert_called_once_with(1, 2, 3)
 

+     def test_can_delete_compose_with_user_in_configured_groups(self):
 

+         c3 = Compose.create(
 

+             db.session, "unknown", PungiSourceType.MODULE, "testmodule-testbranch",
 

+             COMPOSE_RESULTS["repository"], 60)
 

+         c3.state = COMPOSE_STATES['done']
 

+         db.session.add(c3)
 

+         db.session.commit()
 

  

-         with app.test_request_context():
 

-             flask.g.groups = ['another_groups']
 

-             flask.g.user = User(id=1, username='tester')
 

+         with self.test_request_context(user='another_admin', groups=['admin']):
 

+             resp = self.client.delete("/odcs/1/composes/%s" % c3.id)
 

+             data = json.loads(resp.data.decode('utf8'))
 

  

-             self.decorated_func(1, 2, 3)
 

-             self.assertEqual(2, self.mock_func.call_count)
 

-             self.mock_func.assert_called_with(1, 2, 3)
 

+         self.assertEqual(resp.status, '202 ACCEPTED')
 

+         self.assertEqual(data['status'], 202)

Rename ALLOWED_GROUPS to 'COMPOSER_GROUP', 'ADMIN_GROUPS' to
'ADMIN_GROUP' in config. And allow specifying group names or users
in them.

Typo here. Unknown group.

Probably also need a space after the <%s>.

Also - this valid_groups check and exception can probably be moved out of wrapped and wrapper. You can do the check at decoration time instead of at call time.

Just to give it some context, we need it to let freshmaker to contact ODCS. Kerberos service principal (like the Freshmaker's one) cannot be added to group and therefore we have to mention services like this directly in the configuration file.

i.e.: 

def requires_group(group): 
    valid_groups = ['admin', 'composer']
    if group not in valid_groups: 
        raise whatever
    def wrapper(f): 
        ...

Just to give it some context, we need it to let freshmaker to contact ODCS. Kerberos service principal (like the Freshmaker's one) cannot be added to group and therefore we have to mention services like this directly in the configuration file.

Makes sense. In general, :+1:.

Looks like COMPOSER_GROUP is becoming a role used to authorize. If so, would COMPOSERS and ADMINS be enough, or or something else by removing _GROUP, to make it clear? Otherwise, a group containing groups and users could be confused easily.

And it would also be helpful to give more explanation or examples in comment to describe how to set groups and users here.
Edited 6 years ago by cqi

rebased
6 years ago

What about ALLOWED_CLIENTS and ADMINS (or ALLOWED_ADMINS, which is kind of stupid, because you don't want to have blacklist of admins).

The problem with "COMPOSERS" is that I don't see from the name what this option is going to do. It can be whitelist/blacklist of clients or some internal composing machines doing the compose (I presume I don't have big knowledge of how it works internally). While ALLOWED_CLIENTS make it clear.

rebased
6 years ago

rebased
6 years ago

+1

rebased
6 years ago

Pull-Request has been merged by qwan
6 years ago
Changes Summary 6
+14 -12
file changed
conf/config.py
+19 -19
file changed
odcs/auth.py
+8 -8
file changed
odcs/config.py
+4 -19
file changed
odcs/views.py
+0 -31
file changed
tests/test_auth.py
+54 -65
file changed
tests/test_views.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.