Skip to content

Fix openapi authorization header for type token#7851

Merged
calvernaz merged 3 commits intomasterfrom
fix-openapi-auth-token
Mar 14, 2023
Merged

Fix openapi authorization header for type token#7851
calvernaz merged 3 commits intomasterfrom
fix-openapi-auth-token

Conversation

@calvernaz
Copy link
Contributor

@calvernaz calvernaz commented Mar 13, 2023

small fix to handle openapi x-amazon-apigateway-authorizer extension for authorizer type TOKEN.

https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-swagger-extensions-authorizer.html

@calvernaz calvernaz temporarily deployed to localstack-ext-tests March 13, 2023 17:03 — with GitHub Actions Inactive
@coveralls
Copy link

coveralls commented Mar 13, 2023

Coverage Status

Coverage: 85.167% (-0.009%) from 85.176% when pulling 1f2b433 on fix-openapi-auth-token into aa0b200 on master.

@github-actions
Copy link

github-actions bot commented Mar 13, 2023

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 39m 26s ⏱️ + 1m 27s
1 797 tests ±0  1 415 ✔️ ±0  382 💤 ±0  0 ±0 
2 523 runs  ±0  1 789 ✔️ ±0  734 💤 ±0  0 ±0 

Results for commit 1f2b433. ± Comparison against base commit aa0b200.

♻️ This comment has been updated with latest results.

@calvernaz calvernaz temporarily deployed to localstack-ext-tests March 13, 2023 21:19 — with GitHub Actions Inactive
@calvernaz calvernaz temporarily deployed to localstack-ext-tests March 13, 2023 21:27 — with GitHub Actions Inactive
Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

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

+1 on the refined logic based on authorizer_type 👍 - just not sure if we're selecting the default identitySource header name properly here..

if identity_source := aws_apigateway_authorizer.get("identitySource"):
if authorizer_type == "TOKEN":
header_name = security_config.get("name")
authorizer["identitySource"] = f"method.request.header.{header_name}"
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we can rely on the name of the security config to be the header name of the identitySource here? I understand that this is part of the examples in the AWS docs , but seems to me that the name can be arbitrarily chosen (and hence may not necessarily map to the header name).

Maybe the default identitySource should rather be "method.request.header.Authorization" here? As usual, a parity/snapshot test would help us get ultimate clarity on this question. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I agree here, so the name is a specific field of the authorizer extension, and for the type TOKEN must be a header.

Maybe the default identitySource should rather be "method.request.header.Authorization" here? As usual, a parity/snapshot test would help us get ultimate clarity on this question.

that's precisely one of the issues, we had always default to that and must use whatever the user defines - https://github.com/localstack/localstack-ext/blob/289bfe82cfa11f9b79412938e8c647c69aa2375c/localstack_ext/services/apigateway/authorizers.py#L387-L389

Copy link
Member

@whummer whummer Mar 13, 2023

Choose a reason for hiding this comment

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

Actually, I think you're right - thanks for clarifying! Looking more closely at this line in the docs - looks like the name of the authorizer is indeed the name of the header 👍 :

      "name" : "Authorization",                  // The name of the header containing the authorization token.

Happy to merge as-is. As usual with API Gateway: the docs can sometimes be misleading/outdated/unclear - so let's try to complement this with a snapshot test in an upcoming PR!

@calvernaz calvernaz merged commit 2fddfda into master Mar 14, 2023
@calvernaz calvernaz deleted the fix-openapi-auth-token branch March 14, 2023 11:03
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