-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add PKCE to user authentication #189
Comments
This raises a problem where each generated code challenge must be matched with a verifier it was generated with. I see these approaches.
This needs to be thought out. Not having an ability to use whatever instance of class Auth:
client_id: str
client_secret: str = None
redirect_uri: str = None
def request_client_token() -> Token
def make_user_flow(state: str = None, verifier_bytes: int = 32) -> AuthorisationCodeFlow
# Verifier bytes in minimum 32, max result clipped to 128 characters
class AuthorisationCodeFlow:
client_id: str
redirect_uri: str
code_verifier: str
challenge_method: str
def authorisation_url(scope, show_dialog) -> str
def request_token(code: str, state_check: str = None) -> str I'm not sure about the names, but that's the gist of what I have in mind. The usage could look like this: auth = tk.Auth(client_id, client_secret, redirect_uri)
flow = auth.make_user_flow(state)
url = flow.authorisation_url(scope, show_dialog)
code, state_check = ... # Redirect and get code and state
token = flow.request_token(code, state_check) It's pretty similar, with one added line instantiating the flow. This is at least better than using the state or having some hacky previous value for verifiers. Additionally, it would allow for automatically checking state, which should also require Maybe we won't implement this in Tekore 2 just yet. It would introduce too many problems without these changes. |
Of course this can and should be implemented as an enhancement and deprecation of the original functionality, provided that the names don't clash. |
With the non-inclusion of Implicit Grant Flow, having one flow object could also be overkill. PKCE changes the refreshing of tokens too, so the refresh method needs to be changed anyway. Not using flow objects would allow for simpler changes. cred = tk.Credentials(client_id, client_secret, redirect_uri)
url, verifier = cred.user_authorisation_url(scope, state, verifier_bytes)
code, state_check = ... # Redirect and get code and state
if state == state_check:
token = cred.request_user_token(code, verifier)
else:
raise SomeError('Important message.') This wouldn't allow for automatically generating state, but a utility with good examples might be sufficient for that. NOTEThe new refreshing method does not work for tokens generated with the previous method. Users should be informed of this. |
Okay, so the Spotify authorisation guide states at the very end of its PKCE section, that its refresh token may be used only once to get a new access token, after which it is invalidated. So this may not be a replacement to the regular user auth flow. The refresh tokens can be chained and tokens used for their full duration, but this makes testing a lot harder. I opened a discussion on Spotify forums. |
Given no quick response from Spotify, we should implement PKCE as an option. So we ought to choose some names.
These are reasonably similar while also highlighting the fact that PKCE introduces special semantics. Sadly this addition makes it impossible to distinguish between ordinary user refresh tokens and PKCE versions. This affects two things: I would lean towards removing the type-agnostic refresh method, if the change didn't affect self-refreshing tokens too. They need a way to tell if they were created with PKCE or not, so let's introduce some boolean to all tokens while were at it. |
In the end, this doesn't require changes. We can naturally use the new boolean for tokens in deciding how to refresh them. |
Closed in #193 |
Spotify has announced support for the PKCE authentication method (guide). We should implement it. It seems that Spotify has documented it as optional, but reading the RFC, it is presented as an extension that should be used instead of the original one. So I think we won't provide any parameters to switch it off.
We should also consider the Implicit Grant flow and could it be useful. I think Spotify's disclaimer of it being a JS-only method put me off until now when I gave it more thought.It seems that the need for this does not exist, as it cannot be used on a server, and PKCE is the preferred method for authentication that does not use a client secret.Let's also add better documentation pulling all these authentication methods together.
The text was updated successfully, but these errors were encountered: