mblackgeo / flask-cognito-lib

A Flask extension that supports protecting routes with AWS Cognito following OAuth 2.1 best practices
https://mblackgeo.github.io/flask-cognito-lib/
MIT License
57 stars 15 forks source link

Support not using app secret #17

Closed jak-sdk closed 1 year ago

jak-sdk commented 1 year ago

Hi! Thanks for this library, it's ultra useful! I've hit a snag with one of our apps though:

When creating a Public Cognito app, you can opt not to use a Client Secret.

The Authorization header is an optional way to pass the App Client ID and App Client Secret, but if the Secret isn't enabled for the given app client, then the Authorization header stops being optional, and becomes forbidden...

I've done some testing and found that there's no valid Authorization header for this use case, and you have to just avoid setting it at all, otherwise Cognito returns

Code: 400
Body: { "error": "invalid_client" }

Some other people have found this too and discussed here: https://stackoverflow.com/questions/54578397/token-endpoint-returns-invalid-client-without-client-secret

The intention of this pull request is to not set the Authorization header if user_pool_client_secret is '' or None

Please let me know if this is a valid approach, Thanks!

mblackgeo commented 1 year ago

Thanks for the PR. I hadn't considered the case of making a Public Client for the User Pool since Flask is a server side application (as opposed to say, a single page application like React which wouldn't be able to store a client secret securely). IMO I'd always make the app a Confidential Client and set the Client Secret in the Flask environment for extra security, though in theory there is no reason why a Public Client should not be supported.

Just for my own interest, could you elaborate on why you are using a Public Client?

jak-sdk commented 1 year ago

IMO I'd always make the app a Confidential Client and set the Client Secret in the Flask environment for extra security

I completely agree, unfortunately in this case my justification is to invoke the dreaded 'legacy application'. I came across this while building a small admin frontend for an existing API that was setup by a predecessor of a predecessor, which was setup as a public client (I'm not convinced it should have been).

Just for my own interest, could you elaborate on why you are using a Public Client?

My understanding is that this Flask application needs to authenticate against the same Public Client as the API as it uses the users token to make API requests on their behalf?

codecov-commenter commented 1 year ago

Codecov Report

Merging #17 (266e233) into main (c493f07) will not change coverage. The diff coverage is 100.00%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##              main       #17   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          304       307    +3     
=========================================
+ Hits           304       307    +3     
Impacted Files Coverage Δ
src/flask_cognito_lib/config.py 100.00% <100.00%> (ø)
src/flask_cognito_lib/services/cognito_svc.py 100.00% <100.00%> (ø)
mblackgeo commented 1 year ago

LGTM, thanks!

mblackgeo commented 1 year ago

Should now be available in version 1.4.0 :tada: