feat: Add more tests for SSO #211
Conversation
|
Warning Rate limit exceeded@superdav42 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds two new WordPress unit/functional test suites for the SSO component, covering URL/path filters, secret/date handling, broker lookup, return URL construction, broker attach URLs, session handler errors/resume, return type resolution, and encode/decode roundtrips. Tests use multisite context switching, filters, and SSO/SSO_Session_Handler APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Broker Site as Broker Site
participant WP SSO as SSO
participant Session as SSO_Session_Handler
User->>Broker Site: Visit URL with SSO params (broker, token, checksum)
Broker Site->>WP SSO: Validate broker + checksum
alt valid
WP SSO->>Session: start()/resume()
Session-->>WP SSO: target_user_id set
WP SSO->>Broker Site: Build final return URL (login action, done token, redirect)
Broker Site-->>User: Redirect to final URL
else invalid/unauthenticated
WP SSO-->>Broker Site: Throw SSO_Session_Exception (401)
Broker Site-->>User: Error/JSONP response
end
note over WP SSO,Broker Site: URL path may be customized via wu_sso_get_url_path
sequenceDiagram
autonumber
participant Test as Test Case
participant SSO as SSO
participant Hash as Encode/Decode
Test->>SSO: encode(broker_id, salt)
SSO->>Hash: obfuscate
Hash-->>SSO: encoded_id
SSO-->>Test: encoded_id
Test->>SSO: decode(encoded_id, salt)
SSO->>Hash: de-obfuscate
Hash-->>SSO: broker_id
SSO-->>Test: original broker_id
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
🧹 Nitpick comments (5)
tests/unit/SSO_Test.php (3)
19-21: Consider adding cleanup for global state modifications.While the current filter cleanup is good, consider also cleaning up any modifications to
$_REQUESTsuperglobal that might affect other tests.public function tearDown(): void { // Remove any filters set in tests. remove_all_filters('wu_sso_enabled'); remove_all_filters('wu_sso_get_url_path'); remove_all_filters('determine_current_user'); + // Clean up $_REQUEST modifications + unset($_REQUEST['broker']); parent::tearDown(); }
70-70: Document the $_REQUEST modification for clarity.The direct modification of
$_REQUESTsimulates SSO_Session_Handler's expected input but could benefit from a comment explaining the simulation.- // Simulate request param that SSO_Session_Handler reads. - $_REQUEST['broker'] = $broker; + // Simulate the 'broker' request parameter that SSO_Session_Handler::resume() expects + $_REQUEST['broker'] = $broker;
58-61: Avoid hardcoded fallback to user ID 1; create or assert the test user.
self::factory()->user->create() is called — falling back to ID 1 is fragile and may not exist in all WP test setups. Remove the hardcoded fallback and either explicitly create an admin (e.g. self::factory()->user->create(['role'=>'administrator'])) or assert $user_id is truthy. Location: tests/unit/SSO_Test.php:57-60tests/functional/SSO_Functional_Test.php (2)
133-135: Consider cleaning up $_REQUEST modifications in tearDown.The test modifies
$_REQUESTbut doesn't clean it up, which could affect other tests.protected function tearDown(): void { remove_all_filters('wu_sso_enabled'); remove_all_filters('wu_sso_get_url_path'); remove_all_filters('wu_sso_salt'); + // Clean up $_REQUEST modifications from tests + unset($_REQUEST['broker'], $_REQUEST['return_type']); parent::tearDown(); }
100-101: Use a realistic, non‑hardcoded fallback for blog->registered in tests.Replace the hard-coded '2024-01-01 00:00:00' in tests/functional/SSO_Functional_Test.php (lines 100–101) with a generated or repo-default value (e.g., Faker->dateTimeThisYear()->format('Y-m-d H:i:s') or reuse the default $date from inc/models/class-site.php:293) to match how dates are produced elsewhere (inc/class-faker.php:342).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/functional/SSO_Functional_Test.php(1 hunks)tests/unit/SSO_Test.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/SSO_Test.php (1)
inc/sso/class-sso.php (5)
SSO(36-1100)with_sso(1084-1099)salt(881-883)encode(117-119)decode(130-132)
tests/functional/SSO_Functional_Test.php (3)
inc/sso/class-sso.php (9)
SSO(36-1100)get_url_path(1063-1074)with_sso(1084-1099)calculate_secret_from_date(942-953)salt(881-883)encode(117-119)get_broker_by_id(978-1007)get_final_return_url(786-819)get_return_type(827-838)inc/sso/class-sso-broker.php (2)
SSO_Broker(24-55)getAttachUrl(41-54)tests/unit/SSO_Test.php (2)
setUp(11-15)tearDown(17-23)
🪛 PHPMD (2.15.0)
tests/functional/SSO_Functional_Test.php
30-30: Avoid unused parameters such as '$default'. (undefined)
(UnusedFormalParameter)
30-30: Avoid unused parameters such as '$action'. (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). (12)
- GitHub Check: cypress (8.4, chrome)
- GitHub Check: Code Quality Checks
- GitHub Check: cypress (8.1, firefox)
- GitHub Check: cypress (8.3, chrome)
- GitHub Check: cypress (8.2, firefox)
- GitHub Check: cypress (8.4, firefox)
- GitHub Check: cypress (8.3, firefox)
- GitHub Check: cypress (8.2, chrome)
- GitHub Check: cypress (8.0, firefox)
- GitHub Check: cypress (7.4, firefox)
- GitHub Check: cypress (8.1, chrome)
- GitHub Check: cypress (7.4, chrome)
🔇 Additional comments (4)
tests/unit/SSO_Test.php (1)
1-82: LGTM! Well-structured unit tests with good coverage.The unit tests are well-organized and cover key SSO functionality including URL augmentation, encoding/decoding, and session handling. The test structure follows WordPress testing conventions properly.
tests/functional/SSO_Functional_Test.php (3)
1-153: LGTM! Comprehensive functional tests with proper multisite handling.The functional tests provide excellent coverage of SSO behaviors including custom URL paths, secret generation, broker management, return URLs, and session handling. The multisite context switching is handled correctly with proper try/finally blocks.
30-33: PHPMD false positive - parameters are required by WordPress filter signature.The static analysis tool flagged
$defaultand$actionparameters as unused, but these are required by the WordPress filter callback signature and cannot be removed.
141-151: Excellent test coverage for return type validation.The test thoroughly covers all valid return types (
jsonp,json,redirect) and properly validates that invalid values default toredirect. This aligns perfectly with the implementation ininc/sso/class-sso.phplines 826-837.
#85
Summary by CodeRabbit