jupyterhub / oauthenticator

OAuth + JupyterHub Authenticator = OAuthenticator
https://oauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
408 stars 362 forks source link

[Bitbucket] Rework required #677

Open consideRatio opened 1 year ago

consideRatio commented 1 year ago

676 reported an issue, but it untangled to multiple issues that needed to be resolved so I've now opened this issue to summarize the findings. I think a proper rework of BitBucketOAuthenticator is required.

jyio commented 10 months ago

I recently ran into the Authorization: Bearer issue while trying to integrate GenericOAuthenticator with Kanidm, and we'd likely hit the same snag with any OAuth2 IdP that doesn't follow Postel's law.

RFC 6749 says token_type is case-insensitive in the access token response. RFC 6750 says Bearer should be capitalized in the Authorization request header. This has caused many headaches related to inconsistent case usage. Bitbucket and Kanidm are technically RFC-compliant by sending bearer and expecting Bearer; but I believe it would be more consistent and just as compliant to send Bearer. On the other hand, OAuthenticator is non-compliant by receiving bearer and sending bearer; and the fix is likely simple:

Option 1: Hard-code Bearer, as recommended by this user and you. It would work for most cases, but miss the unusual IdP that wants to use another scheme such as Basic, Digest, or HOBA.

Option 2: Title-case token_type as token_type.lower().title(), which is how I fixed the Kanidm integration. It would work for Basic, Digest, and Bearer schemes but mess up HOBA. If we wanted to do this, we might need to special-case HOBA.

Option 3: Rewrite any case of bearer to Bearer; e.g. 'Bearer' if token_type.lower() == 'bearer' else token_type. This would selectively fix the common case of Bearer authentication while keeping the current behavior of mirroring the IdP if the IdP wanted to try something else. Probably the most sensible approach.

Option 4: Option 3, but also for Basic, Digest, HOBA, etc... falling back to current behavior if an unknown scheme is used. A dictionary could help, e.g. {x.lower(): x for x in ('Basic', 'Digest', 'Bearer', 'HOBA')}[token_type.lower()]. Most comprehensive, but probably overkill.

Also, which file should be patched? Even though I use GenericOAuthenticator, the error was raised in oauth2.py so that's the file I edited. We'd probably have to factor out this auth-scheme normalizing code so it could be shared by multiple authenticators...

consideRatio commented 10 months ago

@jyio this was a fantastic summary!! I agree os option Option 3 as a sensible approach, and that was what @yaleman did in #698 referencing back here - thank you both!! :heart: :tada: :sunflower:

consideRatio commented 10 months ago

Also, which file should be patched? Even though I use GenericOAuthenticator, the error was raised in oauth2.py so that's the file I edited. We'd probably have to factor out this auth-scheme normalizing code so it could be shared by multiple authenticators...

The things in oauth2.py is defining the OAuthenticator base class common for all authenticators, so updating it there was perfect - at least now that this thorough research establishes it should be the right call for all authenticators!