-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
if authorizer := get_authorizer(method_schema) or default_authorizer: | ||
method_authorizer = authorizer or default_authorizer |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😄
There was a problem hiding this 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! 💯
if authorizer := get_authorizer(method_schema) or default_authorizer: | ||
method_authorizer = authorizer or default_authorizer |
There was a problem hiding this comment.
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"])) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
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