Enable API Gateway authorizers #133

vlewin · 2018-07-14T15:01:26Z

https://serverless.com/framework/docs/providers/aws/events/apigateway/#http-endpoints-with-aws-iam-authorizers

vlewin · 2018-07-14T15:03:03Z

This PR is based on #131

vlewin · 2018-07-14T19:54:03Z

@horike37 Hello Takahiro, is there a reason why not to use a serverless internal libraries instead of copying files from serverless source?

horike37 · 2018-07-17T16:00:11Z

Hey @vlewin !
Thank you for implementing this feature! Really looking forward to seeing this will be live as well as #131!!
Let me know if you will be ready for review 😄

is there a reason why not to use a serverless internal libraries instead of copying files from serverless source?

Good point 👍
sls internal libraries around events have not been considered as libraries which referenced from external plugins.
Of course, I understanding the best way that using those in terms of keeping codebase clean. However, If I use it, this plugin might be broken when releasing the serverless framework new version.

That being said, I have decided to get copying way for keeping it safe.

vlewin · 2018-07-17T16:46:36Z

@horike37 Thank you for quick response. Maybe we can pin tested and supported version of serverless in package.json and update it once a new version is released. So we can test your plugin and if all green release it. As you can see from my PR serverless is under active development and this results in a lot of changes per minor version. I already tested your plugin with the libraries from lastest serverless version and did't detected any issues.

horike37

Hi @vlewin, thank you for the effort and implementing awesome features 👍 💯
Looks great and can't wait the next release!!

I just reviewed this PR, it seems that this PR include the following features not just authorizer.

maxAge for cors
endpointType configuration
share API
resourcePolicy configuration

You might want to separate PR for each feature, otherwise, I can't review and test properly with narrow ing it down. The main point of this PR should authorizer feature.

Also, Please add the explanation of those to README so that Let users knows those awesome features!

Thanks in advance!

horike37 · 2018-07-20T22:11:12Z

CHANGELOG.md

@@ -1,6 +1,10 @@
+# 1.5.1(10.07.2018)


Could you delete it once?
This should be added while releasing a new version.

horike37 · 2018-07-20T22:14:27Z

lib/deploy/events/apiGateway/cors.js

@@ -24,6 +25,16 @@ module.exports = {
        'Access-Control-Allow-Credentials': `'${config.allowCredentials}'`,
      };

+      // Enable CORS Max Age usage if set
+      if (_.has(config, 'maxAge')) {


maxAge feature is great!
However, does this need for authorizer?
If not, would be great if you can separate maxAge PR from this one.

horike37 · 2018-07-20T22:19:27Z

lib/deploy/events/apiGateway/resources.js

@@ -6,65 +6,247 @@ const _ = require('lodash');
 module.exports = {


Are those changes of this file for share API feature?
If so, could you remove those from this PR?

horike37 · 2018-07-20T22:20:20Z

lib/deploy/events/apiGateway/resources.test.js

@@ -70,15 +70,15 @@ describe('#compileResources()', () => {
        },


Same here. maybe those are for share API.

horike37 · 2018-07-20T22:24:43Z

lib/deploy/events/apiGateway/restApi.js

+    let endpointType = 'EDGE';
+
+    if (this.serverless.service.provider.endpointType) {
+      const validEndpointTypes = ['REGIONAL', 'EDGE', 'PRIVATE'];


Selecting endpointType is great!
However, those are not for authorizer feature.
Could you separate PR for each feature?

horike37 · 2018-07-20T22:26:12Z

lib/deploy/events/apiGateway/restApi.js

        },
      },
    });

