[RHCLOUD-33414] - restrict the Custom default access group renaming

[EvanCasey13]

Link(s) to Jira

https://issues.redhat.com/browse/RHCLOUD-33414

Description of Intent of Change(s)

Local Testing

How can the feature be exercised?
How can the bug be exploited and fix confirmed?
Is any special local setup required?

Checklist

• if API spec changes are required, is the spec updated?
• are there any pre/post merge actions required? if so, document here.
• are theses changes covered by unit tests?
• if warranted, are documentation changes accounted for?
• does this require migration changes?
  • if yes, are they backwards compatible?
• is there known, direct impact to dependent teams/components?
  • if yes, how will this be handled?

Secure Coding Practices Checklist Link

• https://github.com/RedHatInsights/secure-coding-checklist

Secure Coding Practices Checklist

• Input Validation
• Output Encoding
• Authentication and Password Management
• Session Management
• Access Control
• Cryptographic Practices
• Error Handling and Logging
• Data Protection
• Communication Security
• System Configuration
• Database Security
• File Management
• Memory Management
• General Coding Practices

[EvanCasey13]

/retest

[astrozzc]

If you put this after the group = self.get_object(), you can check if the group is custom default group by group.platform_default. Which will simplify your method a lot

[astrozzc]

Since you get the group above, you can put it into the method. If the name/description different from group.name/description, raise validation error

[EvanCasey13]

/retest

[astrozzc]

LGTM!

[petracihalova]

hi Evan, thank you for the change, but we need to review it a little bit, please after you try to change the Custom default access group and you get 400 (that is expected), check the data from database and you will see the record was changed, that is because self.restrict_custom_default_group_renaming(group) is called after group = super().update(instance, validated_data).

    def update(self, instance, validated_data):
        """Update the role object in the database."""
        group = super().update(instance, validated_data)
        self.restrict_custom_default_group_renaming(group)
        group_obj_change_notification_handler(self.context["request"].user, group, "updated")
        return group

I think we should use different direction here, I am open to discussion because I am not so experienced here and maybe we can use something similar as protect_system_groups() in the view (here), and we should still keep in mind that it is possible to delete this group and add and remove roles from it

one option is to add new condition into protect_system_groups(), for example something like

    def protect_system_groups(self, action, group=None):
        """Deny modifications on system groups."""
        if group is None:
            group = self.get_object()
        if group.system:
            key = "group"
            message = "{} cannot be performed on system groups.".format(action.upper())
            error = {key: [_(message)]}
            raise serializers.ValidationError(error)
        if group.platform_default and action == "update":
            key = "group"
            message = "It is not allowed to change name or description of the 'Custom default access' group."
            error = {key: [_(message)]}
            raise serializers.ValidationError(error)

cc: @lpichler @astrozzc

[astrozzc]

Yea, thanks Petra, good catch!!! And I agree putting the function inside the view seems to be appropriate.

The protect_system_groups has been used in several places, it is better have a separate function restrict_custom_default_group_renaming next to protect_system_groups, so that we don't run this checking of renaming logic only when necessary.