Fix cascading settings attr writer behavior
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:
- Create a group
- In
Group
->Settings
->General
->Permissions, LFS, 2FA
check theEnable delayed project removal
setting and click save. - Create a subgroup
- In
Subgroup
->Settings
->General
->Permissions, LFS, 2FA
theEnable delayed project removal
setting should be enabled. Change an unrelated setting in thePermissions, LFS, 2FA
section and click save. - In
Group
->Settings
->General
->Permissions, LFS, 2FA
uncheck theEnable delayed project removal
setting and click save. - In
Subgroup
->Settings
->General
->Permissions, LFS, 2FA
theEnable 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
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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