Split authentication method interfaces - #115
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #115 +/- ##
============================================
+ Coverage 95.87% 95.93% +0.06%
- Complexity 77 79 +2
============================================
Files 10 10
Lines 194 197 +3
============================================
+ Hits 186 189 +3
Misses 8 8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors authentication challenges to be handled by dedicated failure handlers instead of being part of AuthenticationMethodInterface, while keeping legacy challenge() behavior working for older authentication methods.
Changes:
- Removes
challenge()fromAuthenticationMethodInterfaceand updates middleware to call legacychallenge()only when applicable. - Introduces
HttpBasicFailureHandlerandHttpBearerFailureHandlerto configureWWW-Authenticatechallenges via failure handlers. - Updates tests, README usage examples, and changelog to reflect the new challenge configuration approach.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/HttpAuthenticationFailureHandlerTest.php | Adds coverage for new Basic/Bearer failure handler challenge headers and realm immutability. |
| tests/AuthenticationMiddlewareTest.php | Updates middleware tests to reflect removal of interface challenge() and adds legacy compatibility/preference tests. |
| src/Middleware/Authentication.php | Implements backward-compatible legacy challenge() invocation while prioritizing configured failure-handler challenges. |
| src/Method/Composite.php | Makes challenge() propagation optional via is_callable() to support methods that no longer implement it. |
| src/Handler/HttpBearerFailureHandler.php | Adds new failure handler that attaches a Bearer WWW-Authenticate challenge. |
| src/Handler/HttpBasicFailureHandler.php | Adds new failure handler that attaches a Basic WWW-Authenticate challenge. |
| src/Debug/AuthenticationMethodInterfaceProxy.php | Keeps legacy challenge() forwarding behavior guarded by is_callable(). |
| src/AuthenticationMethodInterface.php | Removes challenge() from the interface. |
| README.md | Updates configuration examples to use failure handlers for challenges (and documents legacy behavior). |
| CHANGELOG.md | Notes the enhancement for configurable challenges via failure handlers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8036517 to
812a09e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
README.md:80
- The README states that the failure handler adds the
WWW-Authenticateresponse header, but in this package the header is added by the authentication method viachallenge()(e.g.,src/Method/HttpBearer.php:26-29). With the newChallengeInterface, this should be attributed to the authenticator/method (or described as optional if not all authenticators implement challenges) to avoid misleading users.
Bearer HTTP authentication is typically used in APIs. The authentication token is passed in the request's
`Authorization` header. On failure, the failure handler adds a `WWW-Authenticate` response header.
src/Method/Composite.php:49
Composite::challenge()silently ignores invalid entries in$methods(becauseinstanceofreturns false for non-objects), whileauthenticate()throws for invalid entries. This makes misconfiguration harder to detect ifchallenge()is called directly (or beforeauthenticate()). Consider validating$methodsconsistently here as well (or validate once in the constructor).
foreach ($this->methods as $method) {
if ($method instanceof ChallengeInterface) {
$response = $method->challenge($response);
}
}
Closes #113.