+    if (!_.isEmpty(this.serverless.service.provider.resourcePolicy)) {


Same here. this is resourcePolicy feature.

horike37 · 2018-07-20T22:27:50Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "serverless-step-functions",
-  "version": "1.5.0",
+  "version": "1.6.0",


Could you turn back to 1.5.0?
This should be updated while releasing a new version.

vlewin · 2018-07-21T07:38:45Z

Hi @horike37 thank you for review! You are right this PR includes changes from #131. Please review #131 Shared API Gateway pull request first.

Once this #131 is merged to master we will see only the changes for authorizers. If you want to add authorizers support first I can open a separate pull request but our team need both changes in the next version. Otherwise we have to use my fork what I want to avoid.

I'm sorry for the confusion.

horike37 · 2018-07-25T19:18:48Z

Hi @vlewin
Could you rebase this branch from the top of master so that we can see only the changes of authorizer feature? #131 was just merged

vlewin · 2018-07-25T19:35:19Z

@horike37 JFYI: branch is rebased, now we should see authorizer related changes only

vlewin · 2018-07-25T19:39:36Z

Should we start using serverless internal libraries?
https://github.com/horike37/serverless-step-functions/pull/133/files#diff-6d186b954a58d5bb740f73d84fe39073R12

If yes we have to rename/reassign a property:
https://github.com/horike37/serverless-step-functions/pull/133/files#diff-6d186b954a58d5bb740f73d84fe39073R110

horike37

@vlewin

Should we start using serverless internal libraries?

Either's fine. If using those, we should move Serverless to dependencies from devDependencies.
Could you add documentation to README as well?

horike37 · 2018-07-25T20:29:45Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "serverless-step-functions",
-  "version": "1.5.1",
+  "version": "1.5.0",


Please revert it

horike37 · 2018-07-29T03:03:29Z

Hi @vlewin, thank you for fixing!
I tested locally but does not work with the following error.

$ sls deploy -v
Serverless: Packaging service...
Serverless: Excluding development dependencies...
 
  Type Error ---------------------------------------------
 
  Cannot read property 'replace' of undefined
 
     For debugging logs, run again after setting the "SLS_DEBUG=*" environment variable.
 
  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues
     Issues:        forum.serverless.com
 
  Your Environment Information -----------------------------
     OS:                     darwin
     Node Version:           8.4.0
     Serverless Version:     1.29.1

Here is my serverless.yml file.

service: step-func

provider:
  name: aws
  runtime: nodejs6.10

functions:
  hello:
    handler: handler.hello
  authorizerFunc:
    handler: authorizer.js

stepFunctions:
  stateMachines:
    step-func:
      events:
        - http:
            path: gofunction
            method: GET
            authorizer: authorizerFunc
      name: my-${opt:stage}-statemachine
      definition:
        StartAt: Ingest
        States:
          Ingest:
            Type: Task
            Resource: arn:aws:lambda:#{AWS::Region}:#{AWS::AccountId}:function:${self:service}-${opt:stage}-hello
            End: true


plugins:
  - serverless-pseudo-parameters
  - serverless-step-functions

Any ideas why?

vlewin · 2018-07-29T13:14:49Z

@horike37 Thank you very much for testing. I extended authorizer support, previously it was only possible with Shared authorizers now it should work with any authorizer type e.g. AWS_AIM, Congnito or custom.

Here is my serverless.yml:

service: sfn-api-gateway-playground

provider:
  name: aws
  runtime: nodejs8.10
  region: eu-central-1

  apiGateway: # Optional API Gateway global config
    restApiId: n42gvt3whk # REST API resource ID. Default is generated by the framework
    restApiRootResourceId: 9f8qmgn5d6 # Root resource ID, represent as / path
    restApiResources: # List of existing resources that were created in the REST API. This is required or the stack will be conflicted
      '/playground': skvi34

functions:
  customAuthorizer:
      handler: authorizer.handler

stepFunctions:
  stateMachines:
    SfnApiGateway:
      events:
        - http:
            path: /playground/step
            method: GET
            authorizer: customAuthorizer
            # authorizer:
            #   type: CUSTOM
            #   authorizerId: 43mrom
            cors:
              origin: '*'
              maxAge: 86400
      definition:
        Comment: "A sample application"
        StartAt: Start
        States:
          Start:
            Type: Pass
            End: true
plugins:
  - serverless-step-functions
  - serverless-pseudo-parameters

horike37 · 2018-07-30T15:30:50Z

@vlewin
Thank you for the update!
LGTM 👍 💯 I will release a new version soon!
Awesome work!!!!

vlewin · 2018-07-30T18:18:30Z

@horike37 Awesome! Thank you very much for review!

…er_support Enable API Gateway authorizers

vlewin added 9 commits July 11, 2018 20:34

Enable HTTP endpoints authorizers

0832918

Bump package version

8eb4478

Use internal serverless libraries instead of clones???

af774da

WIP: add getMethodAuthorization method

ebaf2fd

Finilize authorizers enablement

6eb4940

Add serverless dependency to package.json

32ada3b

Merge branch 'master' into add_authorizer_support

6735820

Remove authorizer files

4b35afd

Add tests for getMethodAuthorization

84f6d2e

Fix coding standard violations

358e205

horike37 added this to the 1.6 milestone Jul 17, 2018

horike37 requested changes Jul 20, 2018

View reviewed changes

vlewin force-pushed the add_authorizer_support branch from 7235233 to 358e205 Compare July 25, 2018 19:31

Update package.json

100d249

horike37 requested changes Jul 25, 2018

View reviewed changes

vlewin and others added 3 commits July 28, 2018 09:40

Revert plugin version change

003f3e2

Add documentation for IAM and custom authorizers

154348e

Merge branch 'master' into add_authorizer_support

d8f1463

vlewin added 3 commits July 29, 2018 14:49

Add support for AWS_AIM, Congnito and custom authorizer

a49d907

Add a note about usage of shared authorizers

67134bc

Updated package-lock.json

cb8d515

horike37 approved these changes Jul 30, 2018

View reviewed changes

horike37 merged commit be46b6d into serverless-operations:master Jul 30, 2018

ss-betseqnzr pushed a commit to BetSEQNZR/serverless-step-functions that referenced this pull request Sep 8, 2023

Merge pull request serverless-operations#133 from vlewin/add_authoriz…

58eca5e

…er_support Enable API Gateway authorizers

		@@ -6,65 +6,247 @@ const _ = require('lodash');
		module.exports = {

		@@ -70,15 +70,15 @@ describe('#compileResources()', () => {
		},

Enable API Gateway authorizers #133

Enable API Gateway authorizers #133

Uh oh!

Conversation

vlewin commented Jul 14, 2018

Uh oh!

vlewin commented Jul 14, 2018

Uh oh!

vlewin commented Jul 14, 2018

Uh oh!

horike37 commented Jul 17, 2018

Uh oh!

vlewin commented Jul 17, 2018

Uh oh!

horike37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlewin commented Jul 21, 2018

Uh oh!

horike37 commented Jul 25, 2018

Uh oh!

vlewin commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlewin commented Jul 25, 2018

Uh oh!

horike37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

horike37 commented Jul 29, 2018

Uh oh!

vlewin commented Jul 29, 2018

Uh oh!

horike37 commented Jul 30, 2018

Uh oh!

vlewin commented Jul 30, 2018

Uh oh!

Uh oh!

vlewin commented Jul 25, 2018 •

edited

Loading