8000 Apigw fix openapi identity sources by cloutierMat · Pull Request #11939 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

Apigw fix openapi identity sources #11939

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

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

cloutierMat
Copy link
Contributor

Motivation

Our current implementation of the OpenApi import did not add authorization scopes when creating a method with an authorizer. This was not an issue with Legacy apigw, as we did not handle the scopes properly.

Authorization scopes can be added to Cognito authorizers. They directly impact whether the api accepts an Identity token or an Access Token see docs. As Next Gen provider does enforce this, providing a greater compatibility with aws, user's configs would fail to authorize a request when an Access token is used to authenticate.

Changes

  • Get the scopes from the default authorizer or the method if specified
  • Added tests

@cloutierMat cloutierMat added aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases labels Nov 26, 2024
@cloutierMat cloutierMat added this to the 4.0.3 milestone Nov 26, 2024
@cloutierMat cloutierMat self-assigned this Nov 26, 2024
@cloutierMat cloutierMat requested a review from bentsku as a code owner November 26, 2024 21:50
Comment on lines 822 to -818
if authorizer := get_authorizer(method_schema) or default_authorizer:
method_authorizer = authorizer or default_authorizer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bentsku This seemed to be a redundant operation... Do you see any reason to keep this? Was this a issue with the minifier/obfuscation tool?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this was just being too safe probably? Don't think it's because of the minifier, as this is community and this code isn't minified. Probably just a mistake not caught in a review 😄

Copy link

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 49m 36s ⏱️ - 1m 21s
3 733 tests ±0  3 387 ✅ ±0  346 💤 ±0  0 ❌ ±0 
3 735 runs  ±0  3 387 ✅ ±0  348 💤 ±0  0 ❌ ±0 

Results for commit 93e614f. ± Comparison against base commit e5aa33d.

Copy link
Contributor
@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Nice find! I can't wait for us to rework the Import/Export functionality in the future, as this helper should at least be extracted in its own file, and reworked to be a bit clearer... and more tested 😄 nice and clean fix! 💯

Comment on lines 822 to -818
if authorizer := get_authorizer(method_schema) or default_authorizer:
method_authorizer = authorizer or default_authorizer
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this was just being too safe probably? Don't think it's because of the minifier, as this is community and this code isn't minified. Probably just a mistake not caught in a review 😄

authorizers = aws_client.apigateway.get_authorizers(restApiId=rest_api_id)
snapshot.match("get-authorizers", authorizers)
snapshot.match("get-authorizers", sorted(authorizers["items"], key=lambda x: x["name"]))
Copy link
Contributor

Choose a reason for hiding this comment

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

very minor nit: could also use itemgetter("name") here like the sorted call below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, don't have to change it, the CI is green, let's keep it!

@cloutierMat cloutierMat merged commit 221fde1 into master Nov 26, 2024
42 checks passed
@cloutierMat cloutierMat deleted the apigw-fix-openapi-identity-sources branch November 26, 2024 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0