#1730 allow tag or target permissions as appropriate
Closed 4 years ago by tkopecek. Opened 4 years ago by cobrien.
cobrien/koji issue_1729  into  koji-1.18-updates

file modified
+16 -16
@@ -63,8 +63,8 @@ 

      group = args[1]

  

      activate_session(session, goptions)

-     if not session.hasPerm('admin'):

-         print("This action requires admin privileges")

+     if not (session.hasPerm('admin') or session.hasPerm('tag')):

+         print("This action requires tag or admin privileges")

          return 1

  

      dsttag = session.getTag(tag)
@@ -94,8 +94,8 @@ 

      group = args[1]

  

      activate_session(session, goptions)

-     if not session.hasPerm('admin'):

-         print("This action requires admin privileges")

+     if not (session.hasPerm('admin') or session.hasPerm('tag')):

+         print("This action requires tag or admin privileges")

          return 1

  

      dsttag = session.getTag(tag)
@@ -3319,8 +3319,8 @@ 

          assert False  # pragma: no cover

      activate_session(session, goptions)

  

-     if not session.hasPerm('admin') and not options.test:

-         parser.error(_("This action requires admin privileges"))

+     if not options.test and not (session.hasPerm('admin') or session.hasPerm('tag')):

+         parser.error(_("This action requires tag or admin privileges"))

  

      if args[0] == args[1]:

          parser.error(_('Source and destination tags must be different.'))
@@ -3772,8 +3772,8 @@ 

          #most targets have the same name as their destination

          dest_tag = name

      activate_session(session, goptions)

-     if not session.hasPerm('admin'):

-         print("This action requires admin privileges")

+     if not (session.hasPerm('admin') or session.hasPerm('target')):

+         print("This action requires target or admin privileges")

          return 1

  

      chkbuildtag = session.getTag(build_tag)
@@ -3807,8 +3807,8 @@ 

          assert False  # pragma: no cover

      activate_session(session, goptions)

  

-     if not session.hasPerm('admin'):

-         print("This action requires admin privileges")

+     if not (session.hasPerm('admin') or session.hasPerm('target')):

+         print("This action requires target or admin privileges")

          return

  

      targetInfo = session.getBuildTarget(args[0])
@@ -3850,8 +3850,8 @@ 

          assert False  # pragma: no cover

      activate_session(session, goptions)

  

-     if not session.hasPerm('admin'):

-         print("This action requires admin privileges")

+     if not (session.hasPerm('admin') or session.hasPerm('target')):

+         print("This action requires target or admin privileges")

          return

  

      target = args[0]
@@ -3875,8 +3875,8 @@ 

          assert False  # pragma: no cover

      activate_session(session, goptions)

  

-     if not session.hasPerm('admin'):

-         print("This action requires admin privileges")

+     if not (session.hasPerm('admin') or session.hasPerm('tag')):

+         print("This action requires tag or admin privileges")

          return

  

      tag = args[0]
@@ -4876,8 +4876,8 @@ 

          parser.error(_("Please specify a name for the tag"))

          assert False  # pragma: no cover

      activate_session(session, goptions)

-     if not session.hasPerm('admin'):

-         print("This action requires admin privileges")

+     if not (session.hasPerm('admin') or session.hasPerm('tag')):

+         print("This action requires tag or admin privileges")

          return

      opts = {}

      if options.parent:

@@ -129,12 +129,13 @@ 

          # Run it and check immediate output

          rv = handle_add_group(options, session, arguments)

          actual = stdout.getvalue()

-         expected = 'This action requires admin privileges\n'

+         expected = 'This action requires tag or admin privileges\n'

          self.assertMultiLineEqual(actual, expected)

  

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

          activate_session_mock.assert_called_once_with(session, options)

-         session.hasPerm.assert_called_once_with('admin')

+         session.hasPerm.assert_has_calls([mock.call('admin'),

+                                           mock.call('tag')])

          session.getTag.assert_not_called()

          session.getTagGroups.assert_not_called()

          session.groupListAdd.assert_not_called()

@@ -44,7 +44,7 @@ 

              activate_session=None)

  

          # Case 2. not admin account

-         expected = "This action requires admin privileges\n"

+         expected = "This action requires tag or admin privileges\n"

          session.hasPerm.return_value = None

          handle_add_tag(options, session, ['test-tag'])

          self.assert_console_message(stdout, expected)

@@ -123,6 +123,6 @@ 

          session.hasPerm.return_value = False

          rv = handle_block_group(options, session, ['tag', 'grp'])

          self.assert_console_message(

-             stdout, 'This action requires admin privileges\n')

+             stdout, 'This action requires tag or admin privileges\n')

          self.assertEqual(rv, 1)

          activate_session_mock.assert_called_with(session, options)

@@ -58,10 +58,10 @@ 

              self.session,

              args,

              stderr=self.format_error_message(

-                 "This action requires admin privileges"),

+                 "This action requires tag or admin privileges"),

              activate_session=None)

          self.activate_session.assert_called_once()

-         self.session.hasPerm.assert_called_once_with('admin')

+         self.session.hasPerm.assert_has_calls([call('admin'), call('tag')])

  

      def test_handle_clone_tag_same_tag(self):

          args = ['src-tag', 'src-tag']

I went the simple route here and set the check to be either tag/target or admin and updated the error message to match.

It may be more correct to just completely remove the permission check and allow the hub to take care of failing. But since the check exists, I figured this was the minimal path.

Fixes #1729

If you would like me to instead just rip out the client-side permission checks (in order for the hub to be the one to make the entire decision) just let me know and I'll update.

I note that this does break some unit tests that are expecting a particular error string when a permission isn't met. I can look at those as well if you'd like and you're happy with the double-check change. Just let me know.

Yes, this is the behaviour we wanted. I've wrongly assumed, that hasPerm works more as assertPerm, so that's the reason. If tests are fixed, I'm +1 for merge.

1 new commit added

  • test for appropriate hasPerm calls and error messages
4 years ago

Pull-Request has been closed by tkopecek

4 years ago