koji

Clone

#3851 sidetag: extend is_sidetag_owner for untag ops
Merged 8 months ago by tkopecek. Opened 10 months ago by tkopecek.
tkopecek/koji issue3848  into  master

file modified
+6 -2 
@@ -122,8 +122,12 @@ 
 

          match action block && is_sidetag_owner :: allow
 

          all :: deny
 

  

- There are two special policy tests `is_sidetag` and `is_sidetag_owner` with
 

- expectable behaviour.
 

+ There are two special policy tests ``is_sidetag`` and ``is_sidetag_owner`` with
 

+ expectable behaviour. ``is_sidetag_owner`` can handle optional
 

+ ``tag``/``fromtag``/``both`` keywords which specify data to be tested. Default
 

+ is testing ``tag`` in policy data, ``fromtag`` can test  this field (e.g. in
 

+ ``untagBuild`` case) and ``both`` fails if any of the involved tags is not owned
 

+ by sidetag owner.
 

  

  Now Sidetag Koji plugin should be installed.  To verify that, run
 

  `koji list-api` command -- it should now display `createSideTag`

file modified
+20 -2 
@@ -65,9 +65,27 @@ 
 

      name = 'is_sidetag_owner'
 

  

      def run(self, data):
 

+         fields = self.str.split()[1:]
 

+         if len(fields) > 1:
 

+             raise koji.GenericError("Just one argument is allowed for this test.")
 

+         elif fields:
 

+             key = fields[0]
 

+             if key not in ('tag', 'fromtag', 'both'):
 

+                 raise koji.GenericError("Policy test is_sidetag_owner has only "
 

+                                         f"/tag/fromtag/both options (got {key})")
 

+             if key == 'both':
 

+                 fields = ['tag', 'fromtag']
 

+         else:
 

+             fields = ['tag']
 

+ 

          user = policy_get_user(data)
 

-         tag = get_tag(data['tag'])
 

-         return is_sidetag_owner(tag, user)
 

+         for field in fields:
 

+             if field not in data:
 

+                 return False
 

+             tag = get_tag(data[field])
 

+             if not tag or not is_sidetag_owner(tag, user):
 

+                 return False
 

+         return True
 

  

  

  # API calls

rebased onto a7bbeb4e21db56da6ee12089a03aa0114f89a3ad
10 months ago

I'm not sure about the untag semantics. It would fail in this case. Does it make sense to test also fromtag if it exists? Isn't it a) confusing b) usable? For b it would mean that policy from the issue would work only if both tags are sidetags owned by the caller.

I'm not sure about the untag semantics. It would fail in this case. Does it make sense to test also fromtag if it exists?

With the other tag based policy checks (actually, it appears there are only tag and fromtag, plus buildtag but that's a different situation really), there is a separate test to consider the fromtag value. I suppose the test could take an argument. Note that move operations have both tag and fromtag values.

If someone is writing a policy and they care about builds coming from side tags, then you need a way to make the test specific for the move case

For my use-case, I want to give owners of sidetags freedom - meaning if they want to untag anything from their side-tag, they can. That means the logic should be if any of tag or fromtag match user, it should pass.

1 new commit added

  • sidetag: option for is_sidetag_owner test
10 months ago

Added argument source/target/both - is the naming reasonable?

Added argument source/target/both - is the naming reasonable?

I'd rather not use the term "target" here, as it could create confusion with build targets

You have "target" looking at fromtag, which is backwards.

The stanza converting arg to action seems overly verbose given that it's basically just setting action=arg and checking that action is in the allowed set.

I'm not sure if "action" is the best name for this var either, since it's not really an action, more of a value/location to check.

rebased onto 95cbb7ae43d231b4203232c561e8dea9eb4e0c82
10 months ago 
values = self.str.split()[1:]

I think the name value here is overly generic and may have contributed to the typo below

for value in values:
    if value not in data:
        return False
    tag = get_tag(value)

It's the corresponding value in the data dict we want to pass to get_tag.

Maybe key or field instead of value here? E.g. tag = get_tag(data[field]). Other policy tests use field in similar way.

rebased onto ba7ec1f
8 months ago

:thumbsup:

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

Metadata Update from @mfilip:
- Pull-request tagged with: testing-done
8 months ago

Commit d324f60 fixes this pull-request

Pull-Request has been merged by tkopecek
8 months ago
Metadata
None

testing-ready testing-done

Changes Summary 2
+6 -2
file changed
docs/source/plugins.rst
+20 -2
file changed
plugins/hub/sidetag_hub.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.