Feat/add role user access controls - #749
Infinite-Null wants to merge 63 commits into
Conversation
…trolSettings to enable explicit saving
… update dependencies
…ion in AccessControlSettings
…ction tracking in AccessControlSettings
… and refresh lockfile
…pdate clear hook type definition
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
…ication logic in helper function
…ic to reset access control settings upon toggle
…rom suggestions in AccessControlSettings
…me variables for consistent styling
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #749 +/- ##
=============================================
+ Coverage 80.87% 81.61% +0.73%
- Complexity 2998 3089 +91
=============================================
Files 124 130 +6
Lines 11987 12368 +381
=============================================
+ Hits 9695 10094 +399
+ Misses 2292 2274 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@Infinite-Null FYI some merge conflicts to resolve to keep this moving along |
…e2e test setup to prevent cross-test interference
|
…cess control UI to specific admin features
…ude them from controller responses
|
Hi @jeffpaul, Thanks for the feedback! I’ve updated the PR accordingly. I’ve also removed both Subscriber and Contributor from the listed roles. |
|
The failing tests seem to be unrelated to the changes introduced in this PR. |
dkotter
left a comment
dkotter
left a comment
There was a problem hiding this comment.
Handful of things that still need addressed here.
I've tested and things appear to work fine though I do have a question/concern on the default state.
When I first toggle on Access controls no user roles are checked. This makes it appear like no one should have access but our default here is if nothing is set, everyone has access. I wonder if that should be the default?
Either we need to make that more clear in the UI (maybe you need to turn on Access controls and then separately toggle that on for each individual feature? Or just better messaging here if nothing is selected it tells you everyone has access?).
Or we need some smart defaults (maybe have Administrator selected by default for all of these). And if someone deselects all options, that feature is now no longer accessible to anyone.
Otherwise the default state doesn't match user expectations and you also have the state where someone checks Administrator and later unchecks that and they'll assume no one has access but in reality, everyone has access.
|
|
||
| printf( | ||
| '<div class="ai-alt-text-media-actions" style="margin-top: 16px;">' . | ||
| '<button id="ai-alt-text-generate-button" class="button button-secondary" type="button" data-attachment-id="%1$d">%2$s</button>' . |
There was a problem hiding this comment.
Any reason for these spacing changes? I'm assuming just some automated linting but would keep diffs cleaner to revert that
| * {@inheritDoc} | ||
| */ | ||
| public function register(): void { | ||
| if ( ! \WordPress\AI\current_user_can_access_feature( $this->get_id() ) ) { |
There was a problem hiding this comment.
Wondering if there's a way we could implement this in the Abstract_Feature class instead of having to duplicate this in every individual feature class? Just seems like a lot of duplicated code that could maybe be handled differently
There was a problem hiding this comment.
As a practical example here, there are a number of features that never call current_user_can_access_feature right now, even though we show those settings in the UI.
Seems we're missing:
- Slug generation
- Type Ahead
- Content Translation
- Image Generation
| * {@inheritDoc} | ||
| */ | ||
| public function register(): void { | ||
| if ( ! \WordPress\AI\current_user_can_access_feature( $this->get_id() ) ) { |
There was a problem hiding this comment.
If we do keep this here, we should follow the approach of doing
use function WordPress\AI\current_user_can_access_feature;So this can then be simplified to just current_user_can_access_feature()
| } | ||
|
|
||
| /** | ||
| * Registers experiment infrastructure. |
There was a problem hiding this comment.
| * Registers experiment infrastructure. | |
| * {@inheritDoc} |
| * @since 1.2.0 | ||
| */ | ||
| public function register(): void { | ||
| $this->register_infrastructure(); |
There was a problem hiding this comment.
Is this needed? Doesn't seem we override that here
| }, [ effectiveUsers, selectedUserMap ] ); | ||
|
|
||
| // Seed selectedUserMap with users returned from the API (capped at | ||
| // MAX_USERS i.e. 10 at a time). If more than |
There was a problem hiding this comment.
So there may be a bug here if someone actually wants to set more than 10 users. I haven't verified directly but looking at the code, it seems anything set over 10 will be dropped on save
| } | ||
|
|
||
| $users = array(); | ||
| $wp_users = get_users( $get_users_args ); |
There was a problem hiding this comment.
We have a few restrictions in place on contributor and subscriber roles but we don't apply that here, meaning I can select a user that is a subscriber even though that role is rejected in other places. I think the user query here needs to remove those users
| if ( array_intersect( $current_user->roles, array( 'subscriber', 'contributor' ) ) ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
What if a site wants to add subscribers or contributors and give them access? At the moment, that's not possible. Wondering if we need to rework this function so all of our return values pass through the wpai_user_has_role_access filter?
| * @since x.x.x | ||
| */ | ||
| protected function register_infrastructure(): void { | ||
| $this->register_post_meta(); |
There was a problem hiding this comment.
I think enqueue_block_assets needs to go here as well as that loads front-end assets
|
|
||
| register_setting( | ||
| self::OPTION_GROUP, | ||
| "wpai_feature_{$feature_id}_users", |
There was a problem hiding this comment.
This setting will be included in our settings export. That may be fine but this means if someone exports from one site to another and they've given specific users access, the user ID's are likely not the same on the site they are importing too and could result in them giving access to users they don't intend to. May want to consider not exporting this data
4 participants
Closes: #736
Description
This PR introduces Feature-Level Access Controls exclusively for Editor Experiments, allowing site administrators to restrict access to individual AI-powered editor features based on specific WordPress roles or individual users.
Testing Instructions
Screencast
User: Ankit Shah | Role: Editor
Screen.Recording.2026-06-22.at.1.35.17.PM-compressed.mp4
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Sonnet 4.6
Used for: Validating bug, suggesting a fix.
Changelog Entry