Skip to content

RFC 105: Site setting permissions#105

Merged
thibaudcolas merged 1 commit intowagtail:mainfrom
gasman:site-setting-permissions
May 20, 2025
Merged

RFC 105: Site setting permissions#105
thibaudcolas merged 1 commit intowagtail:mainfrom
gasman:site-setting-permissions

Conversation

@gasman
Copy link
Contributor

@gasman gasman commented Jan 24, 2025

Rendered

Currently, permissions on site settings are governed by a standard global Django permission record on the model, meaning that users have edit permission for the settings of all sites, or none. This does not fit well with organisational structures where a member of staff has overall responsibility for one particular site. This RFC proposes a new permission model where groups can be assigned permissions at the level of individual sites.

@gasman gasman added the 1:New label Jan 24, 2025
Copy link
Member

@Stormheg Stormheg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a sensible approach to me. Well explained with good examples - kudos @gasman 👍 👌

@laymonage what do you think? You have more in-depth knowledge of how the permission system works in Wagtail.

Comment on lines +43 to +45
For this reason, we will preserve the ability to set permissions globally for a settings model via standard Django permission records, and respect these as part of the permission-checking logic.

This also avoids the complication of performing a data migration to move existing Django permission records to `GroupSitePermission` as part of the rollout of this feature. This would be decidedly non-trivial, especially if we wish to avoid developers having to write their own data migrations as part of upgrading Wagtail (since this would involve having a central migration somewhere within Wagtail that operates on some set of models that isn't known in advance).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 🙏

Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the well-written RFC! Seems sensible and fairly straightforward to fit into the current state of play for permissions.

If we had the first bit of #102 (registry for permission policies), it would make it easier to swap the SitePermissionPolicy for projects that do need it. But I don't think that'd be necessary for most use cases.

Perhaps the quirk of "site setting records are created when they are first saved" will impact how we proceed with #102, but for now I'm happy for us to work on SitePermissionPolicy first.

Edit: @Stormheg you beat me to it 😆


On submitting the form, a `SitePermissionPolicy` record will be created for each item checked, corresponding to the `change_...` permission for the given settings model and the given site.

Additionally, settings models will continue to be listed in the "Object permissions" section of the page, as they are now, to allow granting the permission globally for all sites. We should consider extending the `register_permissions` hook to allow passing help text to display here, to explain the difference between this checkbox and the site-by-site options. For example, this might read: "Allow changing social media settings globally for all sites. To set this on a per-site basis, see 'Social media settings permissions' below."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, without this it'd be confusing to users as they wouldn't know the difference between the two, as well as the implications of enabling the setting's model permission.

@thibaudcolas
Copy link
Member

I see this has been approved plenty so will merge this now. Nice one!

@thibaudcolas thibaudcolas merged commit 076d37b into wagtail:main May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants