10000 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.

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

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Apigw fix openapi identity sources #11939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
645b097
improved test
cloutierMat Nov 26, 2024
93e614f
Added authorizer_scopes to `add_method`
cloutierMat Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions localstack-core/localstack/services/apigateway/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import json
import logging
from datetime import datetime
from typing import List, Optional, Union
from typing import List, Optional, TypedDict, Union
from urllib import parse as urlparse

from jsonpatch import apply_patch
Expand Down Expand Up @@ -91,6 +91,11 @@ class OpenAPIExt:
TAG_VALUE = "x-amazon-apigateway-tag-value"


class AuthorizerConfig(TypedDict):
authorizer: Authorizer
authorization_scopes: Optional[list[str]]


# TODO: make the CRUD operations in this file generic for the different model types (authorizes, validators, ...)


Expand Down Expand Up @@ -564,14 +569,14 @@ def create_authorizers(security_schemes: dict) -> None:

authorizers[security_scheme_name] = authorizer

def get_authorizer(path_payload: dict) -> Optional[Authorizer]:
def get_authorizer(path_payload: dict) -> Optional[AuthorizerConfig]:
if not (security_schemes := path_payload.get("security")):
return None

for security_scheme in security_schemes:
for security_scheme_name in security_scheme.keys():
for security_scheme_name, scopes in security_scheme.items():
if authorizer := authorizers.get(security_scheme_name):
return authorizer
return AuthorizerConfig(authorizer=authorizer, authorization_scopes=scopes)

def get_or_create_path(abs_path: str, base_path: str):
parts = abs_path.rstrip("/").replace("//", "/").split("/")
Expand Down Expand Up @@ -815,7 +820,7 @@ def create_method_resource(child, method, method_schema):
kwargs = {}

if authorizer := get_authorizer(method_schema) or default_authorizer:
method_authorizer = authorizer or default_authorizer
Comment on lines 822 to -818
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 😄

method_authorizer = authorizer["authorizer"]
# override the authorizer_type if it's a TOKEN or REQUEST to CUSTOM
if (authorizer_type := method_authorizer["type"]) in ("TOKEN", "REQUEST"):
authorization_type = "CUSTOM"
Expand All @@ -824,6 +829,9 @@ def create_method_resource(child, method, method_schema):

kwargs["authorizer_id"] = method_authorizer["id"]

if authorization_scopes := authorizer.get("authorization_scopes"):
kwargs["authorization_scopes"] = authorization_scopes

return child.add_method(
method,
api_key_required=api_key_required,
Expand Down
84 changes: 83 additions & 1 deletion tests/aws/files/openapi.cognito-auth.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,75 @@
"version": "1.0"
},
"paths": {
"/default-no-scope": {
"get": {
"security": [
{"cognito-test-identity-source": []}
],
"responses": {
"200": {
"description": "200 response"
}
},
"x-amazon-apigateway-integration": {
"responses": {
"default": {
"statusCode": "200"
}
},
"uri": "http://petstore-demo-endpoint.execute-api.com/petstore/pets",
"passthroughBehavior": "when_no_match",
"httpMethod": "GET",
"type": "http"
}
}
},
"/default-scope-override": {
"get": {
"security": [
{"cognito-test-identity-source": ["openid"]}
],
"responses": {
"200": {
"description": "200 response"
}
},
"x-amazon-apigateway-integration": {
"responses": {
"default": {
"statusCode": "200"
}
},
"uri": "http://petstore-demo-endpoint.execute-api.com/petstore/pets",
"passthroughBehavior": "when_no_match",
"httpMethod": "GET",
"type": "http"
}
}
},
"/non-default-authorizer": {
"get": {
"security": [
{"extra-test-identity-source": ["email", "openid"]}
],
"responses": {
"200": {
"description": "200 response"
}
},
"x-amazon-apigateway-integration": {
"responses": {
"default": {
"statusCode": "200"
}
},
"uri": "http://petstore-demo-endpoint.execute-api.com/petstore/pets",
"passthroughBehavior": "when_no_match",
"httpMethod": "GET",
"type": "http"
}
}
},
"/pets": {
"get": {
"operationId": "GET HTTP",
Expand Down Expand Up @@ -66,6 +135,18 @@
"${cognito_pool_arn}"
]
}
},
"extra-test-identity-source": {
"type": "apiKey",
"name": "TestHeaderAuth",
"in": "header",
"x-amazon-apigateway-authtype": "cognito_user_pools",
"x-amazon-apigateway-authorizer": {
"type": "cognito_user_pools",
"providerARNs": [
"${cognito_pool_arn}"
]
}
}
},
"schemas": {
Expand Down Expand Up @@ -93,5 +174,6 @@
}
}
}
}
},
"security": [{"cognito-test-identity-source": ["email"]}]
}
20 changes: 16 additions & 4 deletions tests/aws/services/apigateway/test_apigateway_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,11 @@ def test_import_with_http_method_integration(
apigw_snapshot_imported_resources(rest_api_id=rest 6DB6 _api_id, resources=response)

@pytest.mark.no_apigw_snap_transformers
@markers.snapshot.skip_snapshot_verify(
paths=[
"$.resources.items..resourceMethods.GET", # AWS does not show them after import
]
)
@markers.aws.validated
def test_import_with_cognito_auth_identity_source(
self,
Expand All @@ -856,10 +861,10 @@ def test_import_with_cognito_auth_identity_source(
[
snapshot.transform.jsonpath("$.import-swagger.id", value_replacement="rest-id"),
snapshot.transform.jsonpath(
"$.import-swagger.rootResourceId", value_replacement="root-resource-id"
"$.resources.items..id", value_replacement="resource-id"
),
snapshot.transform.jsonpath(
"$.get-authorizers.items..id", value_replacement="authorizer-id"
"$.get-authorizers..id", value_replacement="authorizer-id"
),
]
)
Expand All @@ -874,6 +879,13 @@ def test_import_with_cognito_auth_identity_source(

rest_api_id = response["id"]

# assert that are no multiple authorizers
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

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!


response = aws_client.apigateway.get_resources(restApiId=rest_api_id)
response["items"] = sorted(response["items"], key=itemgetter("path"))
snapshot.match("resources", response)

# this fixture will iterate over every resource and match its method, methodResponse, integration and
# integrationResponse
apigw_snapshot_imported_resources(rest_api_id=rest_api_id, resources=response)
Loading
Loading
0