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.

Sign up for GitHub

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

Jump to bottom

Added FromCustomAuthorizerAttribute and tests. #1466

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Added FromCustomAuthorizerAttribute and tests. #1466

wants to merge 13 commits into from

Conversation

kabaluk
Copy link
Contributor

@kabaluk kabaluk commented Mar 26, 2023

Description of changes:
Added FromCustomAuthorizerAttribute and tests.
This will allow to retrieve values from CustomAuthorizer context without having to receive the full APIGatewayProxyRequest or APIGatewayHttpApiV2ProxyRequest..

Both REST and HttpApi supported

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

normj
normj requested changes Mar 28, 2023
View reviewed changes
Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

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

Cool PR!

Can you fix the whitespaces changes to the LambdaFunctionTemplate.tt file? The whole file is looks different.

Who should we handle the other type of authorizer properties. For example on the APIGatewayHttpApiV2ProxyRequest besides the Lambda property there is also Jwt and IAM properties. Should we make the string accessing the authorizer field have the prefix of the property.

@kabaluk
Copy link
Contributor Author

kabaluk commented Mar 29, 2023 •
edited
Loading

Wiil do my best with the withspace.
The other types of authorizer are not custom authorizers as far as i am aware.
I would not create string prefixes.
We could create an iam authorizer attribute and a jwt authorizer attribute and those wold look on the respective locations...

@kabaluk
Copy link
Contributor Author

kabaluk commented Mar 30, 2023 •
edited
Loading

don't seem to be able to fix the whitespace issue to save my life :(
Please review ignoring whitespace ?? Maybe?

Tried setting EOL to LF and CR, both cause problems with the generated file. The correct one seems to be CRLF .
but obviously git doens't like it :(

Please feel free to edit.

@kabaluk kabaluk requested a review from normj March 30, 2023 01:09
@kabaluk
Copy link
Contributor Author

kabaluk commented May 4, 2023

Would be really nice to get this avail;able. I will gladly fix the conflicts if this can be looked at again.

@kabaluk
Copy link
Contributor Author

kabaluk commented May 8, 2023

I can see you did a major refactoring in the dev branch will push again once released.

If i might be so bold, I looked at what you did.
why not take it one step further and separate each attribute into it's own file?
Talking about the APIGatewaySetupParameters.tt
Breaking that file down, using the technique you used before, would make it much easier to add new attributes in the future

Just a thought.

@normj
Copy link
Member

normj commented May 9, 2023

@kabaluk I suspect adding new attributes would be done in separate files. For example if we added SQS or S3 I would put them in separate TT. One of my goals with the refactor was to make it easier to add new attributes as the previous monolithic tt file was really hard to reconcile with. HttpApi and RestApi attributes are 9X percent the same code so I didn't really want to make 2 separate tt files for each one of those. Plus I thought the parameter setup was getting pretty long so I was looking for some separation which is why I divided the parameter setup and the invoke. I suspect in SQS or S3 use cases the parameter setup wouldn't be so complicated.

Another goal I had was I wanted to generate the exact same code including whitespaces to make the PR review easier and then later I could do more specific refactoring. Otherwise the PR review would have been every line is different and that is hard to review. But going forward I could see putting each of the FromXXX in a separate tt file as well.

@kabaluk
Copy link
Contributor Author

kabaluk commented May 9, 2023

@kabaluk I suspect adding new attributes would be done in separate files. For example if we added SQS or S3 I would put them in separate TT. One of my goals with the refactor was to make it easier to add new attributes as the previous monolithic tt file was really hard to reconcile with. HttpApi and RestApi attributes are 9X percent the same code so I didn't really want to make 2 separate tt files for each one of those. Plus I thought the parameter setup was getting pretty long so I was looking for some separation which is why I divided the parameter setup and the invoke. I suspect in SQS or S3 use cases the parameter setup wouldn't be so complicated.

Another goal I had was I wanted to generate the exact same code including whitespaces to make the PR review easier and then later I could do more specific refactoring. Otherwise the PR review would have been every line is different and that is hard to review. But going forward I could see putting each of the FromXXX in a separate tt file as well.

I was talking specifically of the FromXXX attributes. Apologies, I should have been more explicit. Sounds like a great change and it would make it a lot easier to add and test more FromXXX attributes. Looking forward to have that in main so i can re add the FromCustomAuthorizer attribute.
Even started looking on how to do the FromJwt. 😄

@kabaluk
Copy link
Contributor Author

kabaluk commented May 11, 2023 •
edited
Loading

Readded CustomAuthorizerAttribute..

Hope you like it :)

@kabaluk
Copy link
Contributor Author

kabaluk commented May 18, 2023

Any possibility of having this looked at again, please?

normj
normj requested changes May 19, 2023
View reviewed changes
Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

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

I did an initial scan and it looks good. I had one comment so far. I need to test the experience end to end next. I'll try and find time to do that soon.

Libraries/src/Amazon.Lambda.Annotations.SourceGenerator/Templates/APIGatewaySetupParameters.tt Outdated
var <#= parameter.Name #> = default(<#= parameter.Type.FullName #>);
if (__request__.RequestContext?.Authorizer == null)
{
validationErrors.Add("Could not find Authorizer data for request");
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if there is missing data from the authorizer if we should be returning back 401 unauthorized instead of a bad parameter status code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. It should return 401.
I am away from pc for 2 weeks, feel free to change or i will once back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be able to have a look at Norm's comments?

@ashishdhingra
Copy link
Contributor

@kabaluk Please review the last review comment from @normj and see if you are able to address it.

@ashishdhingra ashishdhingra added module/aspnetcore-support p2 This is a standard priority issue feature-request A feature should be added or improved. queued labels Sep 1, 2023
@kabaluk
Copy link
Contributor Author

kabaluk commented Sep 3, 2023

@kabaluk Please review the last review comment from @normj and see if you are able to address it.

Please have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@normj normj Awaiting requested review from normj

@philasmar philasmar Awaiting requested review from philasmar

Assignees
No one assigned
Labels
feature-request A feature should be added or improved. module/aspnetcore-support p2 This is a standard priority issue queued
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kabaluk @normj @ashishdhingra @philasmar