Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ No commented-out code—delete dead code.

- **Use NameScope helpers** for type references: `GoTypeRef`, `GoFullTypeRef`, `GoTypeName`. Never concatenate strings for types.
- Let Goa decide pointer/value semantics. Do not force `pointer=true` except in transport validation.
- **Keep helper visibility minimal**: If logic is shared only inside one codegen area, keep it package-private or move it under an `internal` package. Do not export helpers from a parent package just to share them across sibling generators.
- **Avoid pass-through wrappers**: When two helper functions differ only by forwarding arguments or hard-coding `nil`, collapse them into a single implementation instead of adding an extra layer.

### Documentation

Expand Down
22 changes: 17 additions & 5 deletions codegen/sections_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,23 @@ import (
package testpackage

`
imprt = []*ImportSpec{{Path: "test"}}
imports = append(imprt, &ImportSpec{Path: "other"})
pathImport = []*ImportSpec{{Path: "import/with/slashes"}}
pathImports = append(pathImport, &ImportSpec{Path: "other/import/with/slashes"})
pathNamedImport = []*ImportSpec{{Name: "myname", Path: "import/with/slashes"}}
imprt = func() []*ImportSpec {
imports := make([]*ImportSpec, 0, 2)
imports = append(imports, &ImportSpec{Path: "test"})
return imports
}()
imports = append(imprt, &ImportSpec{Path: "other"})
pathImport = func() []*ImportSpec {
imports := make([]*ImportSpec, 0, 2)
imports = append(imports, &ImportSpec{Path: "import/with/slashes"})
return imports
}()
pathImports = append(pathImport, &ImportSpec{Path: "other/import/with/slashes"})
pathNamedImport = func() []*ImportSpec {
imports := make([]*ImportSpec, 0, 2)
imports = append(imports, &ImportSpec{Name: "myname", Path: "import/with/slashes"})
return imports
}()
pathNamedImports = append(pathNamedImport, &ImportSpec{Name: "myothername", Path: "other/import/with/slashes"})
)
cases := map[string]struct {
Expand Down
6 changes: 4 additions & 2 deletions expr/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,17 @@ func Title(s string) string {
}

// findKey finds the given key in the endpoint expression and returns the
// transport element name and the position (header, query, or body for HTTP or
// message, metadata for gRPC endpoint).
// transport element name and the position (header, query, cookie, or body for
// HTTP or message, metadata for gRPC endpoint).
func findKey(exp eval.Expression, keyAtt string) (string, string) {
switch e := exp.(type) {
case *HTTPEndpointExpr:
if n, exists := e.Params.FindKey(keyAtt); exists {
return n, "query"
} else if n, exists := e.Headers.FindKey(keyAtt); exists {
return n, "header"
} else if n, exists := e.Cookies.FindKey(keyAtt); exists {
return n, "cookie"
} else if e.Body == nil {
return "", "header"
}
Expand Down
156 changes: 156 additions & 0 deletions http/codegen/cookie_security_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
package codegen

import (
"bytes"
"context"
"path/filepath"
"testing"
"text/template"

"github.com/getkin/kin-openapi/openapi2"
"github.com/getkin/kin-openapi/openapi3"
"github.com/stretchr/testify/require"

"goa.design/goa/v3/codegen"
"goa.design/goa/v3/dsl"
"goa.design/goa/v3/expr"
"goa.design/goa/v3/http/codegen/openapi"
openapiv2 "goa.design/goa/v3/http/codegen/openapi/v2"
openapiv3 "goa.design/goa/v3/http/codegen/openapi/v3"
)

func TestCookieAPIKeySecurity(t *testing.T) {
t.Run("endpoint requirement uses cookie transport", func(t *testing.T) {
root := RunHTTPDSL(t, cookieAPIKeySecurityDSL)
endpoint := root.API.HTTP.Services[0].HTTPEndpoints[0]
require.Len(t, endpoint.Requirements, 1)
require.Len(t, endpoint.Requirements[0].Schemes, 1)

scheme := endpoint.Requirements[0].Schemes[0]
require.Equal(t, "cookie", scheme.In)
require.Equal(t, "__Host-ak_session", scheme.Name)

headers := expr.AsObject(endpoint.Headers.Type)
require.Zero(t, len(*headers), "cookie-backed api key must not synthesize an Authorization header")
})

t.Run("openapi uses cookie security scheme", func(t *testing.T) {
root := RunHTTPDSL(t, cookieAPIKeySecurityDSL)
openapi.Definitions = make(map[string]*openapi.Schema)

v2JSON := renderOpenAPIJSON(t, openapiv2.Files, root)
var swagger openapi2.T
require.NoError(t, swagger.UnmarshalJSON(v2JSON))
require.Len(t, swagger.SecurityDefinitions, 1)
require.Len(t, swagger.Paths, 1)
require.Contains(t, swagger.Paths, "/auth/profile")
require.NotNil(t, swagger.Paths["/auth/profile"].Get.Security)
require.Len(t, *swagger.Paths["/auth/profile"].Get.Security, 1)
for name, def := range swagger.SecurityDefinitions {
require.Equal(t, "apiKey", def.Type, name)
require.Equal(t, "cookie", def.In, name)
require.Equal(t, "__Host-ak_session", def.Name, name)
require.Contains(t, (*swagger.Paths["/auth/profile"].Get.Security)[0], name)
}

openapi.Definitions = make(map[string]*openapi.Schema)
v3JSON := renderOpenAPIJSON(t, openapiv3.Files, root)
loader := openapi3.NewLoader()
doc, err := loader.LoadFromData(v3JSON)
require.NoError(t, err)
require.NoError(t, doc.Validate(context.Background()))
require.Len(t, doc.Components.SecuritySchemes, 1)
require.NotNil(t, doc.Paths.Find("/auth/profile"))
require.NotNil(t, doc.Paths.Find("/auth/profile").Get.Security)
require.Len(t, *doc.Paths.Find("/auth/profile").Get.Security, 1)
for name, ref := range doc.Components.SecuritySchemes {
require.NotNil(t, ref.Value, name)
require.Equal(t, "apiKey", ref.Value.Type, name)
require.Equal(t, "cookie", ref.Value.In, name)
require.Equal(t, "__Host-ak_session", ref.Value.Name, name)
require.Contains(t, (*doc.Paths.Find("/auth/profile").Get.Security)[0], name)
}
})

t.Run("http codegen does not duplicate cookie-backed auth fields", func(t *testing.T) {
root := RunHTTPDSL(t, cookieAPIKeySecurityDSL)
services := CreateHTTPServices(root)

serverTypes := serverType("gen", root.API.HTTP.Services[0], services)
var serverTypesBuf bytes.Buffer
for _, section := range serverTypes.SectionTemplates[1:] {
require.NoError(t, section.Write(&serverTypesBuf))
}
serverTypesCode := codegen.FormatTestCode(t, "package foo\n"+serverTypesBuf.String())
require.Contains(t, serverTypesCode, "func NewProfilePayload(browserSession string)")
require.NotContains(t, serverTypesCode, "browserSession *string, browserSession *string")
require.NotContains(t, serverTypesCode, "browserSession string, browserSession string")

serverFiles := ServerFiles("", services)
require.Len(t, serverFiles, 2)
serverDecode := codegen.SectionCode(t, serverFiles[1].SectionTemplates[2])
require.Contains(t, serverDecode, `r.Cookie("__Host-ak_session")`)
require.NotContains(t, serverDecode, "Authorization")
require.NotContains(t, serverDecode, "browserSession *string, browserSession *string")
require.NotContains(t, serverDecode, "browserSession string, browserSession string")

clientFiles := ClientFiles("", services)
require.Len(t, clientFiles, 2)
clientEncode := codegen.SectionCode(t, clientFiles[1].SectionTemplates[2])
require.Contains(t, clientEncode, `req.AddCookie(&http.Cookie{`)
require.Contains(t, clientEncode, `Name: "__Host-ak_session"`)
require.NotContains(t, clientEncode, "Authorization")
})
}

func renderOpenAPIJSON(
t *testing.T,
build func(*expr.RootExpr) ([]*codegen.File, error),
root *expr.RootExpr,
) []byte {
t.Helper()

files, err := build(root)
require.NoError(t, err)
for _, f := range files {
if filepath.Ext(f.Path) != ".json" {
continue
}
require.Len(t, f.SectionTemplates, 1)
section := f.SectionTemplates[0]
require.NotEmpty(t, section.Source)
require.NotNil(t, section.Data)

var buf bytes.Buffer
tmpl := template.Must(template.New("openapi").Funcs(section.FuncMap).Parse(section.Source))
require.NoError(t, tmpl.Execute(&buf, section.Data))
return buf.Bytes()
}

t.Fatalf("no JSON OpenAPI file generated")
return nil
}

var cookieAPIKeySecurityDSL = func() {
var browserSessionCookie = dsl.APIKeySecurity("browser_session_cookie", func() {
dsl.Description("Browser session cookie")
})

dsl.Service("cookieSecurity", func() {
dsl.Method("profile", func() {
dsl.Security(browserSessionCookie)
dsl.Payload(func() {
dsl.APIKey("browser_session_cookie", "browser_session", dsl.String, func() {
dsl.Description("Opaque browser session cookie")
})
dsl.Required("browser_session")
})
dsl.Result(dsl.Empty)
dsl.HTTP(func() {
dsl.GET("/auth/profile")
dsl.Cookie("browser_session:__Host-ak_session")
dsl.Response(dsl.StatusOK)
})
})
})
}
33 changes: 33 additions & 0 deletions http/codegen/openapi/internal/security.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package internal

import (
"strings"

"goa.design/goa/v3/expr"
)

// IsSecurityParameter returns true if the given HTTP transport element is used
// by one of the endpoint security schemes and should therefore not be emitted
// again as a regular OpenAPI parameter.
func IsSecurityParameter(endpoint *expr.HTTPEndpointExpr, in, name string) bool {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be better to keep this private, this is probably not something that the openapi package should expose.

if endpoint == nil {
return false
}
for _, req := range endpoint.Requirements {
for _, scheme := range req.Schemes {
if scheme.In != in {
continue
}
if in == "header" {
if strings.EqualFold(scheme.Name, name) {
return true
}
continue
}
if scheme.Name == name {
return true
}
}
}
return false
}
26 changes: 10 additions & 16 deletions http/codegen/openapi/v2/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"goa.design/goa/v3/codegen"
"goa.design/goa/v3/expr"
"goa.design/goa/v3/http/codegen/openapi"
openapiinternal "goa.design/goa/v3/http/codegen/openapi/internal"
)

// NewV2 returns the OpenAPI v2 specification for the given API.
Expand All @@ -35,7 +36,7 @@ func NewV2(root *expr.RootExpr, h *expr.HostExpr) (*V2, error) {
if hasAbsoluteRoutes(root) {
basePath = ""
}
params := paramsFromExpr(root.API.HTTP.Params, basePath)
params := paramsFromExpr(nil, root.API.HTTP.Params, basePath)
var paramMap map[string]*Parameter
if len(params) > 0 {
paramMap = make(map[string]*Parameter, len(params))
Expand Down Expand Up @@ -269,7 +270,7 @@ func summaryFromMeta(name string, meta expr.MetaExpr) string {
return name
}

func paramsFromExpr(params *expr.MappedAttributeExpr, path string) []*Parameter {
func paramsFromExpr(endpoint *expr.HTTPEndpointExpr, params *expr.MappedAttributeExpr, path string) []*Parameter {
if params == nil {
return nil
}
Expand All @@ -283,6 +284,9 @@ func paramsFromExpr(params *expr.MappedAttributeExpr, path string) []*Parameter
in = "path"
required = true
}
if endpoint != nil && in != "path" && openapiinternal.IsSecurityParameter(endpoint, in, pn) {
return nil
}
param := paramFor(at, pn, in, required)
res = append(res, param)
return nil
Expand All @@ -294,24 +298,14 @@ func paramsFromHeaders(endpoint *expr.HTTPEndpointExpr) []*Parameter {
var params []*Parameter

expr.WalkMappedAttr(endpoint.Headers, func(name, elem string, att *expr.AttributeExpr) error { // nolint: errcheck
if openapiinternal.IsSecurityParameter(endpoint, "header", elem) {
return nil
}
required := endpoint.Headers.IsRequiredNoDefault(name)
params = append(params, paramFor(att, elem, "header", required))
return nil
})

// Add basic auth to headers
if att := expr.TaggedAttribute(endpoint.MethodExpr.Payload, "security:username"); att != "" {
// Basic Auth is always encoded in the Authorization header
// https://golang.org/pkg/net/http/#Request.SetBasicAuth
params = append(params, &Parameter{
In: "header",
Name: "Authorization",
Required: endpoint.MethodExpr.Payload.IsRequired(att),
Description: "Basic Auth security using Basic scheme (https://tools.ietf.org/html/rfc7617)",
Type: "string",
})
}

return params
}

Expand Down Expand Up @@ -502,7 +496,7 @@ func buildPathFromExpr(s *V2, root *expr.RootExpr, h *expr.HostExpr, route *expr
// Remove any wildcards that is defined in path as a workaround to
// https://github.com/OAI/OpenAPI-Specification/issues/291
key = expr.HTTPWildcardRegex.ReplaceAllString(key, "/{$1}")
params := paramsFromExpr(endpoint.Params, key)
params := paramsFromExpr(endpoint, endpoint.Params, key)
params = append(params, paramsFromHeaders(endpoint)...)
var produces []string

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,33 +14,6 @@
"get": {
"description": "\n**Required security scopes for basic**:\n * `api:read`\n\n**Required security scopes for jwt**:\n * `api:read`\n\n**Required security scopes for api_key**:\n * `api:read`",
"operationId": "testService#testEndpointA",
"parameters": [
{
"in": "query",
"name": "k",
"required": true,
"type": "string"
},
{
"in": "header",
"name": "Token",
"required": true,
"type": "string"
},
{
"in": "header",
"name": "X-Authorization",
"required": true,
"type": "string"
},
{
"description": "Basic Auth security using Basic scheme (https://tools.ietf.org/html/rfc7617)",
"in": "header",
"name": "Authorization",
"required": true,
"type": "string"
}
],
"responses": {
"204": {
"description": "No Content response."
Expand All @@ -66,20 +39,6 @@
},
"post": {
"operationId": "testService#testEndpointB",
"parameters": [
{
"in": "query",
"name": "auth",
"required": true,
"type": "string"
},
{
"in": "header",
"name": "Authorization",
"required": true,
"type": "string"
}
],
"responses": {
"204": {
"description": "No Content response."
Expand Down
Loading
Loading