#1701 fix user operations typos
Merged 2 years ago by tkopecek. Opened 2 years ago by julian8628.
julian8628/koji issue/1629-fix2  into  master

file modified
+17 -2
@@ -2064,7 +2064,14 @@ 

      usage += _("\n(Specify the --help global option for a list of other help options)")

      parser = OptionParser(usage=usage)

      parser.add_option("--rename", help=_("Rename the user"))

-     parser.add_option("--krb", help=_("Change kerberos principal of the user"))

+     parser.add_option("--edit-krb", action="append", default=[],

+                       metavar="OLD=NEW",

+                       help=_("Change kerberos principal of the user"))

+     parser.add_option("--add-krb", action="append", default=[], metavar="KRB",

+                       help=_("Add kerberos principal of the user"))

+     parser.add_option("--remove-krb", action="append", default=[],

+                       metavar="KRB",

+                       help=_("Remove kerberos principal of the user"))

      (options, args) = parser.parse_args(args)

      if len(args) < 1:

          parser.error(_("You must specify the username of the user to edit"))
@@ -2072,7 +2079,15 @@ 

          parser.error(_("This command only accepts one argument (username)"))

      activate_session(session, goptions)

      user = args[0]

-     session.editUser(user, options.rename, options.krb)

+     princ_mappings = []

+     for p in options.edit_krb:

+         old, new = p.split('=', 1)

+         princ_mappings.append({'old': arg_filter(old), 'new': arg_filter(old)})

+     for a in options.add_krb:

+         princ_mappings.append({'old': None, 'new': arg_filter(a)})

+     for r in options.remove_krb:

+         princ_mappings.append({'old': arg_filter(r), 'new': None})

+     session.editUser(user, options.rename, princ_mappings)

  

  

  def handle_list_signed(goptions, session, args):

file modified
+99 -33
@@ -3666,7 +3666,11 @@ 

  def get_user(userInfo=None, strict=False, krb_princs=False):

      """Return information about a user.

  

-     :param userInfo: either a str (Kerberos principal or name) or an int (user id)

+     :param userInfo: a str (Kerberos principal or name) or an int (user id)

+                      or a dict:

+                          - id: User's ID

+                          - name: User's name

+                          - krb_principal: Kerberos principal

      :param strict: whether raising Error when no user found

      :param krb_princs: whether show krb_principals in result

      :return: a dict as user's information:
@@ -3682,14 +3686,41 @@ 

              # not logged in

              raise koji.GenericError("No user provided")

      fields = ['id', 'name', 'status', 'usertype']

-     data = {'info': userInfo}

-     if isinstance(userInfo, six.integer_types):

-         clauses = ['id = %(info)i']

-     elif isinstance(userInfo, str):

+     if isinstance(userInfo, dict):

+         data = userInfo

+     elif isinstance(userInfo, six.integer_types):

+         data = {'id': userInfo}

+     elif isinstance(userInfo, six.string_types):

+         data = {'info': userInfo}

          clauses = ['krb_principal = %(info)s OR name = %(info)s']

      else:

          raise koji.GenericError('invalid type for userInfo: %s'

                                  % type(userInfo))

+     if isinstance(data, dict) and not data.get('info'):

+         clauses = []

+         uid = data.get('id')

+         if uid is not None:

+             if isinstance(uid, six.integer_types):

+                 clauses.append('users.id = %(id)i')

+             else:

+                 raise koji.GenericError('invalid type for userid: %s'

+                                         % type(uid))

+         username = data.get('name')

+         if username:

+             if isinstance(username, six.string_types):

+                 clauses.append('users.name = %(name)s')

+             else:

+                 raise koji.GenericError('invalid type for username: %s'

+                                         % type(username))

+         krb_principal = data.get('krb_principal')

+         if krb_principal:

+             if isinstance(krb_principal, six.string_types):

+                 clauses.append('user_krb_principals.krb_principal'

+                                ' = %(krb_principal)s')

+             else:

+                 raise koji.GenericError('invalid type for krb_principal: %s'

+                                         % type(krb_principal))

+ 

      query = QueryProcessor(tables=['users'], columns=fields,

                             joins=['LEFT JOIN user_krb_principals'

                                    ' ON users.id = user_krb_principals.user_id'],
@@ -3701,22 +3732,28 @@ 

          user['krb_principals'] = list_user_krb_principals(user['id'])

      return user

  

- def edit_user(userInfo, name=None, krb_principal=None):

+ def edit_user(userInfo, name=None, krb_principal_mappings=None):

      """Edit information for an existing user.

  

      userInfo specifies the user to edit

      fields changes are provided as keyword arguments:

          name: rename the user

-         krb_principal: change user's kerberos principal

+         krb_principal_mappings: change user's kerberos principal, it is a list

+                                 contains krb_principal pair(s)

+                                 - old: krb_principal to modify, None and ''

+                                        indicates adding a new krb_principal

+                                 - new: new value of krb_principal, None and ''

+                                        indicates removing the old krb_principal

      """

  

      context.session.assertPerm('admin')

-     _edit_user(userInfo, name=name, krb_principal=krb_principal)

+     _edit_user(userInfo, name=name,

+                krb_principal_mappings=krb_principal_mappings)

  

  

- def _edit_user(userInfo, name=None, krb_principal=None):

+ def _edit_user(userInfo, name=None, krb_principal_mappings=None):

      """Edit information for an existing user."""

-     user = get_user(userInfo, strict=True)

+     user = get_user(userInfo, strict=True, krb_princs=True)

      if name and user['name'] != name:

          # attempt to update user name

          values = {
@@ -3728,14 +3765,42 @@ 

          if id is not None:

              # new name is taken

              raise koji.GenericError("Name %s already taken by user %s" % (name, id))

-         update = UpdateProcessor('users', values={'userID': user['id']}, clauses=['id = %(userID)i'])

+         update = UpdateProcessor('users',

+                                  values={'userID': user['id']},

+                                  clauses=['id = %(userID)i'])

          update.set(name=name)

          update.execute()

-     if krb_principal and user['krb_principal'] != krb_principal:

+     if krb_principal_mappings:

+         added = set()

+         removed = set()

+         for pairs in krb_principal_mappings:

+             old = pairs.get('old')

+             new = pairs.get('new')

+             if old:

+                 removed.add(old)

+             if new:

+                 added.add(new)

+         dups = added & removed

+         if dups:

+             raise koji.GenericError("There are some conflicts between added"

+                                     " and removed Kerberos principals: %s"

+                                     % ', '.join(dups))

+         currents = set(user.get('krb_principals'))

+         dups = added & currents

+         if dups:

+             raise koji.GenericError("Cannot add existing Kerberos"

+                                     " principals: %s" % ', '.join(dups))

+         unable_removed = removed - currents

+         if unable_removed:

+             raise koji.GenericError("Cannot remove non-existent Kerberos"

+                                     " principals: %s"

+                                     % ', '.join(unable_removed))

+ 

          # attempt to update kerberos principal

-         update = UpdateProcessor('users', values={'userID': user['id']}, clauses=['id = %(userID)i'])

-         update.set(krb_principal=krb_principal)

-         update.execute()

+         for r in removed:

+             context.session.removeKrbPrincipal(user['id'], krb_principal=r)

+         for a in added:

+             context.session.setKrbPrincipal(user['id'], krb_principal=a)

  

  

  def list_user_krb_principals(user_info=None):
@@ -3785,16 +3850,8 @@ 

      if not isinstance(krb_principal, str):

          raise koji.GenericError("invalid type for krb_principal: %s"

                                  % type(krb_principal))

-     fields = ['user_id']

-     data = {'krb_principal': krb_principal}

-     clauses = ['krb_principal = %(krb_principal)s']

-     query = QueryProcessor(tables=['users_krb_principals'], columns=fields,

-                            clauses=clauses, values=data)

-     princ_item = query.executeOne()

-     if not princ_item and strict:

-         raise koji.GenericError("Cannot find user with kerberos principal: %s"

-                                 % krb_principal)

-     return get_user(data, strict=strict, krb_princ=krb_princs)

+     return get_user({'krb_principal': krb_principal}, strict=strict,

+                     krb_princs=krb_princs)

  

  

  def find_build_id(X, strict=False):
@@ -8036,14 +8093,23 @@ 

      if not ginfo or ginfo['usertype'] != koji.USERTYPES['GROUP']:

          raise koji.GenericError("Not a group: %s" % group)

      group_id = ginfo['id']

-     fields = ('id', 'name', 'usertype', 'array_remove(array_agg(krb_principal)'

-                                         ', NULL) AS krb_principals')

-     q = """SELECT %s FROM user_groups

-     JOIN users ON user_groups.user_id = users.id

-     LEFT JOIN user_krb_principals ON users.id = user_krb_principals.user_id

-     WHERE active = TRUE AND group_id = %%(group_id)i

