feat(bfabric): expose the authorization-code primitives for confidential clients - #628
Draft
leoschwarz wants to merge 5 commits into
Draft
feat(bfabric): expose the authorization-code primitives for confidential clients#628leoschwarz wants to merge 5 commits into
leoschwarz wants to merge 5 commits into
Conversation
…ial clients A web app driving its own authorization-code login cannot use pkce_login: its callback lands on a registered public redirect URI, not the loopback listener pkce_login starts. Everything upstream of that listener is reusable, so make the final leg public rather than have each app reimplement it. Only the parts that can actually drift are exported. The authorize parameters (RFC 6749 4.1.1, RFC 7636 4.3) and the token request body (4.1.3, 4.5) are fixed by spec and are left for callers to build; what is B-Fabric's own are the two endpoint paths, and what no spec pins is which client authentication method the token endpoint accepts, so client_secret follows the client_secret_basic form already used for the RFC 8693 exchange. An empty client_secret is treated as absent, because that is how the rest of the library spells "public client". The token path was built at eight call sites; four of them are in bfabric.py and _webapp_client.py, which #621 rewrites, so those are left for a later sweep.
…dpoint authorize_url returned the bare endpoint, leaving every caller to append the same seven query parameters. Those parameters are fixed by RFC 6749 4.1.1 and RFC 7636 4.3, so no copy of that block can drift -- but two copies had already appeared, and exporting the token exchange while leaving half the redirect leg to the caller is an odd surface for a library that claims to offer the flow. Fold the query into authorize_url rather than adding a second function beside it: once the full builder exists the bare endpoint has no caller left, so this keeps the exported surface at three names and avoids a confusing near-synonym.
authorize_url asked the caller for a code_challenge and a state, both of which pkce_login generates three lines above the call. So the exported half of the flow handed back the hard part: a consumer had to implement RFC 7636 4.1 and 4.2 and base64url-without-padding to use functions whose purpose is hiding exactly that, and a mistake surfaces only at the token step, one redirect later, as an opaque server rejection. AuthorizationRequest.create generates the verifier, derives the challenge and picks the CSRF state, returning the redirect URL plus the two values the caller has to carry across the round trip. Nothing PKCE-shaped is left in the public vocabulary and the challenge cannot disagree with the verifier. authorize_url stops being exported, since the arguments that made it public are the ones now generated. token_url stays: OAuthCredentialProvider takes a token_url, so an external caller building one still needs it. Giving the provider a base_url instead would change the field it pickles, so that is left alone. Also export verify_jwt, already used by WebappClient and already caching JWKS for an hour, so a caller stops hand-rolling base64url to read a token's claims.
basedpyright runs against a baseline that grandfathers the existing reportUnannotatedClassAttribute warnings, so a new one fails CI even though 45 like it already exist. Annotate with the ClassVar[ConfigDict] that pydantic itself declares, which satisfies the rule without the reportIncompatible- VariableOverride an unparameterised ConfigDict annotation would provoke.
It was exported on the strength of a consumer that then turned out not to want it: the log-viewer webapp learns its instance from the token's own iss, so it cannot name a base_url before decoding, and verify_jwt takes one. With no external caller left, exporting it only fixes a name we would have to keep. Nothing else changes — WebappClient still imports it from _url_token, so it stays covered by tests. Re-exporting is one line in __all__ if a webapp serving a single configured instance ever shows up; at that point it should also take a str and normalise, like its exported siblings do.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
bfabric.oauth.AuthorizationRequest.create, which starts an authorization-code login: it returns the URL to redirect a user to, plus the CSRF state and PKCE verifier to keep until they come back. A web app no longer derives a code challenge itself, so it cannot produce one that disagrees with its verifier.bfabric.oauth.exchange_codeto redeem the returned code for tokens, with an optionalclient_secretsent asclient_secret_basicfor confidential clients. Omitting it makes the request as a public client, relying on PKCE alone.bfabric.oauth.token_url, which builds the token endpoint URL for an instance.client_secret_basicon theauthorization_codegrant specifically: Basic is proven for the RFC 8693 exchange, and the POST-body form is proven forauthorization_code. Needs one live login against a real instance before this merges.