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

Feature/issue 1475 #1500

Closed
wants to merge 7 commits into from
Closed

Conversation

vidueirof
Copy link

@vidueirof vidueirof commented Jul 5, 2022

Description

Support simple lambda authorizer response by checking if function result has isAuthorized flag and prevent running policy document validations.

Motivation and Context

To add support for simple response in authorizer function result.

Fixes: #1475 and #1341

How Has This Been Tested?

Simple lambda authorizer.

authorizer.js

export const handler = async (event) => {
    let response = {
        "isAuthorized": false,
    };
    
    if (event.authorizationToken === 'secretToken') { 
        response = {
            "isAuthorized": true,
        };
    }

    return response;
}

serverless.yml

.
.
.
provider:
  .
  .
  .
  httpApi:
    authorizers:
      authorizer:
        type: request
        functionName: authorizer
        enableSimpleResponses: true

functions:
  authorizer:
    handler: authorizer.handler
  hello:
    handler: hello.handler
    events:
      - httpApi:
          method: GET
          path: /
          authorizer:
            name: authorizer

Using correct and wrong token

@dnalborczyk
Copy link
Collaborator

thank you @vidueirof ! could you add your tests to the tests of this project? let me know if you have any questions or need any pointers.

@vidueirof
Copy link
Author

@dnalborczyk I added some tests, let me know what you think. Thanks

authorizer:
type: request
functionName: authorizer
enableSimpleResponses: true
Copy link
Collaborator

@dnalborczyk dnalborczyk Jul 31, 2022

Choose a reason for hiding this comment

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

I don't see enableSimpleResponses anywhere in the current code being used nor in your PR. the tests pass with or without this property being set. is that intentional? I must admit I currently don't know much about the topic.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's right. I'm checking if the response is in simple response format.

Copy link
Collaborator

Choose a reason for hiding this comment

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

what I mean is that I don't see any usage of enableSimpleResponses, e.g. in a condition anywhere in your code.

also your tests pass as well if I comment out enableSimpleResponses: true

    authorizer:
        type: request
        functionName: authorizer
        # enableSimpleResponses: true

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I understand your point. I'm not using it because I'm checking if the response is in simple response format without caring enableSimpleResponses flag. That's why if you set to false but your response is in simple format test will pass. I don't think we need to force to use enableSimpleResponses to true, just using a valid format is enough. But if you think is better to check enableSimpleResponses try to do it (no idea how :p but I'll work it out).

@dnalborczyk
Copy link
Collaborator

dnalborczyk commented Jul 31, 2022

thanks for adding the tests @vidueirof just added one question, otherwise looks good.

could you also merge (or rebase) with master? should be just an import fix as far as I can tell: import { setup, teardown } from '../../_testHelpers/index.js'.

@dnalborczyk dnalborczyk force-pushed the master branch 2 times, most recently from d5ea86d to 22fd667 Compare October 9, 2022 02:13
@dnalborczyk dnalborczyk force-pushed the master branch 2 times, most recently from fdd1699 to 51a30e9 Compare October 15, 2022 19:39
@rion18
Copy link
Contributor

rion18 commented Nov 5, 2022

Hi @dnalborczyk @vidueirof .

I've implemented this on #1600, where we accept payloadFormat 1.0, 2.0 and 2.0 + simpleResponses.

There are also some checks done where enableSimpleResponses HAS to be used along with payloadFormat 2.0, as well as a set of tests with httpApi with payloadFormat 1.0, 2.0 and simple responses.

Please feel free to take a look at it and see if that PR fixes the needs of this PR.

@dnalborczyk
Copy link
Collaborator

@vidueirof could you have a look at v11.3.0 if that covers your PR? thank you again for putting in the work! much appreciated!

closing in the meantime for good housekeeping.

@dnalborczyk dnalborczyk closed this Nov 8, 2022
@vidueirof
Copy link
Author

@vidueirof could you have a look at v11.3.0 if that covers your PR? thank you again for putting in the work! much appreciated!

closing in the meantime for good housekeeping.

Yes, it's looks fine. I'm sorry I couldn't fix this PR on time.

@rion18
Copy link
Contributor

rion18 commented Nov 8, 2022

No worries man, I was also looking for support for PayloadFormat 2.0. Glad I could help out.

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.

serverless-offline doesn't support simple lambda authorizer response
3 participants