This should not be referred to as a "whitelist".

rpm_macros_allowed = parent['extra']['sidetag_rpm_macros_allowed']

This will error if the value is unset in the parent tag. We should use .get() with a default of False.

Furthermore, we should validate this value a bit first. If the value is invalid, we should log an error and treat it as False.

if isinstance(rpm_macros_allowed, list) and macro not in rpm_macros_allowed:

Perhaps it would make sense to allow a list of glob patterns. Could use koji.util.multi_fnmatch.

Should we also apply the allow list for remove_rpm_macros?

The only case where it would matter is when an admin has set a value for the side tag that is outside of the allowed list. I suppose it is relatively harmless to allow the sidetag owner to remove those, but on the other hand it would probably be more consistent to check for both operations.

Not inherent to this change, since we've been reading tag.extra this way here for a while, but I'm wondering about whether we want to allow for inheritance here (e.g. use getBuildConfig instead of get_tag(t)['extra'])

This will error if the value is unset in the parent tag. We should use .get() with a default of False.

fixed

Furthermore, we should validate this value a bit first. If the value is invalid, we should log an error and treat it as False.

Done accordingly with https://github.com/rpm-software-management/rpm/blob/master/rpmio/macro.c#L627

Perhaps it would make sense to allow a list of glob patterns. Could use koji.util.multi_fnmatch.

done

Not inherent to this change, since we've been reading tag.extra this way here for a while, but I'm wondering about whether we want to allow for inheritance here (e.g. use getBuildConfig instead of get_tag(t)['extra'])

I think that current behaviour is more in line with user expectations?

I think that current behaviour is more in line with user expectations?

Sure, we can stick with it

Oh, not quite what I meant by validating the rpm_macros_allowed value.

The code assumes that this value is either

• a list of strings
• a value that evaluates as False

So, I really meant a type validation, so if someone sets this value to, say, a bare string or a dict, or even a list containing non-strings, that we don't throw a confusing error to the user.

That said, the _valid_rpm_macro_name function is nice to have here, however we would need to apply this check to the actual macro values that we're setting (i.e. the ones the user passes to the call). If we're allowing glob patterns in rpm_macros_allowed, then the values there might not actually be valid macros.

In the validation of the rpm_macros_allowed value, we're still calling _valid_rpm_macro_name(macro), but since we're accepting patterns, the pattern might not be a valid name. I think the type check is enough here, since we're validating the macro names to set later.

The docs and docstring refer to setting to "1/True", but it doesn't look like the code handles this anymore. Setting the pattern to "*" is an easy way to allow anything. We could add a special case for other types, but it might be simpler not to.

Actually at the moment, it looks like the code will allow any macro value if rpm_macros_allowed is set to any non-list value.

Given that specifying lists in edit-tag can be a little awkward and that we elsewhere have a pattern of accepting pattern lists as a space separated string (i.e. .split()), it would be nice to accept this here as well.

I suggest the following:

• we require rpm_macros_allowed to be a string, a list of strings, or None/unset
• any invalid setting here is treated as None (not allowed) and logs an error
• If we also want the special case for True/False, that's fine, but if so I'd like to be strict about the type in this case, since there is potential confusion with polymorphism.

I'm for '*' solution. No need for other variants now.

Hmm, looks like we're still allowing modification when rpm_macros_allowed is not a list. Need to handle the None case.

also, trailing whitespace :wink:

I've put all rpm_macros_allowed checks to one place for better readability.

Oh, that is much cleaner. Thanks!

    for macro in rpm_macros_allowed:
        if not isinstance(macro, str):
            raise koji.GenericError(f"Allowed rpm macro list {rpm_macros_allowed:r} "
                                    f"is invalid for {parent['name']}.")

This will error even in cases where we don't need to check rpm_macros_allowed

           if not _valid_rpm_macro_name(macro):
               raise koji.GenericError(f"Invalid macro {macro:r} in {parent['name']}, setting "
                                       "sidetag_rpm_macros_allowed=False.")

This error text seems a bit out of place.

Perhaps something like this?
https://pagure.io/fork/mikem/koji/commits/pr3674updates

:thumbsup: - cherry-picked

:thumbsup:

Commit c82543a6 fixes this pull-request