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

Add back custom auth #322

Merged
merged 9 commits into from
May 2, 2017
Merged

Add back custom auth #322

merged 9 commits into from
May 2, 2017

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented May 1, 2017

This needs to go through the define_authorizer in order to work
with the swagger generation.

This needs to go through the `define_authorizer` in order to work
with the swagger generation.
@jamesls jamesls requested review from stealthycoin and kyleknap May 1, 2017 20:46
@codecov-io
Copy link

codecov-io commented May 1, 2017

Codecov Report

Merging #322 into master will increase coverage by 0.17%.
The diff coverage is 95.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
+ Coverage   88.95%   89.13%   +0.17%     
==========================================
  Files          18       18              
  Lines        1865     1905      +40     
  Branches      225      231       +6     
==========================================
+ Hits         1659     1698      +39     
  Misses        152      152              
- Partials       54       55       +1
Impacted Files Coverage Δ
chalice/__init__.py 100% <ø> (ø) ⬆️
chalice/deploy/swagger.py 100% <100%> (ø) ⬆️
chalice/app.py 95.13% <92.3%> (-0.31%) ⬇️
chalice/pipeline.py 100% <0%> (+1.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a68aba...1b223a7. Read the comment docs.

auth_config = {
'type': auth_type,
'authorizerUri': authorizer_config['authorizer_uri'],
'authorizerResultTtlInSeconds': 300,
Copy link
Contributor

@stealthycoin stealthycoin May 1, 2017

Choose a reason for hiding this comment

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

Any reason not to expose authorizerResultTtlInSeconds in the config?

@jamesls
Copy link
Member Author

jamesls commented May 1, 2017

Ok, after chatting about this, I'd like to propose some changes here.

First, we should have a consistent pattern for configuring advanced resources. I like the pattern used with the CORSConfig PR: #311, which is you pass in a specific object that represents the configuration you'd like for that route. The define_authorizer relies on string name references. It's nice that it decouples the order of the auth needed, but I think a better option here is to just pass in an Authorizer object, with a different subclass for each type you need. With the current PR, I have the union of args needed for both the cognito user pools auth and custom auth, and would need to add additional validation to get decent error messages (what if you specify both? or you specify neither?). So the new version would look like this:

from chalice.app import CustomAuthorizer
from chalice.app import CognitoUserPoolsAuthorizer
from chalice import Chalice

app = Chalice(app_name='testauth')
custom = CustomAuthorizer('MyAuth', header='Authorization', provider_uri='...')
cognito_auth = CognitoUserPoolsAuthorizer(
    'MyUserPool', header='Authorization', provider_arns=['myarn...'],
)

@app.route('/auth1', authorizer=custom)
def custom_auth(): ...

@app.route('/cognito-auth', authorizer=cognito_auth)
def cognito_auth(): ...

To help transition, we should keep define_authorizer(...), which only works with cognito user pools, for a period of time. We could add a deprecation warning and eventually remove it.

Let me know if anyone has concerns about this.

README.rst Outdated
)
'function:FunctionName/invocations'))

@app.route('/user-pools', methods=['GET'], authorizer=authorizer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a different route name here since the other example was for user pools

chalice/app.py Outdated
raise NotImplementedError("to_swagger")


class CognitoUserPoolAuthorizer(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing you meant to inherit from Authorizer here.

@jamesls
Copy link
Member Author

jamesls commented May 2, 2017

@stealthycoin Feedback incorporated, ready for another look.

These aren't intended to be tested.
@jamesls jamesls merged commit 1b223a7 into aws:master May 2, 2017
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