#2081 new policy for dist-repo
Merged 4 years ago by tkopecek. Opened 4 years ago by tkopecek.
tkopecek/koji issue1660  into  master

file modified
+2 -1
@@ -11816,7 +11816,8 @@ 

  

      def distRepo(self, tag, keys, **task_opts):

          """Create a dist-repo task. returns task id"""

-         context.session.assertPerm('dist-repo')

+         if not context.session.hasPerm('dist-repo') and not context.session.hasPerm('admin'):

+             assert_policy('dist_repo', {'tag': tag})

          repo_id, event_id = dist_repo_init(tag, keys, task_opts)

          task_opts['event'] = event_id

          # cancel potentially running distRepos

@@ -85,14 +85,16 @@ 

  

  class TestDistRepo(unittest.TestCase):

  

+     @mock.patch('kojihub.assert_policy')

      @mock.patch('kojihub.dist_repo_init')

      @mock.patch('kojihub.make_task')

-     def test_DistRepo(self, make_task, dist_repo_init):

+     def test_DistRepo(self, make_task, dist_repo_init, assert_policy):

          session = kojihub.context.session = mock.MagicMock()

          session.user_id = 123

          # It seems MagicMock will not automatically handle attributes that

          # start with "assert"

-         session.assertPerm = mock.MagicMock()

+         session.hasPerm = mock.MagicMock()

+         session.hasPerm.return_value = False

          dist_repo_init.return_value = ('repo_id', 'event_id')

          make_task.return_value = 'task_id'

          exports = kojihub.RootExports()
@@ -101,7 +103,8 @@ 

  

          ret = exports.distRepo('tag', 'keys')

  

-         session.assertPerm.assert_called_once_with('dist-repo')

+         session.hasPerm.has_calls(mock.call('dist_repo'), mock.call('admin'))

+         assert_policy.assert_called_once_with('dist_repo', {'tag': 'tag'})

          dist_repo_init.assert_called_once()

          make_task.assert_called_once()

          self.assertEquals(ret, make_task.return_value)

rebased onto 57336632009784857882e8a7bf45b68c60d987de

4 years ago

these 2 asserts failed,'dist-repo' <-> 'dist_repo'

1 new commit added

  • fix typo
4 years ago

Ouch, I've renamed it in the end to be consistent with existing policy names. Fixed in test.

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

4 years ago

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

4 years ago

I think we should assert login before checking policy.

We're providing the keys value to the policy, but there is no policy handler than can interpret it.

  • there are no specific handlers for this value
  • none of the generic handlers understand lists

Otoh, we're omitting the opts parameter, which could be relevant in principle.

I suppose all we really need to solve #1660 is user data, which is implicit. Perhaps the easiest thing would be to only explicitly provide the tag value.

rebased onto ef60c2e

4 years ago

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-done

4 years ago

Commit 164e4bf fixes this pull-request

Pull-Request has been merged by tkopecek

4 years ago