fix(sso): lazily generate admin-bar magic links - #1712
Conversation
superdav42
added
the
origin:worker
📝 WalkthroughWalkthroughAdmin-bar dashboard links now use same-origin lazy action URLs. An authenticated handler validates the site, nonce, access, and destination before redirecting. Super administrators can access any site, and focused tests cover URL generation, rejection cases, redirects, and access verification. ChangesAdmin-bar magic-link resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdminBar
participant AdminPostHandler
participant SiteAccess
participant DashboardURL
AdminBar->>AdminPostHandler: Submit site ID and nonce
AdminPostHandler->>SiteAccess: Verify authenticated site access
SiteAccess-->>AdminPostHandler: Return validation result
AdminPostHandler->>DashboardURL: Resolve the selected site dashboard
DashboardURL-->>AdminPostHandler: Return validated URL
AdminPostHandler-->>AdminBar: Redirect to the dashboard
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Completion Summary
aidevops.sh v3.32.247 plugin for OpenCode v1.18.9 with gpt-5.6-terra spent 14m and 171,884 tokens on this as a headless worker. Merged via PR #1712 to main. Routing feedback
|
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php (1)
68-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required test fixture helpers.
tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php#L68-L72: Create the user and site membership with the applicablewu_create_*()helper.tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php#L129-L140: Replace user and site factory fixtures with the applicablewu_create_*()helpers.tests/WP_Ultimo/SSO/Magic_Link_Test.php#L160-L163: Replace user and site factory fixtures with the applicablewu_create_*()helpers.As per coding guidelines, tests must use
wu_create_*()helper functions to set up test data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php` around lines 68 - 72, Replace the direct WordPress factory and membership setup with the applicable wu_create_*() test helpers at tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php:68-72, including user creation and site membership; update the user and site fixtures at tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php:129-140 and tests/WP_Ultimo/SSO/Magic_Link_Test.php:160-163 to use the corresponding helpers while preserving each test’s existing setup behavior.Source: Coding guidelines
inc/sso/class-admin-bar-magic-links.php (1)
106-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required PHP syntax.
Use
[]instead ofarray()at line 107. Use a Yoda condition at line 148.0 === strcasecmp($destination_host, $host)preserves the case-insensitive comparison.Proposed fix
- array( + [ 'action' => self::ADMIN_POST_ACTION, self::SITE_ID_QUERY_ARG => $site_id, '_wpnonce' => wp_create_nonce($this->get_admin_bar_nonce_action($site_id)), - ), + ], @@ - if (strtolower($destination_host) === strtolower($host)) { + if (0 === strcasecmp($destination_host, $host)) {As per coding guidelines, use “short array syntax
[]” and require “Yoda conditions … in production code.”Also applies to: 147-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/sso/class-admin-bar-magic-links.php` around lines 106 - 112, Update the array argument in the admin-post URL construction within the relevant method to use short array syntax [] instead of array(). In the destination-host comparison around strcasecmp(), use the Yoda condition 0 === strcasecmp($destination_host, $host) while preserving the existing case-insensitive behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inc/sso/class-admin-bar-magic-links.php`:
- Around line 122-163: Add tests that execute handle_admin_bar_magic_link()
directly, covering valid same-domain and mapped-domain redirects, invalid
nonces, unauthorized or invalid sites, and request-supplied redirect parameters.
Assert the expected redirect destination and status, ensure access failures are
rejected, and verify request parameters cannot override the handler’s resolved
site destination.
---
Nitpick comments:
In `@inc/sso/class-admin-bar-magic-links.php`:
- Around line 106-112: Update the array argument in the admin-post URL
construction within the relevant method to use short array syntax [] instead of
array(). In the destination-host comparison around strcasecmp(), use the Yoda
condition 0 === strcasecmp($destination_host, $host) while preserving the
existing case-insensitive behavior.
In `@tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php`:
- Around line 68-72: Replace the direct WordPress factory and membership setup
with the applicable wu_create_*() test helpers at
tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php:68-72, including user
creation and site membership; update the user and site fixtures at
tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php:129-140 and
tests/WP_Ultimo/SSO/Magic_Link_Test.php:160-163 to use the corresponding helpers
while preserving each test’s existing setup behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f2a6c20-664c-4755-a576-a0450a6f7e7a
📒 Files selected for processing (4)
inc/sso/class-admin-bar-magic-links.phpinc/sso/class-magic-link.phptests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.phptests/WP_Ultimo/SSO/Magic_Link_Test.php
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php (1)
115-120: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAssert the same-origin action endpoint.
Lines 115-120 validate only query parameters. This test also passes if the dashboard URL has another host or another path with the same parameters. Assert that the scheme, host, and path match
admin_url('admin-post.php').As per PR objectives, dashboard links must use a same-origin
admin-postredirect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php` around lines 115 - 120, Extend the assertions in the dashboard link test around $dashboard_node->href to verify that its scheme, host, and path match the corresponding components of admin_url('admin-post.php'). Keep the existing query-parameter, nonce, and generated-link assertions unchanged.
🧹 Nitpick comments (1)
tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php (1)
178-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse short array syntax in the changed PHP tests.
Replace
array(...)with[...]at each changed site.
tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php#L178-L189: convert the request and redirect fixtures.tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php#L80-L113: convert the admin-bar node and query fixtures.tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php#L156-L163: convert malformed request fixtures.tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php#L220-L251: convert mapped-domain and redirect fixtures.tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php#L296-L311: convert rejection request fixtures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php` around lines 178 - 189, In Admin_Bar_Magic_Links_Test.php, replace long array(...) syntax with short [...] syntax for all changed fixtures: request and redirect fixtures at lines 178-189, admin-bar node and query fixtures at 80-113, malformed request fixtures at 156-163, mapped-domain and redirect fixtures at 220-251, and rejection request fixtures at 296-311.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php`:
- Around line 242-245: Update the magic-link filter callback in the test to
capture and assert that $filter_user_id and $filter_site_id match the expected
$user_id and $site_id, while preserving the existing redirect assertion and
return value. Ensure the test also verifies selected-site-only resolution rather
than allowing magic-link generation for another user or site.
- Around line 301-317: Add $this->fail() immediately after each
handle_admin_bar_magic_link() call in the two rejection-case try blocks, before
their WPDieException catches, so both invalid and inaccessible site scenarios
require wp_die() rather than allowing normal return.
---
Outside diff comments:
In `@tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php`:
- Around line 115-120: Extend the assertions in the dashboard link test around
$dashboard_node->href to verify that its scheme, host, and path match the
corresponding components of admin_url('admin-post.php'). Keep the existing
query-parameter, nonce, and generated-link assertions unchanged.
---
Nitpick comments:
In `@tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php`:
- Around line 178-189: In Admin_Bar_Magic_Links_Test.php, replace long
array(...) syntax with short [...] syntax for all changed fixtures: request and
redirect fixtures at lines 178-189, admin-bar node and query fixtures at 80-113,
malformed request fixtures at 156-163, mapped-domain and redirect fixtures at
220-251, and rejection request fixtures at 296-311.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7302c3bf-c68b-40ab-8708-4e992b906195
📒 Files selected for processing (1)
tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
|
The full-suite failures are deterministic test-isolation defects in the new tests, not unrelated infrastructure noise.
Please make both tests explicit about their same-domain/no-magic-link fixture: add Verification: vendor/bin/phpunit --filter Admin_Bar_Magic_Links_Test
vendor/bin/phpunitThe production code and focused lazy-link behavior otherwise match #1711; the existing CI failures occurred only in the 10,155-test full suite. aidevops.sh v3.32.247 plugin for OpenCode v1.18.9 with gpt-5.6-sol |
CI repair feedback routed to issue #1711This worker PR had terminal failed CI checks. The check details have been appended Terminal failed checks: Closed by deterministic merge pass (pulse-merge.sh). |
superdav42
added
the
ci-feedback-routed
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php (1)
106-108: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the test filter when the method throws.
If
modify_my_sites_menu()throws, Line 108 does not run. Thewu_magic_link_urlfilter then remains registered and can change later test results. Put the invocation and filter removal in atry/finallyblock.Proposed fix
add_filter('wu_magic_link_url', $magic_link_filter); -$this->magic_links->modify_my_sites_menu($admin_bar); -remove_filter('wu_magic_link_url', $magic_link_filter); +try { + $this->magic_links->modify_my_sites_menu($admin_bar); +} finally { + remove_filter('wu_magic_link_url', $magic_link_filter); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php` around lines 106 - 108, Wrap the modify_my_sites_menu invocation and filter cleanup in a try/finally block, ensuring remove_filter for wu_magic_link_url always executes even when modify_my_sites_menu throws. Keep the existing filter setup and test assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php`:
- Around line 106-108: Wrap the modify_my_sites_menu invocation and filter
cleanup in a try/finally block, ensuring remove_filter for wu_magic_link_url
always executes even when modify_my_sites_menu throws. Keep the existing filter
setup and test assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 56981ef8-61ec-4032-8565-30bc1d05dfca
📒 Files selected for processing (2)
inc/sso/class-admin-bar-magic-links.phptests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php
🚧 Files skipped from review as they are similar to previous changes (1)
- inc/sso/class-admin-bar-magic-links.php
|
DISPATCH_CLAIM nonce=4bfeb49291db691755ac4b57153b4184 runner=superdav42 ts=2026-08-10T19:07:03Z max_age_s=120 version=3.32.250 opencode_version=1.18.9 lease_token=4bfeb49291db691755ac4b57153b4184 device=device-1783824528-2609248-26808 session=issue-1712 phase=prelaunch expires_at=1786388944 |
|
REVIEW_FOLLOWUP_CREATED source_pr=1712 issue=1715 fingerprint=source-pr-1712 runner=superdav42 ts=2026-08-10T19:07:26Z |
1 participant
Summary
Testing
vendor/bin/phpunit --filter Admin_Bar_Magic_Links_Testvendor/bin/phpunit --filter Magic_Link_Testvendor/bin/phpcs inc/sso/class-admin-bar-magic-links.php inc/sso/class-magic-link.php tests/WP_Ultimo/SSO/Admin_Bar_Magic_Links_Test.php tests/WP_Ultimo/SSO/Magic_Link_Test.phpvendor/bin/phpstan analyse inc/sso/class-admin-bar-magic-links.php inc/sso/class-magic-link.phpResolves #1711
aidevops.sh v3.32.247 plugin for OpenCode v1.18.9 with gpt-5.6-terra
Summary by CodeRabbit
Bug Fixes
Tests