From 007a880aa38c364c58850e69e4a98cf4a36eb945 Mon Sep 17 00:00:00 2001 From: Georges Haidar Date: Wed, 2 Apr 2025 16:20:29 +0000 Subject: [PATCH 1/2] fix(codegen): dedupe security schemes by type This commit solves a bug where designs that use multiple security schemes _of the same type_ break the generated `Auther` interface for a service as well as endpoint-related init methods due to unnecessary code duplication from ranging over `.Schemes`. --- codegen/service/service_data.go | 15 +++++++++++++++ codegen/service/templates/service.go.tpl | 2 +- .../templates/service_endpoint_method.go.tpl | 2 +- .../templates/service_endpoints_init.go.tpl | 2 +- 4 files changed, 18 insertions(+), 3 deletions(-) diff --git a/codegen/service/service_data.go b/codegen/service/service_data.go index c29d8631d3..c5e22b57f8 100644 --- a/codegen/service/service_data.go +++ b/codegen/service/service_data.go @@ -679,6 +679,21 @@ func (s SchemesData) Append(d *SchemeData) SchemesData { return append(s, d) } +// DedupeByType returns a new SchemesData slice that is deduplicated by scheme +// type. +func (s SchemesData) DedupeByType() SchemesData { + seen := make(map[string]struct{}) + uniqueSchemes := SchemesData{} + for _, s := range s { + if _, ok := seen[s.Type]; !ok { + seen[s.Type] = struct{}{} + uniqueSchemes = append(uniqueSchemes, s) + } + } + + return uniqueSchemes +} + // analyze creates the data necessary to render the code of the given service. // It records the user types needed by the service definition in userTypes. func (d ServicesData) analyze(service *expr.ServiceExpr) *Data { diff --git a/codegen/service/templates/service.go.tpl b/codegen/service/templates/service.go.tpl index 6a3334a5bd..396b825295 100644 --- a/codegen/service/templates/service.go.tpl +++ b/codegen/service/templates/service.go.tpl @@ -29,7 +29,7 @@ type Service interface { {{- if .Schemes }} // Auther defines the authorization functions to be implemented by the service. type Auther interface { - {{- range .Schemes }} + {{- range .Schemes.DedupeByType }} {{ printf "%sAuth implements the authorization logic for the %s security scheme." .Type .Type | comment }} {{ .Type }}Auth(ctx context.Context, {{ if eq .Type "Basic" }}user, pass{{ else if eq .Type "APIKey" }}key{{ else }}token{{ end }} string, schema *security.{{ .Type }}Scheme) (context.Context, error) {{- end }} diff --git a/codegen/service/templates/service_endpoint_method.go.tpl b/codegen/service/templates/service_endpoint_method.go.tpl index ccc654e0d2..dca0391e8b 100644 --- a/codegen/service/templates/service_endpoint_method.go.tpl +++ b/codegen/service/templates/service_endpoint_method.go.tpl @@ -1,7 +1,7 @@ {{ printf "New%sEndpoint returns an endpoint function that calls the method %q of service %q." .VarName .Name .ServiceName | comment }} -func New{{ .VarName }}Endpoint(s {{ .ServiceVarName }}{{ range .Schemes }}, auth{{ .Type }}Fn security.Auth{{ .Type }}Func{{ end }}) goa.Endpoint { +func New{{ .VarName }}Endpoint(s {{ .ServiceVarName }}{{ range .Schemes.DedupeByType }}, auth{{ .Type }}Fn security.Auth{{ .Type }}Func{{ end }}) goa.Endpoint { return func(ctx context.Context, req any) (any, error) { {{- if or .ServerStream }} ep := req.(*{{ .ServerStream.EndpointStruct }}) diff --git a/codegen/service/templates/service_endpoints_init.go.tpl b/codegen/service/templates/service_endpoints_init.go.tpl index b1c9e6c461..38770a22f7 100644 --- a/codegen/service/templates/service_endpoints_init.go.tpl +++ b/codegen/service/templates/service_endpoints_init.go.tpl @@ -11,7 +11,7 @@ func New{{ .VarName }}(s {{ .ServiceVarName }}{{ if .HasServerInterceptors }}, s return &{{ .VarName }}{ {{- end }} {{- range .Methods }} - {{ .VarName }}: New{{ .VarName }}Endpoint(s{{ range .Schemes }}, a.{{ .Type }}Auth{{ end }}), + {{ .VarName }}: New{{ .VarName }}Endpoint(s{{ range .Schemes.DedupeByType }}, a.{{ .Type }}Auth{{ end }}), {{- end }} } {{- if .HasServerInterceptors }} From 14ae51d6aa5543a6a624aee57489a6e3cd3ba06d Mon Sep 17 00:00:00 2001 From: Georges Haidar Date: Sun, 11 May 2025 17:23:01 +0000 Subject: [PATCH 2/2] chore: add tests for multiple security services --- codegen/service/service_test.go | 2 + codegen/service/testdata/service_code.go | 76 ++++++++++++++++++++++++ codegen/service/testdata/service_dsls.go | 52 ++++++++++++++++ 3 files changed, 130 insertions(+) diff --git a/codegen/service/service_test.go b/codegen/service/service_test.go index dce4304d85..6d0197e621 100644 --- a/codegen/service/service_test.go +++ b/codegen/service/service_test.go @@ -56,6 +56,8 @@ func TestService(t *testing.T) { {"service-bidirectional-streaming-no-payload", testdata.BidirectionalStreamingNoPayloadMethodDSL, testdata.BidirectionalStreamingNoPayloadMethod}, {"service-bidirectional-streaming-result-with-views", testdata.BidirectionalStreamingResultWithViewsMethodDSL, testdata.BidirectionalStreamingResultWithViewsMethod}, {"service-bidirectional-streaming-result-with-explicit-view", testdata.BidirectionalStreamingResultWithExplicitViewMethodDSL, testdata.BidirectionalStreamingResultWithExplicitViewMethod}, + {"service-multiple-api-key-security", testdata.MultipleAPIKeySecurityDSL, testdata.MultipleAPIKeySecurity}, + {"service-mixed-and-multiple-api-key-security", testdata.MixedAndMultipleAPIKeySecurityDSL, testdata.MixedAndMultipleAPIKeySecurity}, } for _, c := range cases { t.Run(c.Name, func(t *testing.T) { diff --git a/codegen/service/testdata/service_code.go b/codegen/service/testdata/service_code.go index b2d92cdf43..36bea5dd61 100644 --- a/codegen/service/testdata/service_code.go +++ b/codegen/service/testdata/service_code.go @@ -3315,3 +3315,79 @@ type Foo struct { IntField *int } ` + +const MultipleAPIKeySecurity = ` +// Service is the MultipleAPIKeySecurity service interface. +type Service interface { + // A implements A. + A(context.Context, *APayload) (err error) +} + +// Auther defines the authorization functions to be implemented by the service. +type Auther interface { + // APIKeyAuth implements the authorization logic for the APIKey security scheme. + APIKeyAuth(ctx context.Context, key string, schema *security.APIKeyScheme) (context.Context, error) +} + +// APIName is the name of the API as defined in the design. +const APIName = "test api" + +// APIVersion is the version of the API as defined in the design. +const APIVersion = "0.0.1" + +// ServiceName is the name of the service as defined in the design. This is the +// same value that is set in the endpoint request contexts under the ServiceKey +// key. +const ServiceName = "MultipleAPIKeySecurity" + +// MethodNames lists the service method names as defined in the design. These +// are the same values that are set in the endpoint request contexts under the +// MethodKey key. +var MethodNames = [1]string{"A"} + +// APayload is the payload type of the MultipleAPIKeySecurity service A method. +type APayload struct { + APIKey string + TenantID string +} +` + +const MixedAndMultipleAPIKeySecurity = ` +// Service is the MixedAndMultipleAPIKeySecurity service interface. +type Service interface { + // A implements A. + A(context.Context, *APayload) (err error) +} + +// Auther defines the authorization functions to be implemented by the service. +type Auther interface { + // JWTAuth implements the authorization logic for the JWT security scheme. + JWTAuth(ctx context.Context, token string, schema *security.JWTScheme) (context.Context, error) + // APIKeyAuth implements the authorization logic for the APIKey security scheme. + APIKeyAuth(ctx context.Context, key string, schema *security.APIKeyScheme) (context.Context, error) +} + +// APIName is the name of the API as defined in the design. +const APIName = "test api" + +// APIVersion is the version of the API as defined in the design. +const APIVersion = "0.0.1" + +// ServiceName is the name of the service as defined in the design. This is the +// same value that is set in the endpoint request contexts under the ServiceKey +// key. +const ServiceName = "MixedAndMultipleAPIKeySecurity" + +// MethodNames lists the service method names as defined in the design. These +// are the same values that are set in the endpoint request contexts under the +// MethodKey key. +var MethodNames = [1]string{"A"} + +// APayload is the payload type of the MixedAndMultipleAPIKeySecurity service A +// method. +type APayload struct { + JWT *string + APIKey *string + TenantID *string +} +` diff --git a/codegen/service/testdata/service_dsls.go b/codegen/service/testdata/service_dsls.go index 76f7f917b8..b018d74599 100644 --- a/codegen/service/testdata/service_dsls.go +++ b/codegen/service/testdata/service_dsls.go @@ -981,3 +981,55 @@ var EndpointWithClientInterceptorDSL = func() { }) }) } + +var MultipleAPIKeySecurityDSL = func() { + var APIKeyAuth = APIKeySecurity("api_key") + var TenantKeyAuth = APIKeySecurity("tenant") + + Service("MultipleAPIKeySecurity", func() { + Security(APIKeyAuth, TenantKeyAuth) + Method("A", func() { + Payload(func() { + APIKey("api_key", "api_key", String) + APIKey("tenant", "tenant_id", String) + Required("api_key", "tenant_id") + }) + + HTTP(func() { + POST("/") + Header("api_key:X-API-Key") + Header("tenant_id:X-Tenant") + }) + }) + }) +} + +var MixedAndMultipleAPIKeySecurityDSL = func() { + var Signature = JWTSecurity("jwt", func() { + Scope("api:read", "Read access to API") + }) + var APIKeyAuth = APIKeySecurity("api_key") + var TenantKeyAuth = APIKeySecurity("tenant") + + Service("MixedAndMultipleAPIKeySecurity", func() { + Security(Signature, func() { + Scope("api:read") + }) + Security(APIKeyAuth, TenantKeyAuth) + + Method("A", func() { + Payload(func() { + Token("jwt", String) + APIKey("api_key", "api_key", String) + APIKey("tenant", "tenant_id", String) + }) + + HTTP(func() { + POST("/") + Header("api_key:X-API-Key") + Header("tenant_id:X-Tenant") + Header("jwt:Authorization") + }) + }) + }) +}