feat(auth): support an auth management key for disabled auth methods - #135
feat(auth): support an auth management key for disabled auth methods#135itaihanski wants to merge 3 commits into
Conversation
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThis PR adds auth management key support to the PHP SDK configuration. The changes span configuration files (.env.example), documentation (README.md), test infrastructure (phpunit.xml), and core SDK logic (src/SDK/API.php, src/SDK/DescopeSDK.php, and new test file). The review assignment strategy prioritizes contributors with the broadest file coverage and specific expertise in the modified areas: gaokevin1 brings extensive experience across all affected file types including configuration and documentation; dorsha provides targeted expertise in README updates and core API implementation; omercnet offers additional validation on SDK API changes. Together, these reviewers provide comprehensive coverage of configuration management, documentation accuracy, API implementation details, and test quality.
Suggested by Shuni based on git history and PR context. Names are not @-mentioned to avoid notifying anyone — request a review from whoever fits best. |
![shuni-bot[bot]](https://avatars.githubusercontent.com/in/2536750?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Adds an optional authManagementKey that is appended to the bearer token on authentication requests (and kept off management requests), wired from SDK config with docs and a dedicated test file.
Sniffed out 1 issue:
- 1 🟢 LOW: the "never sent on management requests" invariant has one narrow hole for direct
$sdk->apicallers
Good bones — the guard in getAuthToken, the default-to-empty handling, and the auth/management split all check out, and no in-SDK call site can leak the key onto a management endpoint. Woof!
Declared coverage: FULL — 6/6 changed files reviewed.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-reviewed the push (9c19ec2), which reworks getAuthToken so the key is chosen from the management flag instead of an early return, and passes $useManagementKey straight through from doPost/doPatch/doGet.
Sniffed out 0 new issues — and my earlier 🟢 finding is fixed.
I walked every in-SDK call site: the new one-liner is behavior-identical to main for all of them (project:mgmtKey for management, project[:token][:authMgmtKey] for auth), the empty-key and null-flag edges land where the new tests say, and StaticStateIsolationTest's Bearer project:mgmt-key assertion still holds. Nice clean-up — good bones! Woof!
Note: I replied to the prior thread but couldn't resolve it (resolve is denied to my token) — needs a manual resolve. The four unchanged files here were covered in the previous review.
⚠️ Review follow-up issue: Request failed due to following response errors:
- Resource not accessible by integration Not every prior review finding was resolved or marked unfixed.
Declared coverage: PARTIAL — 2/6 changed files reviewed.
1 participant
Related Issues
https://github.com/descope/etc/issues/8683
Description
authManagementKeyto the SDK config, sent with every authentication request so that methods whose public access has been disabled can still be used, similar to the other Descope server SDKsMust