-     GROUP BY users.id""" % ','.join(fields)

-     return _multiRow(q, locals(), fields)

+     columns = ('id', 'name', 'usertype', 'array_remove(array_agg(krb_principal)'

+                                          ', NULL)')

+     aliases = ('id', 'name', 'usertype', 'krb_principals')

+     joins = ['JOIN users ON user_groups.user_id = users.id',

+              'LEFT JOIN user_krb_principals'

+              ' ON users.id = user_krb_principals.user_id']

+     clauses = [eventCondition(None), 'group_id = %(group_id)i']

+ 

+     query = QueryProcessor(tables=['user_groups'],

+                            columns=columns,

+                            aliases=aliases,

+                            joins=joins,

+                            clauses=clauses,

+                            values=locals(),

+                            opts={'group': 'users.id'},

+                            enable_group=True)

+     return query.iterate()

  

  def set_user_status(user, status):

      context.session.assertPerm('admin')

file modified
+16 -5
@@ -689,7 +689,12 @@ 

      def setKrbPrincipal(self, name, krb_principal, krb_princ_check=True):

          if krb_princ_check:

              self.checkKrbPrincipal(krb_principal)

-         select = """SELECT id FROM users WHERE name = %(name)s"""

+         select = """SELECT id FROM users WHERE %s"""

+         if isinstance(name, six.integer_types):

+             user_condition = 'id = %(name)i'

+         else:

+             user_condition = 'name = %(name)s'

+         select = select % user_condition

          cursor = context.cnx.cursor()

          cursor.execute(select, locals())

          r = cursor.fetchone()
@@ -707,20 +712,26 @@ 

      def removeKrbPrincipal(self, name, krb_principal):

          select = """SELECT id FROM users

                      JOIN user_krb_principals

-                     WHERE name = %(name)s

-                     AND krb_principal = %(krb_principal)s"""

+                     ON users.id = user_krb_principals.user_id

+                     WHERE %s

+                     AND krb_principal = %%(krb_principal)s"""

+         if isinstance(name, six.integer_types):

+             user_condition = 'id = %(name)i'

+         else:

+             user_condition = 'name = %(name)s'

+         select = select % user_condition

          cursor = context.cnx.cursor()

          cursor.execute(select, locals())

          r = cursor.fetchone()

          if not r:

              context.cnx.rollback()

              raise koji.AuthError(

-                 'could not automatically remove Kerberos Principal:'

+                 'cannot remove Kerberos Principal:'

                  ' %(krb_principal)s with user %(name)s' % locals())

          else:

              user_id = r[0]

          delete = """DELETE FROM user_krb_principals

-                     WHERE user_id = (%(user_id)i

+                     WHERE user_id = %(user_id)i

                      AND krb_principal = %(krb_principal)s"""

          cursor.execute(delete, locals())

          context.cnx.commit()

@@ -3,7 +3,10 @@ 

  import os

  import six

  import sys

- import unittest

+ try:

+     import unittest2 as unittest

+ except ImportError:

+     import unittest

  

  

  from koji_cli.commands import handle_edit_user
@@ -20,10 +23,11 @@ 

      def test_handle_edit_user(self, activate_session_mock, stdout):

          user = 'user'

          rename = 'user2'

-         krb_principal = 'krb'

          args = [user]

          args.append('--rename=' + rename)

-         args.append('--krb=' + krb_principal)

+         args.append('--add-krb=addedkrb')

+         args.append('--remove-krb=removedkrb')

+         args.append('--edit-krb=oldkrb=newkrb')

          options = mock.MagicMock()

  

          # Mock out the xmlrpc server
@@ -38,7 +42,10 @@ 

          self.assertMultiLineEqual(actual, expected)

          # Finally, assert that things were called as we expected.

          activate_session_mock.assert_called_once_with(session, options)

-         session.editUser.assert_called_once_with(user, rename, krb_principal)

+         session.editUser.assert_called_once_with(user, rename,

+                                                  [{'new': 'oldkrb', 'old': 'oldkrb'},

+                                                   {'new': 'addedkrb', 'old': None},

+                                                   {'new': None, 'old': 'removedkrb'}])

          self.assertEqual(rv, None)

  

          stdout.seek(0)
@@ -67,9 +74,11 @@ 

  (Specify the --help global option for a list of other help options)

  

  Options:

-   -h, --help       show this help message and exit

-   --rename=RENAME  Rename the user

-   --krb=KRB        Change kerberos principal of the user

+   -h, --help          show this help message and exit

+   --rename=RENAME     Rename the user

+   --edit-krb=OLD=NEW  Change kerberos principal of the user

+   --add-krb=KRB       Add kerberos principal of the user

