Exchanging an authorization code for an access token needs no client_secret

[tdg5]

It could be that I'm doing something wrong, but if I attempt to exchange an authorization code for an access token and I don't supply some kind of dummy client_secret, then I get an InvalidClientError. If I supply a blank client_secret, I don't have a problem.

Relevant stacktrace is:

  File ".../aioauth/server.py", line 256, in create_token_response
    client_id, client_secret = self.get_client_credentials(request)
  File ".../aioauth/server.py", line 216, in get_client_credentials
    raise InvalidClientError[TRequest](

I think the code I've suggested solves the problem, badly, so I'm open to other ideas. I don't love that it hardcodes a check for request.post.grant_type != "authorization_code" because it doesn't solve the problem for other grant types and makes the library harder to extend with custom grant types, but that may not even be possible currently based on adjacent logic that has some strong opinions about the possible set of GrantTypes:

GrantTypeClass: Type[
    Union[
        GrantTypeBase[TRequest, TStorage],
        AuthorizationCodeGrantType[TRequest, TStorage],
        PasswordGrantType[TRequest, TStorage],
        RefreshTokenGrantType[TRequest, TStorage],
        ClientCredentialsGrantType[TRequest, TStorage],
    ]
]

Which is all to say that maybe it's not a problem to hardcode a check of request.post.grant_type != "authorization_code".

Thinking it over some more, I think this needs to be a bigger change because I think this only would fix the issue for the authorization_code grant type and it suggests that here are a some broken tests that need fixing. I'm happy to help with that, but would appreciate some confirmation that I'm not crazy first.

[tdg5]

Still trying to wrap my head around what is expected here... The RFC says:

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. https://www.rfc-editor.org/rfc/rfc6749#section-4.1.3

So I guess one important thing to callout there is the if. I don't believe there is any requirement that a client must be issued client credentials, so a client that is allowed to use only the authorization_code grant type should not need to provide client_secret as part of a token request. The existence of browser-based OAuth flows seems like it should make this point obvious because a browser has no privacy with which to utilize a client_secret.

All this certainly argues that the code I've proposed needs work. Looking down that road, it argues to me that Client.client_secret should be Optional[str] instead of str (though, frankly, I think it really ought to be List[str], but that's a different discussion, I'd think), and I imagine things get messier from there.

Let me know how you would like to proceed and we can take it from there 😀

[tdg5]

One of the implementation options from the RFC related to Browser Based Apps is specific in pointing out that no secret is required or possible for the given implementation option:

This application is considered a public client, since there is no way to issue it a client secret in this model.

The code in the browser initiates the Authorization Code flow with the PKCE extension (described in Section 7) (B) above, and obtains an access token via a POST request (C). https://datatracker.ietf.org/doc/html/draft-ietf-oauth-browser-based-apps#section-6.4-3

[tdg5]

I made this other branch that takes a more robust approach to solving the problem, but was struggling to fix the test failure it introduces: https://github.com/aliev/aioauth/compare/master...tdg5:aioauth:dannyg/other-approach?expand=1

If something more like that is preferable, I can update this PR with the code from that branch.

Specifically, I was struggling to get tests/test_flow.py:368 test_client_credentials_flow_post_data to pass because it seems the tests expect all requests to work the same as far as client_secret is concerned and the change in that branch makes it so a missing client_secret can still result in a successful request.

[aliev]

According to RFC documentation and auth0 specification you're completely right. The client_secret is required only for two grant types: password and client_credentials:

https://auth0.com/docs/get-started/authentication-and-authorization-flow/call-your-api-using-resource-owner-password-flow#example-post-to-token-url https://auth0.com/docs/get-started/authentication-and-authorization-flow/call-your-api-using-the-client-credentials-flow#example-post-to-token-url https://www.rfc-editor.org/rfc/rfc6749#section-2.3.1

nice catch! 🌟

[aliev]

I made this other branch that takes a more robust approach to solving the problem, but was struggling to fix the test failure it introduces: https://github.com/aliev/aioauth/compare/master...tdg5:aioauth:dannyg/other-approach?expand=1

If something more like that is preferable, I can update this PR with the code from that branch.

Specifically, I was struggling to get tests/test_flow.py:368 test_client_credentials_flow_post_data to pass because it seems the tests expect all requests to work the same as far as client_secret is concerned and the change in that branch makes it so a missing client_secret can still result in a successful request.

I think the first implementation with if condition was good, it's more explicit and can be easily documented with resources:

        client_secret: Optional[str] = None

        if request.post.grant_type in ("client_credentials", "password", ):
            # client_secret is only expected for client_credentials, password grant types
            # https://www.oauth.com/oauth2-servers/access-tokens/client-credentials/
            # https://www.oauth.com/oauth2-servers/access-tokens/password-grant/
            client_id, client_secret = self.get_client_credentials(request)
        else:
            client_id = request.post.client_id

[tdg5]

@alilev I think this is ready for another look

[tdg5]

@aliev sorry about that, I've made the corrections you requested :+1:

[codecov-commenter]

Codecov Report

Merging #74 (2437acf) into master (72046ee) will decrease coverage by 0.60%. The diff coverage is 63.63%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
- Coverage   99.38%   98.78%   -0.60%     
==========================================
  Files          14       14              
  Lines         649      658       +9     
  Branches       96       99       +3     
==========================================
+ Hits          645      650       +5     
- Misses          2        4       +2     
- Partials        2        4       +2     

Impacted Files	Coverage Δ
aioauth/grant_type.py	93.02% <42.85%> (-4.48%)	:arrow_down:
aioauth/server.py	100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[aliev]

@tdg5 I created new release based on these changes.

[tdg5]

Thanks @aliev! I apologize in advance for fretting over decisions made, but do you think we made the right choice for handling the password grant type? The spec seems pretty clear that client_secret is not a requirement for the password grant:

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.

More from the spec about the design intent of the password grant type further supports the notion that a client_secret should be optional, otherwise some of the migrations they mention wouldn't be possible because a client_secret couldn't be properly secured:

This grant type is suitable for clients capable of obtaining the resource owner's credentials (username and password, typically using an interactive form). It is also used to migrate existing clients using direct authentication schemes such as HTTP Basic or Digest authentication to OAuth by converting the stored credentials to an access token. https://www.rfc-editor.org/rfc/rfc6749#section-4.3

To be clear, I don't love the password grant and I am aware that:

1. Current best practices recommend not using the password grant type at all
2. The password grant type is not included in the OAuth 2.1 spec

but, that said, I could see the password grant (without requiring a client_secret) being useful for adoption of this library since it enables migrating less secure flows in the direction of more secure flows.

I guess that is all to say, I'm fine to neglect the password grant type since its days are numbered, but I'm willing to put some more time into making client_secret optional for the password grant type if you're open to that change.

[aliev]

Thanks @aliev! I apologize in advance for fretting over decisions made, but do you think we made the right choice for handling the password grant type? The spec seems pretty clear that client_secret is not a requirement for the password grant:

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.

More from the spec about the design intent of the password grant type further supports the notion that a client_secret should be optional, otherwise some of the migrations they mention wouldn't be possible because a client_secret couldn't be properly secured:

This grant type is suitable for clients capable of obtaining the resource owner's credentials (username and password, typically using an interactive form). It is also used to migrate existing clients using direct authentication schemes such as HTTP Basic or Digest authentication to OAuth by converting the stored credentials to an access token. https://www.rfc-editor.org/rfc/rfc6749#section-4.3

To be clear, I don't love the password grant and I am aware that:

1. Current best practices recommend not using the password grant type at all
2. The password grant type is not included in the OAuth 2.1 spec

but, that said, I could see the password grant (without requiring a client_secret) being useful for adoption of this library since it enables migrating less secure flows in the direction of more secure flows.

I guess that is all to say, I'm fine to neglect the password grant type since its days are numbered, but I'm willing to put some more time into making client_secret optional for the password grant type if you're open to that change.

ah you're right, sorry for my inattention. let's make client_secret optional then!