Feat/api keys oauth #3961
Feat/api keys oauth #3961
Conversation
📝 WalkthroughWalkthroughAdds API key and OAuth2 authentication: new ApiKeyAuthenticator, AuthChain combining session/api-key/OAuth2, ApiKeyController for per-user key CRUD, database schema + migration for API keys and OAuth2 tables, DI service registrations, OAuth2 server/resource facades, and accompanying unit tests. Changes
Sequence DiagramsequenceDiagram
autonumber
actor Client
participant AuthChain
participant CurrentUser
participant ApiKeyAuthenticator
participant Database
participant OAuth2Resource
Client->>AuthChain: authenticate(request, requiredScopes)
AuthChain->>CurrentUser: getUser()
alt session user present
CurrentUser-->>AuthChain: user_id
AuthChain-->>Client: success (source: session)
else no session user
AuthChain->>ApiKeyAuthenticator: authenticate(request, requiredScopes)
ApiKeyAuthenticator->>ApiKeyAuthenticator: parse Authorization: Bearer
alt bearer token present
ApiKeyAuthenticator->>Database: SELECT ... WHERE api_key = hashed(token)
Database-->>ApiKeyAuthenticator: api_key_record
ApiKeyAuthenticator->>ApiKeyAuthenticator: verify expiry & scopes
alt valid
ApiKeyAuthenticator->>Database: UPDATE last_used_at
ApiKeyAuthenticator-->>AuthChain: user_id
AuthChain-->>Client: success (source: api_key)
else invalid/expired/insufficient scopes
ApiKeyAuthenticator-->>AuthChain: failure
AuthChain->>OAuth2Resource: call authenticator (if configured)
alt OAuth2 returns user_id
OAuth2Resource-->>AuthChain: user_id
AuthChain-->>Client: success (source: oauth2)
else
AuthChain-->>Client: failure
end
end
else no bearer token
ApiKeyAuthenticator-->>AuthChain: failure
AuthChain->>OAuth2Resource: call authenticator (if configured)
OAuth2Resource-->>AuthChain: result
AuthChain-->>Client: result
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/phpMyFAQ/Captcha/GoogleRecaptchaTest.php (1)
92-104:⚠️ Potential issue | 🟡 Minor
expectNotToPerformAssertions()— this test verifies nothing.The test sets up a mock but never calls
checkCaptchaCode, so it exercises no behavior. If the intent is to verify configuration access when the user is not logged in, the method should actually be invoked and the result asserted (using the same HTTP-abstraction approach suggested above).
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ApiKeyController.php`:
- Around line 143-158: The UPDATE currently returns 200 even when no rows are
affected; after executing the query in ApiKeyController (the $update SQL using
$db->query($update)), check the number of affected rows (e.g. $db->numRows() or
the DB driver's equivalent) and if it is 0 return a 404 JSON response (similar
shape to the existing error response) indicating the API key was not found or
does not belong to the current user (use $this->currentUser->getUserId() to
validate), otherwise continue returning the successful JSON payload.
- Line 129: The route parameter retrieval in ApiKeyController currently casts
$request->attributes->get('id') to int without checking for null, which turns
missing ids into 0; update the code in the methods around the lines referencing
this retrieval (the id extraction at ApiKeyController where $id is assigned, and
the similar occurrence at the second location) to first check that the attribute
exists and is not null (e.g. $request->attributes->has('id') or
$request->attributes->get('id') !== null) and if missing return an appropriate
error (throw a 404/400 or return a JSON error response) instead of proceeding
with id = 0; ensure you still cast to int after the presence check so subsequent
DB WHERE clauses use a valid id.
- Around line 172-197: The delete method in ApiKeyController currently relies on
json_decode($request->getContent(), ..., JSON_THROW_ON_ERROR) to read a CSRF
token, which will throw a JsonException when DELETE bodies are missing; update
ApiKeyController::delete to robustly obtain the CSRF token by first checking
$request->headers->get('X-CSRF-Token') and $request->query->get('csrf') and only
falling back to decoding the body (using a non-throwing decode or catching
JsonException) from $request->getContent(); then pass the retrieved token into
verifySessionCsrfToken('api-key-delete', $csrf) and keep existing error
responses for verification failure and DB errors (references: delete method,
$request->getContent(), JSON_THROW_ON_ERROR, verifySessionCsrfToken).
- Around line 82-99: The insert in ApiKeyController currently stores the
plaintext $apiKey in the faqapi_keys table; change this to store a one-way hash
(e.g., hash('sha256', $apiKey)) instead of the raw token (adjust the INSERT
generated in ApiKeyController to persist the hashed value), keep returning the
plaintext $apiKey to the user only at creation, and update
ApiKeyAuthenticator::authenticate to hash the incoming bearer token with the
same algorithm before performing the WHERE lookup against the stored hash.
Ensure the column you use (existing api_key or a new api_key_hash) is
consistently used in both ApiKeyController and ApiKeyAuthenticator and that any
NULL/expiry logic (expires_at, last_used_at) remains unchanged.
In `@phpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.php`:
- Around line 423-438: Update the faqapiKeys() TableBuilder so the created
timestamp is NOT NULL by changing the timestamp declaration to include not-null
and default behavior (use ->timestamp('created', false, true) to match
faqpush_subscriptions); for the id field, either leave the existing
integer('id', false) as-is (since ApiKeyController uses $db->nextId()) or if you
switch to ->autoIncrement('id') you must also update ApiKeyController to stop
calling nextId() and remove id from the INSERT—make a deliberate choice and
update the corresponding controller if switching to auto-increment.
In `@tests/phpMyFAQ/Captcha/GoogleRecaptchaTest.php`:
- Around line 171-179: Test currently accepts any outcome (bool or exception)
making it unfalsifiable; change the assertion to require a deterministic false
for an empty token by calling googleRecaptcha->checkCaptchaCode('') and
asserting false instead of swallowing exceptions: replace the try/catch around
$this->googleRecaptcha->checkCaptchaCode('') in GoogleRecaptchaTest with a
direct $this->assertFalse(...) assertion (keep the existing
set_error_handler/restore_error_handler scaffolding if needed to suppress
warnings).
- Around line 146-156: The test GoogleRecaptchaTest currently swallows all
outcomes by using try/catch (\Throwable) around checkCaptchaCode and only
asserting that any thrown exception has a non-empty message, making the test
unfalsifiable; instead, control the external HTTP call (file_get_contents) used
by checkCaptchaCode by either injecting an HTTP client dependency into the class
under test or registering a custom stream wrapper to return a deterministic JSON
response, then remove the broad catch and assert the exact expected boolean
(e.g., $this->assertTrue(...) or $this->assertFalse(...)) for
checkCaptchaCode('test-token') based on your mocked response, or if testing
error behavior keep a narrow exception expectation for the specific exception
class; reference GoogleRecaptchaTest::checkCaptchaCode, the
set_error_handler/restore_error_handler usage, and file_get_contents/get() mock
point when implementing this change.
In `@tests/phpMyFAQ/Plugin/PluginManagerTest.php`:
- Line 279: The test stub's registerEvents method has an unused $dispatcher
parameter causing PHPMD warnings; fix it by referencing the parameter inside the
method body (e.g. add a no-op like (void)$dispatcher;) in the
registerEvents(\Symfony\Component\EventDispatcher\EventDispatcherInterface
$dispatcher): void {} implementation so the parameter is used and the PHPMD
unused-parameter warning is silenced.
🧹 Nitpick comments (6)
phpmyfaq/src/phpMyFAQ/Auth/ApiKeyAuthenticator.php (2)
59-61: SQL query uses string interpolation — consider parameterized queries.The API key value is regex-validated to
[A-Za-z0-9_]and additionally escaped with$db->escape(), so this is safe against injection. However, building SQL viasprintfwith inline values is fragile and deviates from the principle of parameterized queries. This pattern is used elsewhere in the codebase (e.g.,ApiKeyController), so it's consistent — but worth flagging as a debt item.
82-87: Silent failure onlast_used_atUPDATE.The return value of the
UPDATEquery is not checked. If it fails (e.g., DB connection issue), the user is still authenticated but the usage timestamp won't be recorded. This is likely acceptable for a non-critical metadata field, but consider logging the failure for observability.tests/phpMyFAQ/Controller/Administration/Api/ApiKeyControllerTest.php (2)
18-36:$this->configurationis stored but never referenced in test methods.The
setUp()callsConfiguration::getConfigurationInstance()and stores it in$this->configuration, but none of the four test methods use this property. If the intent is to initialize the configuration singleton for the controller, the return value doesn't need to be stored.🔧 Suggested cleanup
- private Configuration $configuration; - /** * `@throws` Exception */ protected function setUp(): void { parent::setUp(); Strings::init(); Translation::create() ->setTranslationsDir(PMF_TRANSLATION_DIR) ->setDefaultLanguage('en') ->setCurrentLanguage('en') ->setMultiByteLanguage(); - $this->configuration = Configuration::getConfigurationInstance(); + Configuration::getConfigurationInstance(); }
41-86: Tests only verify that unauthenticated calls throw — consider expanding coverage.The four tests confirm that the permission guard (
userHasPermission) rejects unauthenticated requests. This is a good baseline. Future tests should cover authenticated happy paths (key creation, listing, update, delete) and edge cases (duplicate names, invalid CSRF, expired keys) to validate the controller logic beyond the auth gate. Based on learnings, admin API routes should follow the naming conventionadmin.api.{resource}.{action}— the controller's route names (admin.api.user.api-keys.list, etc.) comply.phpmyfaq/src/phpMyFAQ/Auth/AuthChain.php (2)
88-96: Getter methods duplicate the public property hooks.
getAuthenticatedUserId()andgetAuthSource()merely forward to the$authenticatedUserIdand$authSourceproperty hooks (lines 31-37). Unless these are required for an interface or to maintain a consistent API surface, consider removing them to avoid the duplicate access paths.
39-40: Ambiguous PHPDoc type for$oauth2Authenticator.
callable(Request): ?int|nullis ambiguous — it could be read ascallable(Request): (?int | null)or(callable(Request): ?int) | null. Since the intent is "nullable callable that returns?int", clarify:- /** `@var` callable(Request): ?int|null */ + /** `@var` (callable(Request): ?int)|null */
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ApiKeyController.php
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ApiKeyController.php
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ApiKeyController.php
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ApiKeyController.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ApiKeyController.php`:
- Around line 37-52: The list() method in ApiKeyController returns DB rows with
the scopes column still JSON-encoded; decode the scopes field for each row
before returning so it matches the array shape used by create/update. In the
list() implementation (ApiKeyController::list), iterate over $rows and replace
each row's 'scopes' with json_decode($row['scopes'], true) and default to an
empty array if decoding fails or value is null/empty; then return the
transformed $rows in the json response. Ensure this handles non-string/null
values robustly and preserves other fields unchanged.
🧹 Nitpick comments (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ApiKeyController.php (1)
188-223: DELETE returns 200 success even when the key doesn't exist (0 rows affected).Similar to the previous update concern that was addressed: when
iddoesn't belong to the user or doesn't exist,DELETE ... WHERE id = X AND user_id = Ysucceeds with 0 affected rows. The response is still{"success": true}. While idempotent deletes are sometimes acceptable, returning 404 when nothing was deleted would be more informative for the client.
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ApiKeyController.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Auth/OAuth2/AuthorizationServer.php`:
- Around line 119-153: buildLeagueAuthorizationServer is currently public and
constructs a CryptKey with file-permission checks disabled; make it private if
it's only called internally from issueToken() and completeAuthorization(), and
change the CryptKey instantiation in buildLeagueAuthorizationServer to enable
permission checks (remove the false flag or set the third parameter to true) so
the private key is validated by the library; update any call sites (issueToken,
completeAuthorization) if needed to access the now-private method.
- Around line 161-196: completeAuthorization currently returns 'body' as a raw
string while issueToken returns an associative array; make completeAuthorization
return the same shape by decoding the response body into an array. Replace the
raw cast (string) $response->getBody() with json_decode((string)
$response->getBody(), true, 512, JSON_THROW_ON_ERROR) (and handle potential
exceptions consistently), referring to the completeAuthorization method and the
$response->getBody() usage so callers receive an array<string,mixed> like
issueToken.
In `@phpmyfaq/src/phpMyFAQ/Auth/OAuth2/Repository/AccessTokenRepository.php`:
- Around line 59-75: The INSERT builds SQL in AccessTokenRepository using
unescaped $accessTokenEntity->getExpiryDateTime()->format('Y-m-d H:i:s') and
treats any $this->db()->query(...) === false as a unique-token error; fix by
escaping the formatted expiry datetime with $this->db()->escape(...) like the
other values when building the SQL (keep the same use of
$this->table('faqoauth_access_tokens') and $this->db()->now()), and after
$this->db()->query(...) returns false inspect the database error/errno via your
DB wrapper (e.g. $this->db()->errno() or $this->db()->error()) to only throw
UniqueTokenIdentifierConstraintViolationException::create() for a
duplicate-identifier constraint and rethrow or throw a more general exception
(or include the DB error) for other failure reasons.
In `@phpmyfaq/src/phpMyFAQ/Auth/OAuth2/Repository/UserRepository.php`:
- Around line 46-50: The SQL string in UserRepository.php builds $query with a
trailing "LIMIT 1" which is incompatible with SQL Server; update the query
construction in the method that creates $query (the code building $query in
UserRepository.php / the UserRepository class) to be DB-agnostic by removing the
"LIMIT 1" clause (since login is unique) or by using the project's existing
cross-database helper for limiting (e.g., switch to a TOP 1 variant when the
connection is SQL Server); ensure you only modify the SQL string creation
($query = sprintf(...)) so it no longer emits "LIMIT 1" for SQL Server.
In `@phpmyfaq/src/phpMyFAQ/Auth/OAuth2/ResourceServer.php`:
- Around line 76-92: The code constructs a CryptKey with permission checks
disabled (new CryptKey($publicKeyPath, null, false)) in
ResourceServer::toPsr7Request / the try block, which weakens security; change
this to enable the library's file-permission check by removing the explicit
false or making the flag configurable (e.g., use new CryptKey($publicKeyPath) or
read a config value and pass true by default), and update any configuration or
constructor for phpMyFAQ\Auth\OAuth2\ResourceServer to allow toggling the
permission-check flag so production defaults to enabled.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/OAuth2Controller.php`:
- Around line 99-102: The getAuthorizationServer method currently returns
$this->authorizationServer or blindly fetches
'phpmyfaq.auth.oauth2.authorization-server' from the container, which can throw
an opaque error or return null; update getAuthorizationServer to check
$this->authorizationServer first, then use
$this->container->has('phpmyfaq.auth.oauth2.authorization-server') before
calling get, and if the service is missing or returns null throw a descriptive
RuntimeException (or InvalidArgumentException) mentioning getAuthorizationServer
and the service id so callers always receive a non-null
OAuth2AuthorizationServer instance.
- Around line 34-53: In token(), don't return raw $exception->getMessage() to
callers; inspect the $statusCode from the caught RuntimeException (from
AuthorizationServer->issueToken) and if it's a 5xx (>=500 && <=599) log the full
exception (e.g. $this->logger->error($exception) or use an injected logger) and
return a generic error_description like "Internal server error" (keep the
existing 'oauth2_unavailable' code); for 4xx errors you may continue to return
the original message. Ensure you remove direct exposure of
$exception->getMessage() in the JsonResponse for 5xx and keep logging the
original exception for diagnostics.
- Around line 58-82: The authorize(Request $request) action currently
auto-approves by default; change it so approval is not assumed: for POST
requests require the 'approve' parameter to be explicitly present and parsed (do
not default to 'true'), and for GET requests always render/return a consent
prompt (or a 400) instead of approving; additionally add CSRF token validation
on POST (validate a CSRF token from the request before calling
getAuthorizationServer()->completeAuthorization) and only call
completeAuthorization when currentUser->isLoggedIn(), the explicit approval
value is true, and CSRF validation passes; update references in this method
(authorize, getAuthorizationServer(), currentUser) accordingly.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php`:
- Around line 644-714: The migration in Migration420Alpha creates OAuth2 tables
with columns like created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, but the
DatabaseSchema builder uses ->timestamp('created') leaving them nullable; update
the DatabaseSchema definitions for all OAuth2 tables to match the migration by
using ->timestamp('created', false, true) (or the equivalent non-null with
default arguments) so DatabaseSchema and Migration420Alpha produce the same NOT
NULL DEFAULT CURRENT_TIMESTAMP semantics and avoid schema drift.
In `@tests/phpMyFAQ/Controller/Api/OAuth2ControllerTest.php`:
- Around line 76-83: The test creates OAuth2Controller with new
OAuth2Controller() but doesn't initialize currentUser, causing a TypeError when
authorize() calls $this->currentUser->isLoggedIn(); update
testAuthorizeRequiresAuthenticatedUser to inject a mock/stub into the
controller's currentUser that has isLoggedIn() returning false (or otherwise set
a non-null user object) before calling authorize(); target the OAuth2Controller
instance in the test and set its currentUser property or use the controller's
setter if available so authorize() runs and returns the expected 401 and
access_denied content.
🧹 Nitpick comments (11)
phpmyfaq/src/phpMyFAQ/Auth/OAuth2/Repository/AuthCodeRepository.php (1)
64-71: Silent failure on revocation — consider error handling.If the
UPDATEquery fails (e.g., DB unavailable), the auth code remains unrevoked with no indication of failure. Since revocation is a security-critical operation, consider throwing an exception or at minimum logging a warning on failure.Proposed error handling
public function revokeAuthCode(string $codeId): void { - $this->db()->query(sprintf( + $result = $this->db()->query(sprintf( "UPDATE %s SET revoked = 1 WHERE identifier = '%s'", $this->table('faqoauth_auth_codes'), $this->db()->escape($codeId), )); + + if ($result === false) { + throw new \RuntimeException( + sprintf('Failed to revoke auth code: %s', $codeId) + ); + } }phpmyfaq/src/phpMyFAQ/Auth/OAuth2/Repository/RefreshTokenRepository.php (2)
51-58: Same silent-failure concern asAuthCodeRepository::revokeAuthCode.Refresh token revocation is equally security-critical. A failed
UPDATEshould be surfaced rather than silently ignored. See the same recommendation fromAuthCodeRepository.Proposed error handling
public function revokeRefreshToken(string $tokenId): void { - $this->db()->query(sprintf( + $result = $this->db()->query(sprintf( "UPDATE %s SET revoked = 1 WHERE identifier = '%s'", $this->table('faqoauth_refresh_tokens'), $this->db()->escape($tokenId), )); + + if ($result === false) { + throw new \RuntimeException( + sprintf('Failed to revoke refresh token: %s', $tokenId) + ); + } }
27-78: Revoke/isRevoked logic is nearly identical toAuthCodeRepository— consider extracting toAbstractRepository.The
revokeX()andisXRevoked()methods in bothAuthCodeRepositoryandRefreshTokenRepositoryfollow the exact same pattern, differing only by table name. This could be DRY-ed intoAbstractRepositorywith a shared helper (e.g.,revokeByIdentifier(string $table, string $id)andisRevokedByIdentifier(string $table, string $id): bool).phpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.php (1)
445-458: OAuth2 tables have nullablecreatedtimestamps — inconsistent withfaqapi_keys.In
faqoauthClients(line 456),faqoauthAccessTokens(line 479),faqoauthRefreshTokens(line 493), andfaqoauthAuthCodes(line 509), thecreatedcolumn uses->timestamp('created')which defaults to nullable with no default value. This is inconsistent withfaqapiKeyswherecreatedwas deliberately made NOT NULL with a default via->timestamp('created', false, true).Since rows in these tables should always have a creation timestamp, consider applying the same pattern.
♻️ Suggested fix (example for faqoauthClients, apply similarly to the other three)
- ->timestamp('created') + ->timestamp('created', false, true)Also applies to: 469-483, 485-496, 498-513
phpmyfaq/src/phpMyFAQ/Auth/OAuth2/Entity/ClientEntity.php (1)
31-45: Simplify$secretand$allowedGrantsby removing trivial property hooks.phpMyFAQ 4.2 requires PHP 8.4+, which supports property hooks. However, the get/set hooks on
$secretand the setter on$allowedGrantsonly pass through values without adding behavior. Since these are public properties, the hooks are functionally identical to declaring them without hooks. Consider removing them to simplify the code.Proposed change
- public ?string $secret = null { - get { - return $this->secret; - } - set { - $this->secret = $value; - } - } - - /** `@var` string[] */ - public array $allowedGrants = [] { - set { - $this->allowedGrants = $value; - } - } + public ?string $secret = null; + + /** `@var` string[] */ + public array $allowedGrants = [];phpmyfaq/src/phpMyFAQ/Auth/OAuth2/Repository/ClientRepository.php (1)
61-87: Client validation logic is solid.The layered checks (existence → grant support → confidentiality → secret verification) cover the expected OAuth2 client validation flow. The
password_get_infofallback tohash_equalsfor plain-text secrets is a pragmatic approach for migration scenarios.One minor note:
$client->secretand$client->allowedGrantsare accessed as public properties. IfClientEntityis also new in this PR, consider adding accessor methods for encapsulation consistency.phpmyfaq/src/phpMyFAQ/Auth/OAuth2/ResourceServer.php (1)
107-127: DuplicatedtoPsr7Requestlogic — extract into a shared utility.This method is identical to
AuthorizationServer::toPsr7Request(). Additionally, joining header values withimplode(', ', $values)collapses multi-value headers into a single string, which can subtly break PSR-7 consumers that rely on per-value header arrays (e.g.,Set-Cookie). Consider extracting this into a shared trait or helper that passes header arrays directly:-$headers[$name] = implode(', ', $values); +$headers[$name] = $values;phpmyfaq/src/services.php (1)
191-214: OAuth2 repositories are registered in DI but not injected into the servers that use them.
AuthorizationServer::buildLeagueAuthorizationServer()andResourceServer::authenticate()instantiateClientRepository,AccessTokenRepository,ScopeRepository, etc. directly vianew, bypassing these DI-registered instances entirely. This means there are two separate instances of each repository at runtime — the DI-managed ones and the ones created inline. If you later add caching, logging, or decoration to the DI services, the servers won't benefit.Consider injecting the repositories into
AuthorizationServerandResourceServervia DI instead ofnew-ing them internally.phpmyfaq/src/phpMyFAQ/Auth/OAuth2/AuthorizationServer.php (1)
155-159: Duplicated helpers:isEnabled()andgetConfigString()exist in bothAuthorizationServerandResourceServer.These could live in a shared trait or base class to avoid drift.
Also applies to: 198-202
tests/phpMyFAQ/Auth/OAuth2/ResourceServerTest.php (1)
12-31: Tests cover the basic happy and unhappy paths; consider expanding coverage.Both tests are correct. However, the suite only covers two of the five branches in
authenticate():
- ✅ No Bearer header → null
- ✅ Custom validator → delegated result
- ❌ OAuth2 disabled → null
- ❌ Missing public key → null
- ❌ League server validation (harder to unit test)
The first two additional cases are easy to cover by configuring the mock
Configurationto return appropriate values. Consider adding them to strengthen confidence in the guard clauses.tests/phpMyFAQ/Auth/OAuth2/AuthorizationServerTest.php (1)
48-58: Mock may be too narrow — consider usingwillReturnMapor a broader stub.Line 51 constrains the mock to only match
get('oauth2.enable'). If theAuthorizationServerconstructor orissueTokencalls$configuration->get()with any other key (e.g., checking another config setting first), the mock will fail unexpectedly or returnnullfor those calls. UsingwillReturnMaporwillReturnCallbackfor the mock'sgetmethod would make the test more resilient to implementation changes.
1 participant
Summary by CodeRabbit
New Features
Chores
Tests