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

allow multiple authorizers by specifying name and arn #3357

Closed
wants to merge 3 commits into from

Conversation

keith301
Copy link

@keith301 keith301 commented Mar 13, 2017

What did you implement:

Ran into issue specifying multiple authorizers (by arn) because the authorizer names were similar (both ended in the same -suffix)

change Allow for multiple authorizers (with similar names) to be specified by arn by allowing to name each authorizer, e.g

authorizer:
   arn: arn:aws:...
   name: authorizer1

authorizer:
   arn: arn:aws:...
   name: authorizer2

How did you implement it:

Check to see if name is specified along with arn. if so, favor specified name over generated name.

How can we verify it:

Create two authorizer functions with similar suffixed names, e.g. auth1-dev and auth2-dev
create two functions authorized by each authorizer, eg.

functions:
  function1:
      handler: handler.function1
      events:
         - http:
             path: /one
             authorizer:
                 arn:  arn:...:auth1-dev
  function2:
      handler: handler.function1
      events:
         - http:
             path: /one
             authorizer:
                 arn:  arn:...:auth2-dev

The resulting template/api gateway will have only a single authorizer named 'dev' used by both functions, which is not the intended behavior.

Now adding 'name' to the authorizer naming each one uniquely will result in two authorizers being created and used by the functions, as intended:

functions:
  function1:
      handler: handler.function1
      events:
         - http:
             path: /one
             authorizer:
                 arn:  arn:...:auth1-dev
                 name: auth1
  function2:
      handler: handler.function1
      events:
         - http:
             path: /one
             authorizer:
                 arn:  arn:...:auth2-dev
                 name: auth2

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens
Copy link
Contributor

pmuens commented Mar 14, 2017

Nice! Thanks for the addition! 👍

Could you please fix the linting error (line is too long) so that the build passes. Thanks!

Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Looks good from a code perspective. 👍

Just needs a quick linting fix (seems like the line is too long) and a test.

@HyperBrain
Copy link
Member

HyperBrain commented Mar 28, 2017

Just created #3413 which seems to be the problem.
I mentioned some examples that I experienced there.

@kminkler Your fix should also respect the aliased arns and disambiguate them properly. Additionally all other ways to declare authorizers should lead to unambiguous names too.

@@ -202,7 +202,7 @@ module.exports = {
} else if (typeof authorizer === 'object') {
if (authorizer.arn) {
arn = authorizer.arn;
name = this.provider.naming.extractAuthorizerNameFromArn(arn);
name = typeof authorizer.name == 'string' ? authorizer.name : this.provider.naming.extractAuthorizerNameFromArn(arn);
Copy link
Member

Choose a reason for hiding this comment

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

Either use '===' or _.isString(authorizer.name)

@@ -202,7 +202,7 @@ module.exports = {
} else if (typeof authorizer === 'object') {
if (authorizer.arn) {
arn = authorizer.arn;
name = this.provider.naming.extractAuthorizerNameFromArn(arn);
name = typeof authorizer.name == 'string' ? authorizer.name : this.provider.naming.extractAuthorizerNameFromArn(arn);
Copy link
Member

Choose a reason for hiding this comment

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

There should also be fixes in extractAuthorizerNameFromArn() as the malicious name generation happens there in the first place.
I think only disambiguating manually is not the complete solution.

@pmuens pmuens added this to the 1.17 milestone Jun 21, 2017
@pmuens pmuens removed this from the 1.17 milestone Jun 29, 2017
@thomasmichaelwallace
Copy link
Contributor

I was just about to put together a PR to fix this issue myself. Is there anything I can do to help get this through faster?

@horike37
Copy link
Member

Closing this issue for now since it took for a long time without any activity.
Please ping us if you want to continue working on this!

@horike37 horike37 closed this Jan 10, 2018
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.

None yet

6 participants