+   --remove-krb=KRB    Remove kerberos principal of the user

  """ % progname

          expected_stderr = ''

          self.assertMultiLineEqual(actual_stdout, expected_stdout)

@@ -1,5 +1,8 @@ 

  import mock

- import unittest

+ try:

+     import unittest2 as unittest

+ except ImportError:

+     import unittest

  

  import koji

  import kojihub
@@ -30,11 +33,11 @@ 

  

      def test_edit(self):

          self.get_user.return_value = {'id': 333,

-                                      'name': 'user',

-                                      'krb_principal': 'krb'}

+                                       'name': 'user',

+                                       'krb_principals': ['krb']}

          self._singleValue.return_value = None

  

-         kojihub._edit_user('user', name='newuser', krb_principal='krb')

+         kojihub._edit_user('user', name='newuser')

          # check the update

          self.assertEqual(len(self.updates), 1)

          update = self.updates[0]
@@ -43,18 +46,53 @@ 

          self.assertEqual(update.values, {'userID': 333})

          self.assertEqual(update.clauses, ['id = %(userID)i'])

  

-         kojihub._edit_user('user', name='user', krb_principal='newkrb')

-         # check the insert/update

-         self.assertEqual(len(self.updates), 2)

-         update = self.updates[1]

-         self.assertEqual(update.table, 'users')

-         self.assertEqual(update.data, {'krb_principal': 'newkrb'})

-         self.assertEqual(update.values, {'userID': 333})

-         self.assertEqual(update.clauses, ['id = %(userID)i'])

+         kojihub._edit_user('user', krb_principal_mappings=[{'old': 'krb',

+                                                    'new': 'newkrb'}])

+         self.context.session.removeKrbPrincipal. \

+             assert_called_once_with(333, krb_principal='krb')

+         self.context.session.setKrbPrincipal. \

+             assert_called_once_with(333, krb_principal='newkrb')

+ 

+         self.context.reset_mock()

+         with self.assertRaises(koji.GenericError) as cm:

+             kojihub._edit_user('user',

+                                krb_principal_mappings=[{'old': 'krb',

+                                                         'new': 'newkrb'},

+                                                        {'old': 'newkrb',

+                                                         'new': 'newnewkrb'}

+                                                        ])

+         self.assertEqual(cm.exception.args[0],

+                          'There are some conflicts between added and removed'

+                          ' Kerberos principals: newkrb')

+         self.context.session.removeKrbPrincipal.assert_not_called()

+         self.context.session.setKrbPrincipal.assert_not_called()

+ 

+         self.context.reset_mock()

+         with self.assertRaises(koji.GenericError) as cm:

+             kojihub._edit_user('user',

+                                krb_principal_mappings=[{'old': 'otherkrb',

+                                                         'new': 'newkrb'}])

+         self.assertEqual(cm.exception.args[0],

+                          'Cannot remove non-existent Kerberos principals:'

+                          ' otherkrb')

+         self.context.session.removeKrbPrincipal.assert_not_called()

+         self.context.session.setKrbPrincipal.assert_not_called()

+ 

+         self.context.reset_mock()

+         with self.assertRaises(koji.GenericError) as cm:

+             kojihub._edit_user('user',

+                                krb_principal_mappings=[{'old': None,

+                                                         'new': 'krb'}])

+         self.assertEqual(cm.exception.args[0],

+                          'Cannot add existing Kerberos principals: krb')

+         self.context.session.removeKrbPrincipal.assert_not_called()

+         self.context.session.setKrbPrincipal.assert_not_called()

+ 

  

          self._singleValue.reset_mock()

          self._singleValue.return_value = 2

          with self.assertRaises(koji.GenericError) as cm:

              kojihub._edit_user('user', name='newuser')

          self._singleValue.assert_called_once()

-         self.assertEqual(cm.exception.args[0], 'Name newuser already taken by user 2')

+         self.assertEqual(cm.exception.args[0],

+                          'Name newuser already taken by user 2')

@@ -287,7 +287,7 @@ 

          self.context.session.assertPerm.side_effect = None

          self.get_user.side_effect = get_user2

          kojihub.get_group_members(group)

-         self.assertEqual(len(self.queries), 0)

+         self.assertEqual(len(self.queries), 1)

          self.assertEqual(len(self.inserts), 0)

          self.assertEqual(len(self.updates), 0)

-         _multiRow.assert_called_once()

+         _multiRow.assert_not_called()

rebased onto 8c59889

2 years ago

also fixes #862
edit-user command and editUser API will support add/remove/change kerberos principal for one user(it's executed as delete/insert in DB, instead of update)

Metadata Update from @julian8628:
- Pull-request tagged with: testing-ready

2 years ago

1 new commit added

  • fix unitest import for el6
2 years ago

Commit 97cee1c fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago