feat(auth): support an auth management key for disabled auth methods - #230
feat(auth): support an auth management key for disabled auth methods#230itaihanski wants to merge 2 commits into
Conversation
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThis PR introduces an authentication management key feature with significant refactoring of the HTTP client layer to support dual-key instantiation (mirroring Python and Go SDKs). The strategy prioritizes reviewers with: (1) broad experience across the management API surface area, (2) familiarity with the core HTTP client architecture being modified, and (3) exposure to documentation changes. ami-descope is selected as the primary reviewer due to extensive commits across tooling and documentation files, while dorsha provides deep coverage of the management API endpoints that now depend on the refactored HTTP client. guyp-descope adds focused expertise on the access_key management module directly impacted by auth management changes.
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. |
There was a problem hiding this comment.
🐕 Shuni's Review
Splits the HTTP layer into two clients so authentication requests carry the new auth_management_key while management requests keep the management key. The mgmt_* rename is complete and consistent across all 24 management modules and their specs — no bare verbs left behind. Good bones!
Sniffed out 2 issues:
- 1 🔴 CRITICAL:
merge(nil)onextra_headersbreaksenchanted_link_sign_in - 1 🟢 LOW: new spec deletes/leaks
DESCOPE_*_KEYenv vars for the rest of the suite
Heads-up (not a finding): now that timeout_seconds is actually applied, long export_project/import_project calls hit the 60s default unless overridden.
See inline comments for details. Woof!
Declared coverage: FULL — 59/59 changed files reviewed.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-reviewed the delta since 150fa57 (5 files). Both prior findings are fixed and their threads are resolved: extra_headers is now guarded with || {}, the enchanted-link caller passes {}, and the spec's around hook snapshots and restores the env vars in ensure.
The two new regression examples drive a real client through the nil argument and enchanted-link sign-in — exactly the DummyClass blind spot — and the new README timeout section matches DEFAULT_TIMEOUT_SECONDS = 60.
No new issues sniffed out. Clean bill of health — good boy! Woof!
Declared coverage: PARTIAL — 5/61 changed files reviewed.
Related Issues
https://github.com/descope/etc/issues/8683
Description
auth_management_keyto the client config, resolved fromDESCOPE_AUTH_MANAGEMENT_KEYwhen not set explicitly, and sent with every authenticationrequest so methods with disabled public access can be used
Descope::HttpClientand instantiate it twice - one client perkey - mirroring the approach taken in the Python and Go SDKs. Previously all requests went
through a single receiver-less verb on one object, so there was no point at which the two keys
could be told apart. Management modules now call
mgmt_prefixed verbs, making the key aproperty of the call site rather than something inferred from the request
Follow-ons from that refactor:
@default_pswdfallbackpreviously sent it on every authentication request that presented no token, which no other
Descope SDK does. Callers relying on that must now set
auth_management_keyAuthorizationheader is built per request rather than merged into a shared@headershash, so concurrent requests on one client cannot send each other's credentials
timeout_secondsis now applied - it was never assigned, leaving every request without atimeout
Authorizationheader,replacing a mask that only covered the last 10 characters
extra_headersis passed onPOST,PATCHandPUT, whichrequestwas droppingpost_file,post_formanddelete_with_body, which passed an invalid verb toRestClientand had no call sites.rubocop.ymland.rubocop_todo.ymlfor the installed RuboCop, which could not loadits config on
mainMust