Skip to content

Fix cascading settings attr writer behavior

Drew Blessing requested to merge dblessing_cascading_override into master

What does this MR do?

Fixes #327172 (closed)

  • If set value matches the cascaded value, do not save locally. This fixes an issue where a simple settings form save by a subgroup would cause a cascaded value to be saved at the subgroup level, defeating future cascading lookups.
  • When setting a lock attribute and the local setting is nil, copy the value locally to ensure ancestors can't change the intended value for subgroups later.

Stated problem from the issue description:

Currently with the cascading setting framework it is possible to unintentionally override the inherited setting value with these steps:

  1. Create a group
  2. In Group -> Settings -> General -> Permissions, LFS, 2FA check the Enable delayed project removal setting and click save.
  3. Create a subgroup
  4. In Subgroup -> Settings -> General -> Permissions, LFS, 2FA the Enable delayed project removal setting should be enabled. Change an unrelated setting in the Permissions, LFS, 2FA section and click save.
  5. In Group -> Settings -> General -> Permissions, LFS, 2FA uncheck the Enable delayed project removal setting and click save.
  6. In Subgroup -> Settings -> General -> Permissions, LFS, 2FA the Enable delayed project removal settings is still checked when it shouldn't be.

Here is a video to help understand the issue: https://www.loom.com/share/99ec8c1b10df48d3b99b29db0b4edc5d

As an MVC fix we should only save the delayed_project_removal attribute if it is different than the inherited value. This will prevent users from accidentally overriding an inherited setting when saving a unrelated setting.

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Merge request reports

Loading