Fix profile links not showing for vendor staff #3040
Conversation
📝 WalkthroughWalkthroughAdds a VendorAuthorizable trait and integrates vendor/staff-aware vendor ID resolution and permission checks into StoreController and StoreSettingController; REST GET/PUT endpoints now validate and resolve vendor IDs, enforce access via the trait, and return 403/404 as appropriate. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(247,250,252,0.9)
participant Client
participant REST as StoreController / StoreSettingController
participant Trait as VendorAuthorizable
participant Util as VendorUtil / WP DB
end
Client->>REST: HTTP GET/PUT /stores/:id or /settings?vendor_id=...
REST->>Trait: validate_store_id(id, request)
Trait->>Util: resolve vendor id / check existence
Util-->>Trait: vendor_id or WP_Error (not found)
alt invalid vendor
Trait-->>REST: WP_Error (404)
REST-->>Client: 404 Not Found
else resolved
Trait-->>REST: resolved_vendor_id
REST->>Trait: can_access_vendor_store(resolved_vendor_id, current_user)
Trait->>Util: check role & vendor meta
Util-->>Trait: authorized? (bool)
alt authorized
REST->>Util: fetch/update store by resolved_vendor_id
Util-->>REST: store data or WP_Error
alt success
REST-->>Client: 200 OK (prepared, filtered response)
else error
REST-->>Client: error response
end
else unauthorized
REST-->>Client: 403 rest_forbidden
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/php/src/DokanTestCase.php (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
includes/REST/StoreController.php (1)
368-386: LGTM! Permission logic correctly validates staff access.The three-tier permission check (admin → owner → staff) correctly allows staff members to update their vendor's store settings. The type casting on line 381 ensures safe comparison.
Optional: Minor type safety improvement
Consider using
absint()forstaff_vendor_idfor consistency with line 374:$staff_vendor_id = get_user_meta( $current_user, '_vendor_id', true ); - if ( $staff_vendor_id && (int) $staff_vendor_id === $requested_id ) { + if ( absint( $staff_vendor_id ) === $requested_id ) { return true; }This approach is more idiomatic in WordPress and handles edge cases like empty strings more safely.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/REST/StoreController.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/REST/StoreController.php (2)
includes/Vendor/Manager.php (1)
get(131-133)includes/Order/Manager.php (1)
get(270-272)
🪛 GitHub Actions: Inspections
includes/REST/StoreController.php
[error] 1-1: phpcs: Expected 1 space after closing brace; newline found.
🪛 GitHub Check: Run PHPCS inspection
includes/REST/StoreController.php
[failure] 322-322:
Expected 1 space after closing brace; newline found
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
includes/REST/StoreController.php (1)
367-385: Improve type safety and consistency for staff vendor ID handling.The permission logic correctly allows staff to update their vendor's store. However, the type casting can be improved to match the method's own pattern at line 373.
Use
absint()when retrievingstaff_vendor_idfor consistency withrequested_idhandling and to ensure non-negative integer values from the start:$current_user = get_current_user_id(); $requested_id = absint( $request->get_param( 'id' ) ); if ( current_user_can( 'dokandar' ) && $current_user === $requested_id ) { return true; } - $staff_vendor_id = get_user_meta( $current_user, '_vendor_id', true ); - if ( $staff_vendor_id && (int) $staff_vendor_id === $requested_id ) { + $staff_vendor_id = absint( get_user_meta( $current_user, '_vendor_id', true ) ); + if ( $staff_vendor_id && $staff_vendor_id === $requested_id ) { return true; } return false;
♻️ Duplicate comments (1)
includes/REST/StoreController.php (1)
313-329: Critical: Staff members still cannot view other vendors' public stores.The previous review concern remains unresolved. This implementation unconditionally overrides the requested
store_idwith the staff member'svendor_idfor every request, preventing staff from viewing any store except their own vendor's store.Issue: Since this is a public endpoint (
permission_callback => '__return_true'at line 71), the current logic breaks expected API behavior. Staff members should be able to view any public store like other users.Root cause: The code doesn't verify whether the staff member is requesting their own data versus viewing another vendor's public store.
🔎 Recommended fix
The solution depends on the intended use case:
Option 1: If staff should only see vendor data when explicitly requesting their own profile/settings (most likely scenario based on PR objectives):
public function get_store( $request ) { $store_id = (int) $request['id']; $current_user = get_current_user_id(); - $staff_vendor_id = get_user_meta( $current_user, '_vendor_id', true ); + $staff_vendor_id = absint( get_user_meta( $current_user, '_vendor_id', true ) ); - if ( $staff_vendor_id ) { - $vendor_id = (int) $staff_vendor_id; - $store = dokan()->vendor->get( $vendor_id ); - } else { + // If staff member is requesting their own data, use vendor_id instead + if ( $staff_vendor_id && $store_id === $current_user ) { + $store = dokan()->vendor->get( $staff_vendor_id ); + } else { $store = dokan()->vendor->get( $store_id ); } $stores_data = $this->prepare_item_for_response( $store, $request ); - $response = rest_ensure_response( $stores_data ); + $response = rest_ensure_response( $stores_data ); return $response; }This ensures staff can view any public store, but when accessing their own profile (e.g.,
/stores/{staff_user_id}), they see the vendor's data instead.Option 2: If the endpoint should be restricted to dashboard access only, change the permission callback to prevent public access and require authentication.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/REST/StoreController.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/REST/StoreController.php (2)
includes/Vendor/Manager.php (1)
get(131-133)includes/Order/Manager.php (1)
get(270-272)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
midu-01
added
Needs: Testing
includes/REST/StoreController.php
Outdated
| */ | ||
| public function get_store( $request ) { | ||
| $store_id = (int) $request['id']; | ||
| $current_user = get_current_user_id(); |
There was a problem hiding this comment.
We may simplify by following the bellow steps:
- Check the requested id(
$store_id) is the seller ID usingdokan_is_user_seller( $store_id, true ). It will ensure that the requested ID is the vendor ID. - Otherwise, Fetch the vendor ID for the current user using
dokan_get_current_user_id()
Suggested code:
if ( ! dokan_is_user_seller( $store_id, true ) ) {
$store_id = dokan_get_current_user_id();
}
// The existing codeThere was a problem hiding this comment.
Yes, this can be done using the suggested function. At that time, I did not notice the availability of the dokan_is_user_seller() function. Thank you for pointing this out, bhai. I will update the implementation accordingly.
includes/REST/StoreController.php
Outdated
| if ( current_user_can( 'dokandar' ) ) { | ||
| return dokan_get_current_user_id() === absint( $request->get_param( 'id' ) ); | ||
| $current_user = get_current_user_id(); | ||
| $requested_id = absint( $request->get_param( 'id' ) ); | ||
|
|
||
| if ( current_user_can( 'dokandar' ) && $current_user === $requested_id ) { | ||
| return true; | ||
| } | ||
|
|
||
| $staff_vendor_id = get_user_meta( $current_user, '_vendor_id', true ); | ||
| if ( $staff_vendor_id && (int) $staff_vendor_id === $requested_id ) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Do we need to change the permission?
There was a problem hiding this comment.
Bhai,
Yes, we need to handle the permission here. Staff users should not be able to add or update this profile link unless the appropriate permission is explicitly granted. That is why I implemented this logic specifically for controlling staff update permissions.
Please let me know if you would like any adjustments to this approach.
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
includes/REST/StoreController.php (1)
393-421: Consider the redundancy with permission checks.The logic at lines 397-403 overrides
requested_idfor staff users, ensuring they update their vendor's store. However, the permission check (lines 374-378) already validates thatstaff_vendor_id === requested_id, so by the time execution reaches this method, the override is redundant (they already match).That said, this redundancy provides defense in depth and ensures staff always update their vendor's store even if permission checks are modified in the future. The current implementation is safe and acceptable.
Optional: If you prefer to eliminate redundancy
Since permission checks already ensure
staff_vendor_id === requested_id, you could simplify to:public function update_store( $request ) { - $requested_id = absint( $request->get_param( 'id' ) ); - $current_user = get_current_user_id(); - - if ( user_can( $current_user, 'vendor_staff' ) ) { - $staff_vendor_id = (int) get_user_meta( $current_user, '_vendor_id', true ); - - if ( $staff_vendor_id ) { - $requested_id = $staff_vendor_id; - } - } - - $store = dokan()->vendor->get( $requested_id ); + $store_id = absint( $request->get_param( 'id' ) ); + $store = dokan()->vendor->get( $store_id );However, keeping the current defensive approach is perfectly reasonable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/REST/StoreController.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/REST/StoreController.php (1)
includes/functions.php (2)
dokan_is_user_seller(73-79)dokan_get_current_user_id(48-61)
🪛 GitHub Actions: Inspections
includes/REST/StoreController.php
[error] 1-1: Whitespace found at end of line. Command: vendor/bin/phpcs includes/REST/StoreController.php -q --report=checkstyle | cs2pr --graceful-warnings
🪛 GitHub Check: Run PHPCS inspection
includes/REST/StoreController.php
[failure] 317-317:
Line indented incorrectly; expected at least 3 tabs, found 2 tabs and 3 spaces
[warning] 317-317:
Found precision alignment of 3 spaces.
[failure] 315-315:
Whitespace found at end of line
[failure] 383-383:
Whitespace found at end of line
[failure] 1157-1157:
Expected 1 newline at end of file; 0 found
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (2)
includes/REST/StoreController.php (2)
362-381: LGTM! Permission logic correctly restricts staff access.The permission check properly allows:
- Admins with
manage_woocommercecapability- Vendors updating their own store (lines 370-372)
- Staff users updating only their parent vendor's store (lines 374-378)
This ensures staff cannot update other vendors' stores while still allowing them to update their own vendor's store settings.
313-324: The concern about public endpoint behavior is partially valid but the specific scenario needs clarification.The
GET /stores/{id}endpoint is marked as public (permission_callback => '__return_true'), but the new logic redirects non-seller IDs to the current user's vendor. Analysis shows:When the logic DOES trigger (requested ID is not a valid seller):
- Staff user requesting an invalid/non-seller ID → redirects to their own vendor
- Anonymous user requesting an invalid/non-seller ID → redirects to ID 0 (which may error)
When the logic DOES NOT trigger (requested ID is a valid seller):
- Staff user requesting another valid vendor's store → returns that store's data as expected
- Anonymous user requesting a valid vendor's store → returns that store's data as expected
The actual issue is narrower than the review suggests: the fallback logic may cause unexpected behavior for non-existent vendor requests rather than blocking access to valid vendors. However, the broader point stands—marking this as a public endpoint while adding user-context-dependent fallback logic requires careful consideration of the intended API contract. If the endpoint should serve both public store directories (anonymous access) and admin dashboards (authenticated access), the fallback should distinguish between these cases rather than always redirecting to the current user.
mrabbani
left a comment
mrabbani
left a comment
There was a problem hiding this comment.
- Check user capability
- Ensure that the user has the proper access to the requested Store.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
includes/REST/StoreController.php (1)
315-327: Critical: This implementation breaks the public store retrieval API.The endpoint
GET /stores/{id}is public (permission_callback => '__return_true'at line 73) and should allow anyone to retrieve any public store by ID. However, this implementation:
- Completely ignores the requested store ID from the URL parameter
- Always uses
dokan_get_current_user_id(), which returns the vendor ID for staff or the current user ID otherwise- Breaks legitimate use cases:
- Staff members cannot view any store except their vendor's store
- Customers and guests cannot view any stores (user ID 0 or non-vendor ID will fail)
- The URL parameter
{id}becomes meaninglessRoot cause confusion: The PR objective is to fix social profile links not showing for staff. However, this is a general store retrieval endpoint, not a social-profile-specific endpoint. The fix should target the specific endpoint or data layer where social profiles are fetched, not break the public store API.
Expected behavior:
- Public users should be able to GET any store by ID
- The social profile bug should be fixed in the social profile data preparation/response logic, not by changing which store is retrieved
🔎 Recommended approach
If the issue is that social profile data within the store response uses the wrong user ID, fix it in the
prepare_item_for_responsemethod or the underlying social profile data retrieval, not by changing which store is loaded:public function get_store( $request ) { - $store_id = dokan_get_current_user_id(); - - if( ! ($this->check_vendor_authorizable_permission( $store_id )) ){ - return new WP_Error( 'rest_forbidden', __( 'You do not have permissions to access this store.', 'dokan' ), [ 'status' => 403 ] ); - } - - $request['id'] = $store_id; + $store_id = (int) $request['id']; $store = dokan()->vendor->get( $store_id ); + + if ( ! $store || ! $store->get_id() ) { + return new WP_Error( 'rest_no_store', __( 'Store not found.', 'dokan-lite' ), [ 'status' => 404 ] ); + } + $stores_data = $this->prepare_item_for_response( $store, $request ); $response = rest_ensure_response( $stores_data ); return $response; }Then fix the social profile data retrieval in the data preparation layer to use the vendor's social profiles when preparing a vendor's store data, regardless of who is viewing it.
🧹 Nitpick comments (1)
includes/REST/StoreController.php (1)
411-411: Fix equals sign alignment (PHPCS warning).There are 2 spaces before the equals sign instead of 1, causing a PHPCS warning.
🔎 Proposed fix
- $store = dokan()->vendor->get( $requested_id ); + $store = dokan()->vendor->get( $requested_id );Based on pipeline failure logs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/REST/StoreController.phpincludes/Traits/VendorAuthorizable.php
🧰 Additional context used
🧬 Code graph analysis (2)
includes/Traits/VendorAuthorizable.php (1)
includes/functions.php (1)
dokan_is_user_seller(73-79)
includes/REST/StoreController.php (2)
includes/functions.php (1)
dokan_get_current_user_id(48-61)includes/Traits/VendorAuthorizable.php (1)
check_vendor_authorizable_permission(23-49)
🪛 GitHub Actions: Inspections
includes/Traits/VendorAuthorizable.php
[error] 1-1: PHP_CodeSniffer: The closing brace for the trait must go on the next line after the body.
🪛 GitHub Check: Run PHPCS inspection
includes/REST/StoreController.php
[failure] 319-319:
Mismatched text domain. Expected 'dokan-lite' but got 'dokan'.
[failure] 318-318:
Expected 1 space after closing parenthesis; found 0
[failure] 318-318:
Expected 1 space before close parenthesis; 0 found
[failure] 318-318:
Expected 1 space after open parenthesis; 0 found
[failure] 318-318:
Expected 1 space after IF keyword; 0 found
[failure] 318-318:
No space before opening parenthesis is prohibited
[failure] 318-318:
Space after opening control structure is required
[failure] 317-317:
Whitespace found at end of line
[failure] 389-389:
Whitespace found at end of line
[warning] 411-411:
Equals sign not aligned correctly; expected 1 space but found 2 spaces
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (1)
includes/Traits/VendorAuthorizable.php (1)
16-49: Authorization logic looks correct.The method properly handles three authorization scenarios:
- Admins can access any vendor via
manage_options- Vendors can access their own store (with staff excluded via the second parameter)
- Staff can access their assigned vendor's store via
_vendor_idmetadataThe implementation aligns with the PR objective to resolve staff users to their parent vendor for store data access.
| return false; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix PHPCS formatting: trait closing brace placement.
The closing brace for the trait must appear on the next line after the body, per coding standards.
🔎 Proposed fix
Ensure there's a blank line before the closing brace:
return false;
}
-
+
}Based on pipeline failure logs.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 51-51:
The closing brace for the trait must go on the next line after the body
🤖 Prompt for AI Agents
In includes/Traits/VendorAuthorizable.php around line 51, the trait's closing
brace is currently on the same line as the last statement; move the closing
brace to its own line and ensure there is a blank line separating the trait body
and the closing brace to comply with PHPCS formatting (i.e., end the last
method/body with a newline, add a blank line, then place the closing brace on
the next line).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
includes/Traits/VendorAuthorizable.php (1)
16-23: Consider adding type hint and parameter documentation.The
$vendor_idparameter lacks a type hint and is not documented in the docblock. Adding these would improve type safety and developer experience.🔎 Suggested improvement
/** * Check if passing id has vendor authorizable permission. * * @since 3.14.11 * + * @param int $vendor_id The vendor/store ID to check permission for. + * * @return bool */ - public function check_vendor_authorizable_permission( $vendor_id ) { + public function check_vendor_authorizable_permission( int $vendor_id ) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Traits/VendorAuthorizable.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/Traits/VendorAuthorizable.php (1)
includes/functions.php (1)
dokan_is_user_seller(73-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (3)
includes/Traits/VendorAuthorizable.php (3)
24-28: Good defensive validation.The early vendor existence and ID validation protects against invalid inputs and prevents potential edge cases in the authorization logic below.
30-46: Authorization logic is sound and secure.The three-tier permission model (admin → vendor → staff) correctly implements the security requirements:
- Admins can access any vendor store via
manage_options.- Vendors can only access their own store, with staff explicitly excluded via
dokan_is_user_seller($current_user_id, true).- Staff members are authorized based on their
vendor_staffcapability and matching_vendor_idmeta.The strict integer comparisons and capability checks provide defense-in-depth authorization.
48-48: Correct fail-closed behavior.The default
return falseimplements proper deny-by-default security, ensuring access is denied unless explicitly authorized.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
includes/Traits/VendorAuthorizable.php (1)
76-76: Fix PHPCS formatting: trait closing brace placement.The closing brace for the trait must appear on the next line after the body, with a blank line before it, per coding standards.
🔎 Proposed fix
return null; } + }Based on pipeline failure logs and past review comments.
🧹 Nitpick comments (1)
includes/Traits/VendorAuthorizable.php (1)
45-47: Consider adding explicit validation for staff vendor ID.While vendor IDs are user IDs that start at 1, adding an explicit check that
$staff_vendor_idis non-zero would make the intent clearer and guard against edge cases where the_vendor_idmeta is missing or empty.🔎 Proposed enhancement
if ( user_can( $current_user_id, 'vendor_staff' ) ) { $staff_vendor_id = (int) get_user_meta( $current_user_id, '_vendor_id', true ); - return $staff_vendor_id === (int) $vendor_id; + return $staff_vendor_id > 0 && $staff_vendor_id === (int) $vendor_id; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/REST/StoreController.phpincludes/Traits/VendorAuthorizable.php
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/REST/StoreController.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/Traits/VendorAuthorizable.php (1)
includes/functions.php (1)
dokan_is_user_seller(73-79)
🪛 GitHub Check: Run PHPCS inspection
includes/Traits/VendorAuthorizable.php
[failure] 71-71:
Using short ternaries is not allowed as they are rarely used correctly
[failure] 63-63:
Using short ternaries is not allowed as they are rarely used correctly
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (3, 3)
| * @return int|false|null Vendor ID on success, false if staff has no vendor, null if not vendor or staff. | ||
| */ | ||
| public function get_user_vendor_id( $user_id = null ) { | ||
| $user_id = $user_id ?: get_current_user_id(); |
There was a problem hiding this comment.
Replace short ternary with null coalescing operator.
The coding standard prohibits short ternaries. Use the null coalescing operator for clarity and compliance.
🔎 Proposed fix
- $user_id = $user_id ?: get_current_user_id();
+ $user_id = $user_id ?? get_current_user_id();Based on static analysis hints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $user_id = $user_id ?: get_current_user_id(); | |
| $user_id = $user_id ?? get_current_user_id(); |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 63-63:
Using short ternaries is not allowed as they are rarely used correctly
🤖 Prompt for AI Agents
In includes/Traits/VendorAuthorizable.php around line 63, replace the short
ternary usage that assigns a fallback current user id with the null coalescing
operator; specifically, change the line that uses "?:" so it instead uses "??"
to fallback to get_current_user_id() when $user_id is null, ensuring compliance
with the coding standard.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Traits/VendorAuthorizable.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/Traits/VendorAuthorizable.php (1)
includes/functions.php (1)
dokan_is_user_seller(73-79)
🪛 GitHub Check: Run PHPCS inspection
includes/Traits/VendorAuthorizable.php
[failure] 29-29:
Expected 1 space after IF keyword; 0 found
[failure] 29-29:
No space before opening parenthesis is prohibited
[failure] 29-29:
Space after opening control structure is required
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run PHPCS inspection
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (2)
includes/Traits/VendorAuthorizable.php (2)
33-55: Authorization logic correctly implements vendor/staff access control.The method properly validates:
- Vendor existence before authorization checks
- Admin override via
manage_woocommercecapability- Vendor-only access to their own store (excludes staff via
dokan_is_user_seller(..., true))- Staff access limited to their assigned vendor store via
_vendor_idmetaThe authorization hierarchy and type-safe comparisons align with the PR objective of resolving vendor IDs for staff users.
66-86: Vendor ID resolution correctly handles vendors and staff.The method appropriately:
- Defaults to current user when
$user_idis not provided- Returns the user's own ID for vendors (excluding staff)
- Returns the parent vendor ID for staff members via
_vendor_idmeta- Distinguishes between staff-with-no-vendor (false) and non-vendor-users (null)
This implementation directly addresses the PR objective of resolving the correct vendor ID for staff users to fetch parent vendor data.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
includes/Traits/VendorAuthorizable.php (1)
1-1: Fix trailing whitespace.Based on pipeline failure logs.
🔎 Proposed fix
Remove the trailing whitespace at the end of line 1.
-<?php +<?php namespace WeDevs\Dokan\Traits;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/REST/StoreController.phpincludes/Traits/VendorAuthorizable.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T06:19:41.376Z
Learnt from: mrabbani
Repo: getdokan/dokan PR: 2891
File: src/admin/dashboard/pages/vendors-single/InformationTabs/WithdrawTab.tsx:58-59
Timestamp: 2025-09-19T06:19:41.376Z
Learning: The Dokan codebase uses 'dokan-lite' as the text domain for WordPress internationalization (__() function calls).
Applied to files:
includes/REST/StoreController.php
🧬 Code graph analysis (2)
includes/Traits/VendorAuthorizable.php (1)
includes/functions.php (1)
dokan_is_user_seller(73-79)
includes/REST/StoreController.php (1)
includes/Traits/VendorAuthorizable.php (2)
check_vendor_authorizable_permission(28-55)get_user_vendor_id(66-86)
🪛 GitHub Actions: Inspections
includes/Traits/VendorAuthorizable.php
[error] 1-1: Whitespace found at end of line.
includes/REST/StoreController.php
[error] 1-1: Whitespace found at end of line.
🪛 GitHub Check: Run PHPCS inspection
includes/REST/StoreController.php
[failure] 319-319:
Whitespace found at end of line
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (3)
includes/Traits/VendorAuthorizable.php (1)
17-86: LGTM! Well-designed authorization helpers.The two new methods provide clear, centralized logic for vendor-based authorization:
check_vendor_authorizable_permissionproperly validates vendor existence and enforces role-based access (admin, seller, staff)get_user_vendor_idcleanly resolves the effective vendor ID for both direct vendors and staff membersincludes/REST/StoreController.php (2)
5-5: LGTM! Trait integration is correct.The VendorAuthorizable trait is properly imported and applied to the controller, enabling the new authorization helpers.
Also applies to: 22-22
393-399: LGTM! Proper error handling added.The 404 error response for missing stores is appropriate and uses the correct text domain.
includes/REST/StoreController.php
Outdated
| if ( ! ( $this->check_vendor_authorizable_permission( $request['id'] ) ) ) { | ||
| return new WP_Error( 'rest_forbidden', __( 'You do not have permissions to access this store.', 'dokan-lite' ), [ 'status' => 403 ] ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix trailing whitespace.
Based on pipeline failure logs.
🔎 Proposed fix
Remove the trailing whitespace at the end of line 319.
return new WP_Error( 'rest_forbidden', __( 'You do not have permissions to access this store.', 'dokan-lite' ), [ 'status' => 403 ] );
}
-
+
$store = dokan()->vendor->get( $request['id'] );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new WP_Error( 'rest_forbidden', __( 'You do not have permissions to access this store.', 'dokan-lite' ), [ 'status' => 403 ] ); | |
| } | |
| $store = dokan()->vendor->get( $request['id'] ); |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 319-319:
Whitespace found at end of line
🤖 Prompt for AI Agents
In includes/REST/StoreController.php at line 319, there is a trailing whitespace
character causing pipeline/lint failures; remove the trailing space at the end
of line 319 (and optionally run a quick trim of end-of-line whitespace across
the file or project to ensure no other trailing spaces remain) so the line ends
cleanly with no extra whitespace.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
includes/Traits/VendorAuthorizable.php (1)
1-89: Remove trailing whitespace to fix pipeline failure.The PHPCS inspection has detected trailing whitespace at the end of a line. Please remove any trailing spaces or tabs from blank lines and the end of code lines throughout this file.
Based on pipeline failure logs.
🧹 Nitpick comments (1)
includes/Traits/VendorAuthorizable.php (1)
68-88: Correct vendor ID resolution for vendors and staff.The method properly distinguishes between vendor users (returning their own ID) and staff users (returning parent vendor ID from meta). The fallback to current user (line 69-71) and handling of missing staff vendor ID (line 80-82) are both appropriate.
Optional: Extract staff vendor ID lookup to reduce duplication
The staff vendor ID retrieval logic appears in both
check_vendor_authorizable_permission(line 51) andget_user_vendor_id(line 78). While not critical, you could extract this to a small private helper method if you expect to add more staff-related logic in the future:/** * Get the parent vendor ID for a staff user. * * @param int $staff_user_id Staff user ID. * @return int|false Vendor ID or false if not found. */ private function get_staff_vendor_id( $staff_user_id ) { return (int) get_user_meta( $staff_user_id, '_vendor_id', true ) ?: false; }Then use it in both methods. This is a minor maintainability improvement and can be deferred.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Traits/VendorAuthorizable.php
🧰 Additional context used
🧬 Code graph analysis (1)
includes/Traits/VendorAuthorizable.php (1)
includes/functions.php (1)
dokan_is_user_seller(73-79)
🪛 GitHub Actions: Inspections
includes/Traits/VendorAuthorizable.php
[error] 1-1: PHPCS: Whitespace found at end of line. (Command: vendor/bin/phpcs includes/REST/StoreController.php includes/Traits/VendorAuthorizable.php -q --report=checkstyle | cs2pr --graceful-warnings)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
🔇 Additional comments (1)
includes/Traits/VendorAuthorizable.php (1)
29-57: Well-implemented authorization logic with proper security checks.The cascading permission checks (admin → vendor → staff) correctly handle all user types. The use of
dokan_is_user_seller($current_user_id, true)properly excludes staff at Line 46, preventing staff from being treated as vendors. The vendor existence validation (lines 34-38) and strict integer comparisons add robustness.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
includes/REST/StoreController.php (1)
367-374: Critical: Authorization bypass allows vendors to update other vendors' stores.The permission check validates the current user's own vendor store ID, not the requested store ID from the request. This creates an authorization bypass:
Attack scenario:
- Vendor A (ID 100) sends
PUT /dokan/v1/stores/200to update Vendor B's storeget_store_id_for_user()returns 100 (Vendor A's own ID)check_vendor_authorizable_permission(100)passes (100 === 100)- Permission granted ✓
- In
update_store(), since user is a vendor (not staff),$requested_idstays 200- Store 200 (Vendor B's store) gets updated ❌
🔎 Proposed fix
public function update_store_permissions_check( $request ) { // Admin can update any store if ( current_user_can( 'manage_woocommerce' ) ) { return true; } - return $this->check_vendor_authorizable_permission( $this->get_store_id_for_user() ); + $requested_id = absint( $request->get_param( 'id' ) ); + $current_user = get_current_user_id(); + + // For staff, resolve to their parent vendor ID + if ( user_can( $current_user, 'vendor_staff' ) ) { + $staff_vendor_id = (int) get_user_meta( $current_user, '_vendor_id', true ); + if ( $staff_vendor_id ) { + // Staff can only update their assigned vendor's store + return $requested_id === $staff_vendor_id; + } + return false; + } + + // Vendors can only update their own store + return $this->check_vendor_authorizable_permission( $requested_id ); }
🧹 Nitpick comments (1)
includes/REST/StoreController.php (1)
386-405: Consider using the trait helper to reduce code duplication.The staff-to-vendor resolution logic (lines 389-395) duplicates what
get_store_id_for_user()already provides. This was also noted in previous review comments.🔎 Proposed refactor
public function update_store( $request ) { - $requested_id = absint( $request->get_param( 'id' ) ); - $current_user = get_current_user_id(); - - if ( user_can( $current_user, 'vendor_staff' ) ) { - $staff_vendor_id = (int) get_user_meta( $current_user, '_vendor_id', true ); - - if ( $staff_vendor_id ) { - $requested_id = $staff_vendor_id; - } - } + // For staff users, resolve to their parent vendor ID + $requested_id = $this->get_store_id_for_user(); + + if ( ! $requested_id ) { + return new WP_Error( + 'dokan_rest_invalid_vendor', + __( 'Invalid vendor context.', 'dokan-lite' ), + [ 'status' => 400 ] + ); + } $store = dokan()->vendor->get( $requested_id );Note: This refactor assumes the authorization bypass in
update_store_permissions_checkis also fixed to properly validate the requested store ID.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/REST/StoreController.phpincludes/Traits/VendorAuthorizable.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T06:19:41.376Z
Learnt from: mrabbani
Repo: getdokan/dokan PR: 2891
File: src/admin/dashboard/pages/vendors-single/InformationTabs/WithdrawTab.tsx:58-59
Timestamp: 2025-09-19T06:19:41.376Z
Learning: The Dokan codebase uses 'dokan-lite' as the text domain for WordPress internationalization (__() function calls).
Applied to files:
includes/REST/StoreController.php
🧬 Code graph analysis (2)
includes/Traits/VendorAuthorizable.php (1)
includes/functions.php (2)
dokan_get_current_user_id(48-61)dokan_is_user_seller(73-79)
includes/REST/StoreController.php (3)
includes/Traits/VendorAuthorizable.php (2)
get_store_id_for_user(64-80)check_vendor_authorizable_permission(29-51)includes/Vendor/Manager.php (1)
get(131-133)includes/Vendor/Vendor.php (1)
get_id(312-314)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (1, 3)
🔇 Additional comments (3)
includes/Traits/VendorAuthorizable.php (2)
29-51: LGTM!The authorization logic correctly handles all user scenarios: admins get full access, vendors can only access their own store, and staff users are properly resolved to their parent vendor via
dokan_get_current_user_id().
64-80: LGTM!The vendor resolution logic works correctly. When no
$user_idis provided,dokan_get_current_user_id()already handles staff-to-vendor resolution, and the subsequent checks correctly identify sellers. The explicit staff handling at lines 73-77 serves as a fallback when a specific staff user ID is passed in.includes/REST/StoreController.php (1)
5-5: LGTM!Trait import and usage are correctly structured.
Also applies to: 22-22
mrabbani
left a comment
mrabbani
left a comment
There was a problem hiding this comment.
Code Review Summary
Overall, this PR addresses an important issue where vendor staff cannot access social profile links. The approach of using a trait for reusable permission logic is good, and the code structure is clean. However, there are critical bugs in the implementation that prevent vendor staff from actually accessing stores, despite the documentation stating they should be able to.
✅ Positive Aspects
- Good use of traits - The
VendorAuthorizabletrait provides reusable permission logic that can be used across controllers - Clear documentation - Method docblocks clearly explain the intended behavior
- Type hints - Proper use of PHP type hints improves code safety
- Security pattern - The permission checks follow a good pattern (admin → vendor → staff)
🔴 Critical Bugs (Must Fix)
1. Missing Vendor Staff Check in check_vendor_authorizable_permission()
Location: includes/Traits/VendorAuthorizable.php:29-51
Issue: The function documentation states "Vendor staff can access only their assigned vendor store," but the implementation doesn't actually check for vendor staff. It only checks if the current user is a seller and if their ID matches the vendor ID, which will always fail for vendor staff.
Current code:
if ( dokan_is_user_seller( $current_user_id ) ) {
return (int) $current_user_id === (int) $vendor_id;
}
return false;Problem: dokan_is_user_seller($current_user_id) returns true for vendor staff (since they have the dokandar capability), but the subsequent check $current_user_id === $vendor_id will always fail for staff members since their user ID is different from the vendor ID.
Fix required:
$current_user_id = dokan_get_current_user_id();
// Check if current user is the vendor (exclude staff)
if ( dokan_is_user_seller( $current_user_id, true ) ) { // exclude_staff = true
return (int) $current_user_id === (int) $vendor_id;
}
// Check if current user is vendor staff with access to this vendor
if ( user_can( $current_user_id, 'vendor_staff' ) ) {
$staff_vendor_id = (int) get_user_meta( $current_user_id, '_vendor_id', true );
return $staff_vendor_id > 0 && (int) $staff_vendor_id === (int) $vendor_id;
}
return false;2. Missing Requested Store ID Check in update_store_permissions_check()
Location: includes/REST/StoreController.php:367-374
Issue: The permission check doesn't verify that the requested store ID matches the current user's vendor ID or their staff's vendor ID.
Current code:
return $this->check_vendor_authorizable_permission( $this->get_store_id_for_user() );Problem: This only checks if the current user has permission to access their own store, but doesn't verify that the requested store ID (from $request->get_param('id')) matches.
Fix required:
$requested_id = absint( $request->get_param( 'id' ) );
$user_store_id = $this->get_store_id_for_user();
// For vendor staff, we need to resolve the requested ID to their vendor ID
if ( user_can( get_current_user_id(), 'vendor_staff' ) && $requested_id === get_current_user_id() ) {
$requested_id = $user_store_id;
}
return $this->check_vendor_authorizable_permission( $requested_id );3. Potential Logic Issue in get_store() Method
Location: includes/REST/StoreController.php:316
Issue: get_store_id_for_user() is called with the requested store ID, but this method is designed to convert a user ID to their vendor ID. If the requested ID is already a vendor ID, this could cause issues.
Current code:
$store_id = $this->get_store_id_for_user( (int) $request->get_param( 'id' ) );Concern: If the API accepts vendor IDs (store IDs), calling get_store_id_for_user() with a vendor ID might incorrectly resolve it if that vendor ID happens to be a user ID that's vendor staff. However, since dokan_is_user_seller($user_id, true) excludes staff, if the ID is a vendor, it should return correctly. This might be intentional to allow the API to accept either vendor IDs or user IDs, but the logic could be clearer.
Recommendation: Add a comment explaining that the API accepts either a vendor ID or user ID, and if it's a user ID for staff, it resolves to their vendor ID.
⚠️ Code Quality Issues
4. Code Duplication in update_store() Method
Location: includes/REST/StoreController.php:389-395
Issue: Manual vendor staff handling duplicates logic that should use get_store_id_for_user().
Current code:
if ( user_can( $current_user, 'vendor_staff' ) ) {
$staff_vendor_id = (int) get_user_meta( $current_user, '_vendor_id', true );
if ( $staff_vendor_id ) {
$requested_id = $staff_vendor_id;
}
}Recommendation: Use the trait method instead:
// If current user is staff and requesting their own user ID, resolve to vendor ID
if ( user_can( $current_user, 'vendor_staff' ) && $requested_id === $current_user ) {
$requested_id = $this->get_store_id_for_user( $current_user );
}🧪 Testing Recommendations
- Test vendor staff can access their vendor's store via GET endpoint
- Test vendor staff can update their vendor's store via PUT/PATCH endpoint
- Test vendor staff cannot access other vendors' stores
- Test regular vendors cannot access other vendors' stores
- Test admins can access any store
- Test edge cases: staff without
_vendor_idmeta, invalid vendor IDs, etc.
Summary
The approach is correct, but the implementation has critical bugs that prevent vendor staff from accessing stores. The main issue is that check_vendor_authorizable_permission() doesn't check for vendor staff, even though the documentation says it should. Once these bugs are fixed, the implementation should work as intended.
Priority fixes:
- ✅ Fix Bug #1 - Add vendor staff check in
check_vendor_authorizable_permission() - ✅ Fix Bug #2 - Add requested store ID verification in
update_store_permissions_check() ⚠️ Refactor Bug #4 - Remove code duplication inupdate_store()
Once these are addressed, this PR should be ready to merge. Great work on identifying the issue and creating a reusable solution!
Method Name SuggestionsAfter reviewing the code, I'd like to suggest better method names that are more readable and follow common PHP naming conventions: 1.
|
- Fix vendor staff authorization in store access checks - Ensure requested store ID is validated in update permission checks - Align permission logic with documented behavior - Refactor staff vendor resolution to use shared trait methods - Improve method naming for clarity and readability - Add comments to clarify store/user ID resolution logic
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
includes/REST/StoreController.php (1)
315-330: Critical: Permission check breaks public store access.The
GET /dokan/v1/stores/{id}endpoint is registered withpermission_callback => '__return_true'(line 73), indicating it's a public endpoint. However,can_access_vendor_store()denies access for:
- Unauthenticated users
- Customers
- Any user not associated with the requested store
This breaks the public store listing functionality that clients depend on for displaying vendor storefronts.
The PR objective is to fix social profile visibility for vendor staff in the dashboard settings—not to restrict public store viewing. Consider one of these approaches:
- Restrict permission only for sensitive data: Apply authorization when the request includes dashboard/settings-specific parameters
- Create a separate secured endpoint for dashboard settings (e.g.,
/stores/me/settings)- Remove the permission check from get_store and apply it only in
update_storeflows
🧹 Nitpick comments (2)
includes/Traits/VendorAuthorizable.php (1)
64-67: Subtle fallback behavior may cause confusion.When
$user_idis empty/0,dokan_get_current_user_id()already resolves staff users to their parent vendor ID. This means the vendor_staff check (lines 73-77) is never reached in that case—the method returns correctly via the seller check instead.While functionally correct, this inconsistent code path could confuse future maintainers. Consider using
get_current_user_id()(WordPress native) as the fallback to keep the resolution logic self-contained:🔎 Proposed refactor
public function get_vendor_id_for_user( int $user_id = 0 ): int { if ( empty( $user_id ) ) { - $user_id = dokan_get_current_user_id(); + $user_id = get_current_user_id(); }includes/REST/StoreController.php (1)
391-407: Consider simplifying with direct trait method usage.The staff ID remapping logic (lines 395-397) duplicates what's in
update_store_permissions_check. Sinceget_vendor_id_for_user()already handles staff→vendor resolution when passed a staff ID, you could simplify:🔎 Proposed refactor
public function update_store( $request ) { - $requested_id = absint( $request->get_param( 'id' ) ); - $current_user = get_current_user_id(); - - if ( user_can( $current_user, 'vendor_staff' ) && $requested_id === $current_user ) { - $requested_id = $this->get_vendor_id_for_user( $current_user ); - } + $requested_id = absint( $request->get_param( 'id' ) ); + + // Resolve staff user ID to parent vendor ID if applicable + if ( ! dokan_is_user_seller( $requested_id, true ) ) { + $requested_id = $this->get_vendor_id_for_user(); + } $store = dokan()->vendor->get( $requested_id );This leverages the trait method more directly and aligns with the reviewer's earlier suggestion to use
dokan_is_user_seller()for validation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/REST/StoreController.phpincludes/Traits/VendorAuthorizable.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T06:19:41.376Z
Learnt from: mrabbani
Repo: getdokan/dokan PR: 2891
File: src/admin/dashboard/pages/vendors-single/InformationTabs/WithdrawTab.tsx:58-59
Timestamp: 2025-09-19T06:19:41.376Z
Learning: The Dokan codebase uses 'dokan-lite' as the text domain for WordPress internationalization (__() function calls).
Applied to files:
includes/REST/StoreController.php
🧬 Code graph analysis (2)
includes/Traits/VendorAuthorizable.php (1)
includes/functions.php (2)
dokan_get_current_user_id(48-61)dokan_is_user_seller(73-79)
includes/REST/StoreController.php (1)
includes/Traits/VendorAuthorizable.php (2)
get_vendor_id_for_user(64-80)can_access_vendor_store(29-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (2, 3)
🔇 Additional comments (3)
includes/Traits/VendorAuthorizable.php (1)
29-51: LGTM!The authorization logic is sound. Using
dokan_get_current_user_id()correctly resolves staff users to their parent vendor ID, allowing staff to access their assigned vendor's store while preventing access to other stores.includes/REST/StoreController.php (2)
5-5: LGTM!Trait import and usage follow PHP conventions.
Also applies to: 22-22
367-380: LGTM for update permission check.The staff ID remapping logic correctly handles the case where a staff user's own ID is passed as the store ID, redirecting it to their parent vendor's store for authorization.
…ndorAuthorizable trait - Add comprehensive test suite for StoreController authorization (20 tests) - Refactor validate_store_id to use VendorAuthorizable trait method - Update can_access_vendor_store to support vendor staff authorization - Centralize vendor ID resolution logic using VendorUtil - All tests passing (20 tests, 44 assertions)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
includes/REST/StoreController.php (1)
372-379: Code duplication: Staff ID resolution logic is repeated.The logic to check
user_can( get_current_user_id(), 'vendor_staff' ) && $requested_id === get_current_user_id()and remap to vendor ID is duplicated in bothupdate_store_permissions_check(lines 375-377) andupdate_store(lines 395-397). This creates a maintenance burden.Consider extracting to a helper method in the trait:
🔎 Suggested refactor
+ /** + * Resolve the effective store ID for the current request. + * For staff users accessing via their own ID, returns their parent vendor ID. + * + * @param int $requested_id The requested store ID from the request. + * @return int The effective store ID to use. + */ + protected function resolve_effective_store_id( int $requested_id ): int { + $current_user = get_current_user_id(); + + if ( user_can( $current_user, 'vendor_staff' ) && $requested_id === $current_user ) { + return $this->get_vendor_id_for_user( $current_user ); + } + + return $requested_id; + }Then use it in both methods:
$effective_id = $this->resolve_effective_store_id( $requested_id );
🧹 Nitpick comments (1)
tests/php/src/REST/StoreControllerAuthorizationTest.php (1)
335-348: Clarify unauthenticated access test expectation.The test accepts both
200and403as valid responses, which makes it non-deterministic. Based on the implementation inget_store, unauthenticated users should consistently receive403sincecan_access_vendor_storereturnsfalsewhenget_current_user_id()returns0.🔎 Proposed fix
public function test_unauthenticated_user_cannot_access_vendor_store(): void { wp_set_current_user( 0 ); // Try to access seller_id1's store $response = $this->get_request( "stores/{$this->seller_id1}" ); - // Note: get_store has permission_callback '__return_true', so unauthenticated users can view stores - // This test verifies the authorization logic within get_store itself - // However, if the store doesn't exist or authorization fails, it returns 403 - $this->assertContains( - $response->get_status(), - [ 200, 403 ], - 'Unauthenticated user access depends on store visibility settings' - ); + // get_store has permission_callback '__return_true', but can_access_vendor_store + // returns false for unauthenticated users (get_current_user_id() = 0) + $this->assertEquals( 403, $response->get_status(), 'Unauthenticated user should not be able to access vendor store' ); + + $data = $response->get_data(); + $this->assertEquals( 'rest_forbidden', $data['code'], 'Error code should be rest_forbidden' ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
includes/REST/StoreController.phpincludes/Traits/VendorAuthorizable.phptests/php/src/REST/StoreControllerAuthorizationTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T06:19:41.376Z
Learnt from: mrabbani
Repo: getdokan/dokan PR: 2891
File: src/admin/dashboard/pages/vendors-single/InformationTabs/WithdrawTab.tsx:58-59
Timestamp: 2025-09-19T06:19:41.376Z
Learning: The Dokan codebase uses 'dokan-lite' as the text domain for WordPress internationalization (__() function calls).
Applied to files:
includes/REST/StoreController.php
🧬 Code graph analysis (2)
includes/Traits/VendorAuthorizable.php (2)
includes/Utilities/VendorUtil.php (1)
VendorUtil(4-89)includes/functions.php (1)
dokan_is_user_seller(73-79)
includes/REST/StoreController.php (3)
includes/Traits/VendorAuthorizable.php (2)
get_vendor_id_for_user(65-67)can_access_vendor_store(32-52)includes/Vendor/Manager.php (1)
get(131-133)includes/Vendor/Vendor.php (1)
get_id(312-314)
🪛 PHPMD (2.15.0)
includes/Traits/VendorAuthorizable.php
84-84: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (12)
includes/REST/StoreController.php (4)
5-5: LGTM! Trait import and usage correctly added.The
VendorAuthorizabletrait is properly imported and used, enabling the new authorization methods.Also applies to: 22-22
67-67: LGTM! Validation callback updated to use trait method.Using
[ $this, 'validate_store_id' ]properly delegates to the trait's validation logic.
315-330: Breaking change: Public GET endpoint now requires authorization.The
get_storeendpoint is registered withpermission_callback => '__return_true'(line 73), declaring it as public. However, the implementation now callscan_access_vendor_store()which blocks unauthenticated users and customers. This is a breaking change for any clients relying on public store data access.Based on PR objectives, this seems intentional for the vendor dashboard/settings context, but it affects all consumers of this endpoint. Consider:
- Create a separate secured endpoint for dashboard settings (e.g.,
/stores/me/settings)- Only apply permission checks conditionally when accessing sensitive data fields
If this breaking change is intentional, please document it in the changelog and migration notes.
391-407: Good addition of 404 handling for non-existent stores.The check for
! $store || ! $store->get_id()with proper 404 error response is a good defensive addition. The text domain'dokan-lite'is correct.includes/Traits/VendorAuthorizable.php (3)
5-6: LGTM! VendorUtil dependency properly imported.
32-52: LGTM! Authorization logic is well-structured.The
can_access_vendor_storemethod correctly:
- Returns
falsefor invalid$vendor_id- Allows admins (
manage_woocommerce) to access any store- Resolves the user to their vendor ID (handles staff → parent vendor)
- Compares resolved vendor ID with requested vendor ID
The type hints and docblock are clear.
65-67: LGTM! Thin wrapper delegates to centralized utility.Delegating to
VendorUtil::get_vendor_id_for_user()keeps the logic centralized and avoids duplication.tests/php/src/REST/StoreControllerAuthorizationTest.php (5)
1-60: LGTM! Well-structured test class with proper setup.Good practices:
- Clear docblocks with
@groupand@coversannotations- Reusable
create_vendor_staffhelper method- Proper initialization of controller and staff users in
setUp
86-164: LGTM! Comprehensive admin and vendor self-access tests.Good coverage of:
- Admin updating/accessing any vendor store
- Vendor updating/accessing their own store
- Proper assertion of response status and data
207-258: LGTM! Vendor staff access and ID resolution tests are thorough.Excellent coverage of the key staff use cases:
- Staff accessing/updating by vendor ID
- Staff accessing/updating by their own staff ID (with resolution)
- Verification that resolved store ID matches parent vendor
438-457: LGTM! Good coverage of non-existent store handling.The test correctly verifies:
- GET returns 400 with
rest_invalid_param(validation callback failure)- PUT returns 404 with
dokan_rest_store_not_found(method-level check)This aligns with the implementation where validation runs first for GET, but PUT has additional store existence check in
update_store.
479-496: LGTM! Edge case for orphan staff is well tested.Testing staff without
_vendor_idmeta is important for robustness. Good cleanup withwp_delete_user.
| public function validate_store_id( $value, $request, $key ) { | ||
| $vendor_id = $this->get_vendor_id_for_user( $value ); | ||
|
|
||
| // Validate that the vendor ID is a valid store/vendor. | ||
| // $vendor_id is fetched via get_vendor_id_for_user: for vendors, it's their own ID; for vendor staff, it's their parent vendor's ID. | ||
| // If both $value and $vendor_id are > 0, the ID is considered valid and belongs to a store/vendor. | ||
| // Otherwise, return a WP_Error indicating the store was not found. | ||
| if ( $value > 0 && $vendor_id > 0 ) { | ||
| return true; | ||
| } | ||
|
|
||
| // translators: 1) rest api endpoint key name | ||
| return new \WP_Error( 'rest_invalid_param', sprintf( esc_html__( 'No store found with given store id', 'dokan-lite' ), $key ), [ 'status' => 400 ] ); | ||
| } |
There was a problem hiding this comment.
Fix unused sprintf argument and suppress PHPMD false positive.
Two issues:
- The
$keyparameter is passed tosprintfbut the format string has no placeholder (%s). Remove the unused argument. - The
$requestparameter is unused but required by WordPress REST API'svalidate_callbacksignature. Add a suppression annotation.
🔎 Proposed fix
+ /**
+ * @SuppressWarnings(PHPMD.UnusedFormalParameter)
+ */
public function validate_store_id( $value, $request, $key ) {
$vendor_id = $this->get_vendor_id_for_user( $value );
if ( $value > 0 && $vendor_id > 0 ) {
return true;
}
- // translators: 1) rest api endpoint key name
- return new \WP_Error( 'rest_invalid_param', sprintf( esc_html__( 'No store found with given store id', 'dokan-lite' ), $key ), [ 'status' => 400 ] );
+ return new \WP_Error( 'rest_invalid_param', esc_html__( 'No store found with given store id', 'dokan-lite' ), [ 'status' => 400 ] );
}🧰 Tools
🪛 PHPMD (2.15.0)
84-84: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In includes/Traits/VendorAuthorizable.php around lines 84-97, the
validate_store_id method passes an unused $key to sprintf and triggers a PHPMD
false positive for the unused $request parameter; remove the unused $key
argument from the sprintf call (or drop sprintf and use the translated string
directly) so the format string matches its arguments, and add a PHPMD
suppression docblock to the method (e.g.
@SuppressWarnings(PHPMD.UnusedFormalParameter)) to acknowledge the $request
parameter is intentionally unused while preserving the required REST API
signature.
- Fix can_access_vendor_store to allow admins to access invalid vendor IDs for proper 404 error handling - Simplify update_store_permissions_check to use get_vendor_id_for_user consistently - Improve update_store to properly resolve vendor IDs for vendor staff - Enhance docblocks with clearer descriptions and role-based behavior explanations - Ensure public endpoints use exact vendor IDs without resolution
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
includes/Traits/VendorAuthorizable.php (1)
93-106: Remove unused sprintf argument.Line 105 passes
$keytosprintfbut the format string contains no placeholder (%s). Since the error message doesn't need the parameter name, either remove thesprintfcall or add the placeholder if you intend to include the key name in the message.🔎 Proposed fix
- return new \WP_Error( 'rest_invalid_param', sprintf( esc_html__( 'No store found with given store id', 'dokan-lite' ), $key ), [ 'status' => 400 ] ); + return new \WP_Error( 'rest_invalid_param', esc_html__( 'No store found with given store id', 'dokan-lite' ), [ 'status' => 400 ] );Note: The
$requestparameter is intentionally unused but required by the WordPress REST APIvalidate_callbacksignature.
🧹 Nitpick comments (1)
includes/REST/StoreController.php (1)
385-423: Vendor ID resolution and error handling are solid.The method correctly resolves vendor IDs for staff users and includes comprehensive error handling for both resolution failures and missing stores.
One minor observation: lines 391-397 and 401-407 both return 404 errors for different conditions (failed resolution vs. missing store). While both checks serve distinct purposes, you might consider consolidating them or adding differentiating error codes for better debugging.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/REST/StoreController.phpincludes/Traits/VendorAuthorizable.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T06:19:41.376Z
Learnt from: mrabbani
Repo: getdokan/dokan PR: 2891
File: src/admin/dashboard/pages/vendors-single/InformationTabs/WithdrawTab.tsx:58-59
Timestamp: 2025-09-19T06:19:41.376Z
Learning: The Dokan codebase uses 'dokan-lite' as the text domain for WordPress internationalization (__() function calls).
Applied to files:
includes/REST/StoreController.php
🧬 Code graph analysis (1)
includes/REST/StoreController.php (1)
includes/Traits/VendorAuthorizable.php (2)
get_vendor_id_for_user(72-74)can_access_vendor_store(34-56)
🪛 PHPMD (2.15.0)
includes/Traits/VendorAuthorizable.php
93-93: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (6)
includes/Traits/VendorAuthorizable.php (3)
5-5: LGTM!The import is correctly added to support the vendor ID resolution functionality.
20-56: LGTM!The authorization logic is well-structured with clear role-based rules. The docblock effectively documents the behavior for admins, vendors, and staff.
58-74: LGTM!Clean delegation to the utility class with a comprehensive docblock explaining the resolution logic for different user types.
includes/REST/StoreController.php (3)
5-5: LGTM!The trait import and usage are correctly added to enable vendor authorization capabilities in the controller.
Also applies to: 22-22
67-67: LGTM!The validation callback correctly references the trait method, following WordPress REST API conventions.
367-373: LGTM!The permission check correctly resolves vendor IDs for both vendors and staff, then validates access appropriately for update operations.
- Make get_store endpoint accessible to all users/guests - Filter sensitive data (payment, enabled status, admin commission) for unauthorized users - Respect admin settings for hiding vendor info (email, phone, address) - Show public data to everyone while protecting sensitive information - Update authorization check to determine data visibility, not access blocking - Update all test cases to reflect new public endpoint behavior - Tests now verify filtered data instead of expecting 403 errors
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
includes/REST/StoreController.php (1)
309-314: Clarify documentation - get_store does NOT resolve vendor IDs.The documentation states "For vendor staff accessing via their own ID, the vendor ID is resolved to show their vendor's store" (line 312-313), but the implementation at lines 322-324 uses
requested_iddirectly without any vendor ID resolution. This is actually the correct behavior for a public read endpoint—users should get data for the exact store ID they request.The vendor ID resolution only happens in write operations (
update_store, line 397). Please update the documentation to accurately reflect this behavior.🔎 Suggested documentation fix
/** * Get singe store * - * Public endpoint: Returns public data for all users/guests (respecting admin settings). - * Sensitive data is only returned for authorized users (vendor, vendor staff, or admin). - * - * For vendor staff accessing via their own ID, the vendor ID is resolved to show their vendor's store. - * Vendors and vendor staff attempting to access another vendor's store will be blocked (403). + * Public endpoint: Returns store data for the requested store ID. + * - Public data is returned for all users/guests (respecting admin settings). + * - Sensitive data (payment, enabled status, admin_commission, etc.) is only returned for authorized users (vendor, vendor staff, or admin). + * - No vendor ID resolution occurs in GET requests; the requested ID is used directly. * * @since 1.0.0tests/php/src/REST/StoreControllerAuthorizationTest.php (1)
348-361: Clarify test expectation for unauthenticated user access.Lines 356-360 allow either 200 or 403 for unauthenticated users accessing a store, with the comment "depends on store visibility settings." However, based on the implementation:
- The
get_storeendpoint haspermission_callback => '__return_true'(line 73 in StoreController)- The endpoint returns public data with filtered sensitive fields for unauthorized users
The test should consistently expect 200 status (assuming the store exists), as the endpoint is public. The 403 response would not occur unless there's additional visibility logic not shown in the reviewed code.
Consider simplifying this test to expect 200 and verify filtered data, similar to the customer test at lines 307-322.
🔎 Suggested test fix
public function test_unauthenticated_user_cannot_access_vendor_store(): void { wp_set_current_user( 0 ); - // Try to access seller_id1's store + // Try to access seller_id1's store (public endpoint, so access is allowed) $response = $this->get_request( "stores/{$this->seller_id1}" ); - // Note: get_store has permission_callback '__return_true', so unauthenticated users can view stores - // This test verifies the authorization logic within get_store itself - // However, if the store doesn't exist or authorization fails, it returns 403 - $this->assertContains( - $response->get_status(), - [ 200, 403 ], - 'Unauthenticated user access depends on store visibility settings' - ); + $this->assertEquals( 200, $response->get_status(), 'Unauthenticated user can access store (public endpoint)' ); + + $data = $response->get_data(); + // Verify sensitive data is filtered + $this->assertArrayNotHasKey( 'payment', $data, 'Payment data should be hidden from unauthorized users' ); + $this->assertArrayNotHasKey( 'enabled', $data, 'Enabled status should be hidden from unauthorized users' ); + $this->assertArrayNotHasKey( 'admin_commission', $data, 'Admin commission should be hidden from unauthorized users' ); + // Verify public data is available + $this->assertArrayHasKey( 'store_name', $data, 'Store name should be available (public data)' ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
includes/REST/StoreController.phptests/php/src/REST/StoreControllerAuthorizationTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T06:19:41.376Z
Learnt from: mrabbani
Repo: getdokan/dokan PR: 2891
File: src/admin/dashboard/pages/vendors-single/InformationTabs/WithdrawTab.tsx:58-59
Timestamp: 2025-09-19T06:19:41.376Z
Learning: The Dokan codebase uses 'dokan-lite' as the text domain for WordPress internationalization (__() function calls).
Applied to files:
includes/REST/StoreController.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (10)
includes/REST/StoreController.php (6)
5-5: LGTM!The VendorAuthorizable trait is properly imported and used to provide authorization helpers.
Also applies to: 22-22
321-338: LGTM!The implementation correctly handles the public GET endpoint:
- Uses requested ID directly (appropriate for public access)
- Properly handles 404 for non-existent stores
- Data filtering is delegated to
prepare_item_for_response
375-382: LGTM!The permission check correctly resolves vendor IDs for staff users and delegates authorization to the trait method. This ensures staff members can update their associated vendor's store.
393-432: LGTM!The update implementation correctly:
- Resolves staff user IDs to their parent vendor IDs (line 397)
- Returns 404 for non-existent stores (lines 399-405, 409-415)
- Handles update errors properly (lines 420-422)
- Refreshes store data after update (line 424)
This ensures vendor staff can update their vendor's store settings as intended.
672-713: LGTM!Excellent authorization-aware data filtering:
- Public data (store_name, id, etc.) is available to all users
- Sensitive data (payment, enabled, admin_commission) is filtered for unauthorized users
- Respects admin settings for hiding vendor info (address, phone, email)
- The
is_authorizedflag is passed to filters for extensibilityThis implementation properly supports the public endpoint with conditional data exposure.
67-67: The validation callback is properly implemented.The
validate_store_idmethod exists in the VendorAuthorizable trait and is correctly implemented to handle vendor-aware validation. The StoreController class uses this trait, so the callback reference is valid.tests/php/src/REST/StoreControllerAuthorizationTest.php (4)
1-79: LGTM!Excellent test setup:
- Comprehensive PHPDoc annotations with
@coverstags- Typed properties for better type safety
- Clean helper method for creating vendor staff
- Proper test initialization in setUp
86-206: LGTM!Comprehensive test coverage for admin and vendor access patterns:
- Admin can access/update any vendor store ✓
- Vendor can access/update their own store ✓
- Public endpoint allows cross-vendor reads with filtered sensitive data ✓
- Cross-vendor updates are properly blocked (403) ✓
The assertions validate both HTTP status codes and data structure, ensuring the authorization logic works as intended.
212-300: LGTM!Thorough vendor staff authorization tests:
- Staff can access their vendor's store with full sensitive data ✓
- Staff can update using both vendor ID and their own staff ID (with resolution) ✓
- Public cross-vendor reads work with filtered data ✓
- Cross-vendor updates are blocked (403) ✓
The test at lines 248-259 specifically validates that staff ID resolution works correctly in update operations.
387-528: LGTM!Excellent coverage of edge cases and vendor ID resolution behavior:
- Cross-vendor staff access with data filtering ✓
- Lines 415-428 confirm
get_storedoes NOT resolve vendor IDs (public endpoint behavior)- Lines 435-455 confirm
update_storeDOES resolve vendor IDs (write operation behavior)- Non-existent stores return appropriate error codes (400 for GET validation, 404 for PUT) ✓
- Invalid vendor IDs are caught by validation ✓
- Orphan staff (without vendor_id meta) are handled correctly ✓
These tests provide strong validation that the authorization and vendor ID resolution logic works correctly across different scenarios.
…ngController - Add field restriction logic for vendor staff (email, password) - Add field restriction logic for vendors (admin commission fields) - Refactor StoreSettingController to extend StoreController for code reuse - Extract field restriction logic into reusable methods: - get_restricted_fields_for_update() - for store updates - get_restricted_fields_for_view() - for store data views - Add comprehensive test suite for StoreSettingController (20 tests) - Add is_staff_only() method to VendorAuthorizable trait - Improve code documentation with PHPDoc blocks - All tests passing (20 tests, 49 assertions)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
includes/Traits/VendorAuthorizable.php (1)
99-112: Unusedsprintfargument:$keyis not used in the format string.The error message uses
sprintfwith$keyas an argument, but the format string has no placeholder. Either add%sto the message or remove the unused argument.🔎 Proposed fix
- // translators: 1) rest api endpoint key name - return new \WP_Error( 'rest_invalid_param', sprintf( esc_html__( 'No store found with given store id', 'dokan-lite' ), $key ), [ 'status' => 400 ] ); + return new \WP_Error( 'rest_invalid_param', esc_html__( 'No store found with given store id', 'dokan-lite' ), [ 'status' => 400 ] );Or if you want to include the key name:
- // translators: 1) rest api endpoint key name - return new \WP_Error( 'rest_invalid_param', sprintf( esc_html__( 'No store found with given store id', 'dokan-lite' ), $key ), [ 'status' => 400 ] ); + // translators: %s: REST API parameter key name + return new \WP_Error( 'rest_invalid_param', sprintf( esc_html__( 'No store found with given %s', 'dokan-lite' ), $key ), [ 'status' => 400 ] );Note: The
$requestparameter is unused but required by WordPress REST API'svalidate_callbacksignature, so that's not an issue.
🧹 Nitpick comments (6)
includes/REST/StoreController.php (4)
416-422: Double permission check may be redundant.The
update_storemethod performscan_access_vendor_store()check at line 416, but this check was already performed inupdate_store_permissions_check()(the permission callback). This creates redundancy since WordPress REST API only calls the endpoint if permission callback returns true.However, this could be intentional defense-in-depth if the method might be called directly from other code paths.
428-432: Indentation inconsistency: tabs vs spaces.Lines 428-432 use tabs for indentation while surrounding code uses consistent spacing. This may cause PHPCS issues.
🔎 Proposed fix
- foreach ( $restricted_fields as $field ) { - if ( isset( $params[ $field ] ) ) { - unset( $params[ $field ] ); - } - } + foreach ( $restricted_fields as $field ) { + if ( isset( $params[ $field ] ) ) { + unset( $params[ $field ] ); + } + }
766-801:can_access_vendor_storeis called twice in the same response flow.
get_restricted_fields_for_viewcallscan_access_vendor_store()at line 769, butprepare_item_for_responsealready called it at line 728. This results in duplicate authorization checks for every response preparation.Consider passing
$is_authorizedas a parameter or caching the result.🔎 Proposed refactor
- protected function get_restricted_fields_for_view( $store, $request ) { + protected function get_restricted_fields_for_view( $store, $request, $is_authorized = null ) { $restricted_fields = []; - $is_authorized = $this->can_access_vendor_store( $store->get_id() ); + if ( $is_authorized === null ) { + $is_authorized = $this->can_access_vendor_store( $store->get_id() ); + }And in
prepare_item_for_response:- $restricted_fields = $this->get_restricted_fields_for_view( $store, $request ); + $restricted_fields = $this->get_restricted_fields_for_view( $store, $request, $is_authorized );
771-777: Redundant condition:$is_authorized && $this->is_staff_only(...).The condition
! $is_authorized || ( $is_authorized && $this->is_staff_only( get_current_user_id() ) )can be simplified. The second part only matters when$is_authorizedis true.🔎 Proposed simplification
- if ( ! $is_authorized || ( $is_authorized && $this->is_staff_only( get_current_user_id() ) ) ) { + if ( ! $is_authorized || $this->is_staff_only( get_current_user_id() ) ) {This is logically equivalent since if
$is_authorizedis false, the short-circuit evaluation prevents theis_staff_onlycall.tests/php/src/REST/StoreSettingControllerTest.php (2)
504-525: Test documents incomplete behavior: vendor_id fallback.This test acknowledges that the controller may not handle missing
vendor_idproperly with the conditional check at line 519. This aligns with the edge case identified inStoreSettingController.phpwherevendor_id = 0causes issues.Consider adding an explicit assertion or marking this as an expected failure scenario until the fallback is implemented.
536-556: Suppress unused parameter warnings in test closure.The static analysis correctly identifies that
$additional_fields,$store, and$requestare unused in the filter callback. While this is intentional for testing purposes, consider using explicit suppression or named placeholders.🔎 Proposed fix
add_filter( 'dokan_rest_store_settings_additional_fields', - function ( $additional_fields, $store, $request ) { + function ( $additional_fields ) { return [ 'custom_field' => 'custom_value' ]; }, 10, - 3 + 1 );Since only the return value matters and you're replacing all additional fields, accepting only the first parameter simplifies the code.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
includes/REST/StoreController.phpincludes/REST/StoreSettingController.phpincludes/Traits/VendorAuthorizable.phptests/php/src/REST/StoreSettingControllerTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T06:19:41.376Z
Learnt from: mrabbani
Repo: getdokan/dokan PR: 2891
File: src/admin/dashboard/pages/vendors-single/InformationTabs/WithdrawTab.tsx:58-59
Timestamp: 2025-09-19T06:19:41.376Z
Learning: The Dokan codebase uses 'dokan-lite' as the text domain for WordPress internationalization (__() function calls).
Applied to files:
includes/REST/StoreController.php
🧬 Code graph analysis (1)
includes/Traits/VendorAuthorizable.php (2)
includes/Utilities/VendorUtil.php (1)
VendorUtil(4-89)includes/functions.php (1)
dokan_is_user_seller(73-79)
🪛 GitHub Check: Run PHPCS inspection
tests/php/src/REST/StoreSettingControllerTest.php
[warning] 538-538:
The method parameter $request is never used
[warning] 538-538:
The method parameter $store is never used
[warning] 538-538:
The method parameter $additional_fields is never used
🪛 PHPMD (2.15.0)
includes/Traits/VendorAuthorizable.php
99-99: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
tests/php/src/REST/StoreSettingControllerTest.php
538-538: Avoid unused parameters such as '$additional_fields'. (undefined)
(UnusedFormalParameter)
538-538: Avoid unused parameters such as '$store'. (undefined)
(UnusedFormalParameter)
538-538: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (12)
includes/REST/StoreController.php (4)
5-22: LGTM! Clean trait integration for centralized authorization.The VendorAuthorizable trait import and usage provide a clean separation of authorization concerns. This aligns with the PR objective to centralize vendor/staff permission checks.
321-338: LGTM! Proper 404 handling for missing stores.The
get_storemethod now correctly validates the store exists before proceeding, returning a proper 404 error. This is a public endpoint that appropriately retrieves store data without blocking public access.
460-485: LGTM! Role-based field restrictions are well-implemented.The
get_restricted_fields_for_updatemethod correctly implements tiered field restrictions:
- Staff cannot update
password- Non-admins cannot update commission-related fields
The filter hook allows extensibility.
375-382: Permission check correctly validates store access.The permission check logic is sound. When
update_store_permissions_check()resolves$requested_idviaget_vendor_id_for_user(), it correctly handles both vendor user IDs and vendor staff IDs. The resulting vendor ID is then passed tocan_access_vendor_store(), which compares it against the current user's vendor ID (also resolved viaget_vendor_id_for_user()). This prevents cross-vendor access while correctly allowing staff to access their assigned vendor's store.includes/REST/StoreSettingController.php (2)
16-16: LGTM! Good refactor to extend StoreController.Extending
StoreControllerenables code reuse for authorization logic, vendor resolution, and restricted field handling. This aligns with the PR's goal of centralizing vendor/staff permission checks.
187-195: LGTM! Clean delegation to parent with additional fields support.The
prepare_item_for_responsecorrectly delegates to parent and then adds settings-specific additional fields via the filter.includes/Traits/VendorAuthorizable.php (2)
36-58: LGTM! Well-structured authorization logic.The
can_access_vendor_storemethod correctly implements role-based access:
- Admins can access any store (for proper error handling)
- Non-admins require valid vendor_id
- Vendors/staff can only access their own/assigned store
The docblock clearly explains the authorization rules.
122-124: Clarify edge case handling: user with bothvendor_staffanddokandarcapabilities.The
is_staff_onlymethod returns! dokan_is_user_seller( $user_id, true ) && dokan_is_user_seller( $user_id ). When a user has both thevendor_staffcapability anddokandarrole, the method returnstrue(treating them as staff-only), because:
dokan_is_user_seller( $user_id, true )returnsfalse(vendor_staff is explicitly excluded)dokan_is_user_seller( $user_id )returnstrue(dokandar role is present)Verify this is the intended behavior: should a user with both capabilities be classified as "staff-only" and therefore restricted from admin commission fields, or should they be treated as a vendor owner?
tests/php/src/REST/StoreSettingControllerTest.php (4)
1-69: LGTM! Comprehensive test setup with proper vendor staff creation.The test class is well-structured with clear setup including vendor staff users for both vendors. The hook tracking mechanism is clean and effective.
76-93: LGTM! Thorough authorization test coverage.The tests properly verify:
- Admin can access any vendor settings and sees sensitive data
- Vendor can access their own settings
- Vendor staff can access their assigned vendor's settings
- Cross-vendor access is properly blocked
This aligns well with the PR objectives.
Also applies to: 131-144, 211-224
271-317: LGTM! Critical security tests for restricted fields.These tests verify that vendor staff cannot update
passwordfields, which is essential for security. The tests correctly verify that the request succeeds (200) but the restricted fields are not actually updated.
457-478: Good test for public data filtering.This test verifies that customers accessing vendor settings (which is apparently allowed) receive only public data with sensitive fields filtered out. This aligns with the authorization-based data shaping in
get_restricted_fields_for_view.
| public function update_settings( $request ) { | ||
| $vendor = $this->get_vendor( $request ); | ||
| $params = $request->get_params(); | ||
| $store_id = dokan()->vendor->update( $vendor->get_id(), $params ); | ||
| $vendor_id = (int) $request->get_param( 'vendor_id' ); | ||
| $request->set_param( 'id', $vendor_id ); | ||
| $response = parent::update_store( $request ); | ||
|
|
||
| if ( is_wp_error( $store_id ) ) { | ||
| return new WP_Error( $store_id->get_error_code(), $store_id->get_error_message() ); | ||
| if ( is_wp_error( $response ) ) { | ||
| return $response; | ||
| } | ||
|
|
||
| $store = dokan()->vendor->get( $store_id ); | ||
| $store = dokan()->vendor->get( $vendor_id ); | ||
|
|
||
| do_action( 'dokan_rest_store_settings_after_update', $store, $request ); | ||
|
|
||
| $stores_data = $this->prepare_item_for_response( $store, $request ); | ||
| $response = rest_ensure_response( $stores_data ); | ||
|
|
||
| return $response; | ||
| } |
There was a problem hiding this comment.
Edge case: vendor_id = 0 when parameter is not provided.
When vendor_id is not in the request, (int) $request->get_param( 'vendor_id' ) returns 0. This 0 is then set as the id parameter, which will cause parent::update_store() to fail with a 404 error since store ID 0 is invalid.
The get_vendor method handles missing vendor_id by falling back to the current user, but update_settings doesn't use that fallback. Consider aligning the behavior.
🔎 Proposed fix
public function update_settings( $request ) {
$vendor_id = (int) $request->get_param( 'vendor_id' );
+
+ // Fall back to current user's vendor ID if not provided
+ if ( ! $vendor_id ) {
+ $vendor = $this->get_vendor( $request );
+ if ( ! is_wp_error( $vendor ) && $vendor->get_id() ) {
+ $vendor_id = $vendor->get_id();
+ }
+ }
+
$request->set_param( 'id', $vendor_id );
$response = parent::update_store( $request );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function update_settings( $request ) { | |
| $vendor = $this->get_vendor( $request ); | |
| $params = $request->get_params(); | |
| $store_id = dokan()->vendor->update( $vendor->get_id(), $params ); | |
| $vendor_id = (int) $request->get_param( 'vendor_id' ); | |
| $request->set_param( 'id', $vendor_id ); | |
| $response = parent::update_store( $request ); | |
| if ( is_wp_error( $store_id ) ) { | |
| return new WP_Error( $store_id->get_error_code(), $store_id->get_error_message() ); | |
| if ( is_wp_error( $response ) ) { | |
| return $response; | |
| } | |
| $store = dokan()->vendor->get( $store_id ); | |
| $store = dokan()->vendor->get( $vendor_id ); | |
| do_action( 'dokan_rest_store_settings_after_update', $store, $request ); | |
| $stores_data = $this->prepare_item_for_response( $store, $request ); | |
| $response = rest_ensure_response( $stores_data ); | |
| return $response; | |
| } | |
| public function update_settings( $request ) { | |
| $vendor_id = (int) $request->get_param( 'vendor_id' ); | |
| // Fall back to current user's vendor ID if not provided | |
| if ( ! $vendor_id ) { | |
| $vendor = $this->get_vendor( $request ); | |
| if ( ! is_wp_error( $vendor ) && $vendor->get_id() ) { | |
| $vendor_id = $vendor->get_id(); | |
| } | |
| } | |
| $request->set_param( 'id', $vendor_id ); | |
| $response = parent::update_store( $request ); | |
| if ( is_wp_error( $response ) ) { | |
| return $response; | |
| } | |
| $store = dokan()->vendor->get( $vendor_id ); | |
| do_action( 'dokan_rest_store_settings_after_update', $store, $request ); | |
| return $response; | |
| } |
| public function get_settings( $request ) { | ||
| $vendor = $this->get_vendor( $request ); | ||
| $response = dokan_get_store_info( $vendor->id ); | ||
|
|
||
| $methods_data = dokan_get_container()->get( 'dashboard' )->templates->settings->get_seller_payment_methods( $vendor->get_id() ); | ||
|
|
||
| $response['bank_payment_required_fields'] = dokan_bank_payment_required_fields(); | ||
| $response['active_payment_methods'] = $methods_data['active_methods'] ?? []; | ||
| $response['connected_methods'] = $methods_data['connected_methods'] ?? []; | ||
| $response['disconnected_methods'] = $methods_data['disconnected_methods'] ?? []; | ||
| $response['withdraw_options'] = dokan_withdraw_get_methods(); | ||
| $response['fields_placeholders'] = dokan_bank_payment_fields_placeholders(); | ||
| $response['chargeable_methods'] = dokan_withdraw_get_chargeable_methods(); | ||
| $vendor_id = (int) $request->get_param( 'vendor_id' ); | ||
| $request->set_param( 'id', $vendor_id ); | ||
|
|
||
| return rest_ensure_response( $response ); | ||
| return parent::get_store( $request ); | ||
| } |
There was a problem hiding this comment.
Same edge case in get_settings: missing vendor_id defaults to 0.
Similar to update_settings, when vendor_id is not provided, the code sets id to 0, which will fail in parent::get_store().
🔎 Proposed fix
public function get_settings( $request ) {
$vendor_id = (int) $request->get_param( 'vendor_id' );
+
+ // Fall back to current user's vendor ID if not provided
+ if ( ! $vendor_id ) {
+ $vendor = $this->get_vendor( $request );
+ if ( ! is_wp_error( $vendor ) && $vendor->get_id() ) {
+ $vendor_id = $vendor->get_id();
+ }
+ }
+
$request->set_param( 'id', $vendor_id );
return parent::get_store( $request );
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In includes/REST/StoreSettingController.php around lines 105 to 110,
get_settings currently casts a missing vendor_id to 0 and sets request id to 0
which causes parent::get_store() to fail; update the method to validate the
vendor_id before setting it (check that request has vendor_id and that it casts
to an int > 0), and if missing/invalid return an appropriate WP_Error or
WP_REST_Response with a 400/422 status; only call $request->set_param('id',
$vendor_id) and parent::get_store($request) when vendor_id is valid.
Testing Guide for Store Setting Controller Changes✅ Automated TestsAll unit tests are passing: 20 tests, 49 assertions vendor/bin/phpunit tests/php/src/REST/StoreSettingControllerTest.php🔍 Manual Testing Checklist1. StoreSettingController (v1) AuthorizationVendor Access:
Vendor Staff Access:
Admin Access:
Customer/Public Access:
2. StoreSettingControllerV2 EndpointsTest all 7 endpoints:
Verify field restrictions apply the same way as v1 endpoints 3. Security Checks
4. Edge Cases
📝 Test User Setup Required
|
|
Issue: The social URL not filled for the role vendor staff. |
All Submissions:
Changes proposed in this Pull Request:
This PR fixes an issue where vendor social profile URLs were not visible to vendor staff users in
Vendor Dashboard → Settings → Social Profile.
Previously, the social profile data was fetched strictly based on the logged-in user ID.
For staff users, this resulted in fetching staff-level data instead of the parent vendor’s store data, causing empty social profile fields.
This change ensures that:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Fix: Vendor social profiles not visible for staff users
Fixed an issue where social profile URLs configured by a vendor were not displayed when accessed by vendor staff members. The fix ensures staff users correctly load and manage the parent vendor’s social profile data.
Before Changes
(Screenshots if applicable)
After Changes
(Screenshots if applicable)
Feature Video (optional)
N/A
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.