Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PKCE fixes #22

Merged
merged 2 commits into from
Dec 19, 2024
Merged

PKCE fixes #22

merged 2 commits into from
Dec 19, 2024

Conversation

chenz-svsarrazin
Copy link

Fixes required to make PKCE work with Google.

(Disclaimer: I am not an OAuth expert)

PKCE is an *extension* to Authorization-Code-Flow, so the
response_type should still be `code`.
(Disclaimer: I am not an OAuth expert)

PKCE does not preclude transmitting `client_secret` - at least Google
OAuth does *not* work without it and explicity complains about it
missing. This is also consistent with their documentation ([1]).

(As far as I understand, PKCE is really orthogonal to client
authenticiation, it is only meant to protect the Authorization Code.)

This is also consistent with my understanding of RFC 6749, "4.1.3.
Access Token Request" ([2], emphasis added by me):

> If the client type is confidential *OR* the client was issued client
> credentials (or assigned other authentication requirements), the
> client MUST authenticate with the authorization server as described
> in Section 3.2.1.

[1] https://developers.google.com/identity/protocols/oauth2/native-app,
"Step 5: Exchange authorization code for refresh and access tokens"

[2] https://www.rfc-editor.org/rfc/rfc6749#section-4.1.3
@nyalldawson
Copy link

8b4ec70 is definitely required, that was accidentally omitted in the port from the QGIS code.

@elpaso what do you think about ef0c16d?

@nyalldawson nyalldawson reopened this Dec 18, 2024
Copy link

@elpaso elpaso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, recommendations have changed since when PKCE was initially implemented and it is now recommended in addition to the code verifier also when client secret is used from confidential clients.

@nyalldawson nyalldawson merged commit 5c78a58 into qgis:main Dec 19, 2024
14 of 15 checks passed
@nyalldawson
Copy link

Thanks @elpaso!

@chenz-svsarrazin chenz-svsarrazin deleted the pkce-fixes branch December 19, 2024 09:03
@chenz-svsarrazin
Copy link
Author

Note also that client_secret is transmitted unconditionally on the token refresh requests, which should follow the same rules wrt client authentication as the initial token request as far as I can tell from the RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants