Repository: goadesign/goa
Pull request #2532 (2020-04-14 03:34:18)

V3: Fix generated endpoints when using SkipRequestBodyEncodeDecode with BasicAuth
Fixed using incorrect type on the generated endpoints when using SkipRequestBodyEncodeDecode() with BasicAuth. 

@raphael and @nitinmohan87, 
When I added a test for JWTAuth, it returned
`HTTP endpoint request body must be empty when using SkipRequestBodyEncodeDecode but not all method payload attributes are mapped to headers and params`

Just want to verify if other authentication types are supported?



Patch:
diff --git a/Makefile b/Makefile
index 5751c61c..201f0517 100644
--- a/Makefile
+++ b/Makefile
@@ -70,7 +70,7 @@ ifneq ($(GOOS),windows)
 	@if [ "`golint ./... | grep -vf .golint_exclude | tee /dev/stderr`" ]; then \
 		echo "^ - Lint errors!" && echo && exit 1; \
 	fi
-	@if [ "`staticcheck -checks all ./... | grep -v ".pb.go" | tee /dev/stderr`" ]; then \
+	@if [ "`staticcheck -checks all ./... | grep -v ".pb.go" | grep -v "SA1019" | tee /dev/stderr`" ]; then \
 		echo "^ - staticcheck errors!" && echo && exit 1; \
 	fi
 endif
diff --git a/README.md b/README.md
index 3de59b27..1653fbe8 100644
--- a/README.md
+++ b/README.md
@@ -4,7 +4,7 @@ Goa is a framework for building micro-services and APIs in Go using a unique
 design-first approach.
 
 ---
-[![Build Status](https://github.com/goadesign/goa/workflows/build/badge.svg?branch=v3)](https://github.com/goadesign/goa/actions?query=branch%3Av3)
+[![Build Status](https://github.com/goadesign/goa/workflows/build/badge.svg?branch=v3&event=push)](https://github.com/goadesign/goa/actions?query=branch%3Av3+event%3Apush)
 [![Godoc Packages](https://img.shields.io/badge/godoc-packages-blue)](https://pkg.go.dev/mod/goa.design/goa/v3?tab=packages)
 [![Godoc DSL](https://img.shields.io/badge/godoc-DSL-blue)](https://pkg.go.dev/goa.design/goa/v3@v3.0.8/dsl?tab=doc)
 [![Slack](https://img.shields.io/badge/slack-gophers-orange.svg?style=flat)](https://gophers.slack.com/messages/goa/)
diff --git a/codegen/service/endpoint.go b/codegen/service/endpoint.go
index f85ef8e7..bce7862d 100644
--- a/codegen/service/endpoint.go
+++ b/codegen/service/endpoint.go
@@ -155,7 +155,7 @@ func endpointData(service *expr.ServiceExpr) *endpointsData {
 }
 
 func payloadVar(e *endpointMethodData) string {
-	if e.ServerStream != nil {
+	if e.ServerStream != nil || e.SkipRequestBodyEncodeDecode {
 		return "ep.Payload"
 	}
 	return "p"
diff --git a/expr/http_body_types.go b/expr/http_body_types.go
index 537a02d0..e6dcc43d 100644
--- a/expr/http_body_types.go
+++ b/expr/http_body_types.go
@@ -6,6 +6,54 @@ import (
 	"unicode"
 )
 
+// defaultRequestHeaderAttributes returns a map keyed by the names of the
+// payload attributes that should come from the request HTTP headers by default.
+// This includes mapping done for certain authorization schemes (basic auth,
+// JWT, OAuth). The corresponding boolean value indicates whether the value maps
+// directly to a payload attribute (true) or whether the value is used to
+// compute the payload attribute (false). The only case where the value is
+// computed by the generated code at this point is for basic authorization (the
+// single "Authorization" header is used to compute both the username and
+// password attributes).
+func defaultRequestHeaderAttributes(e *HTTPEndpointExpr) map[string]bool {
+	if len(e.MethodExpr.Requirements) == 0 {
+		return nil
+	}
+	headers := make(map[string]bool)
+	for _, req := range e.MethodExpr.Requirements {
+		for _, sch := range req.Schemes {
+			var field string
+			switch sch.Kind {
+			case NoKind:
+				continue
+			case BasicAuthKind:
+				user := TaggedAttribute(e.MethodExpr.Payload, "security:username")
+				if name, _ := findKey(e, user); name == "" {
+					// No explicit mapping, use HTTP header by default
+					headers[user] = false
+				}
+				pass := TaggedAttribute(e.MethodExpr.Payload, "security:password")
+				if name, _ := findKey(e, pass); name == "" {
+					// No explicit mapping, use HTTP header by default
+					headers[pass] = false
+				}
+				continue
+			case APIKeyKind:
+				field = TaggedAttribute(e.MethodExpr.Payload, "security:apikey:"+sch.SchemeName)
+			case JWTKind:
+				field = TaggedAttribute(e.MethodExpr.Payload, "security:token")
+			case OAuth2Kind:
+				field = TaggedAttribute(e.MethodExpr.Payload, "security:accesstoken")
+			}
+			if name, _ := findKey(e, field); name == "" {
+				// No explicit mapping, use HTTP header by default
+				headers[field] = true
+			}
+		}
+	}
+	return headers
+}
+
 // httpRequestBody returns an attribute describing the HTTP request body of the
 // given endpoint. If the DSL defines a body explicitly via the Body function
 // then the corresponding attribute is used. Otherwise the attribute is computed
@@ -23,30 +71,11 @@ func httpRequestBody(a *HTTPEndpointExpr) *AttributeExpr {
 	}
 
 	var (
-		payload   = a.MethodExpr.Payload
-		headers   = a.Headers
-		params    = a.Params
-		userField string
-		passField string
+		payload  = a.MethodExpr.Payload
+		headers  = a.Headers
+		params   = a.Params
+		bodyOnly = headers.IsEmpty() && params.IsEmpty() && a.MapQueryParams == nil
 	)
-	{
-		obj := AsObject(payload.Type)
-		if obj != nil {
-			for _, at := range *obj {
-				if _, ok := at.Attribute.Meta["security:username"]; ok {
-					userField = at.Name
-				}
-				if _, ok := at.Attribute.Meta["security:password"]; ok {
-					passField = at.Name
-				}
-				if userField != "" && passField != "" {
-					break
-				}
-			}
-		}
-	}
-
-	bodyOnly := headers.IsEmpty() && params.IsEmpty() && a.MapQueryParams == nil
 
 	// 1. If Payload is not an object then check whether there are params or
 	// headers defined and if so return empty type (payload encoded in
@@ -68,11 +97,8 @@ func httpRequestBody(a *HTTPEndpointExpr) *AttributeExpr {
 	if a.MapQueryParams != nil && *a.MapQueryParams != "" {
 		removeAttribute(body, *a.MapQueryParams)
 	}
-	if userField != "" {
-		removeAttribute(body, userField)
-	}
-	if passField != "" {
-		removeAttribute(body, passField)
+	for att := range defaultRequestHeaderAttributes(a) {
+		removeAttribute(body, att)
 	}
 
 	// 3. Return empty type if no attribute left
@@ -140,9 +166,9 @@ func httpResponseBody(a *HTTPEndpointExpr, resp *HTTPResponseExpr) *AttributeExp
 // the corresponding attribute is returned. Otherwise the attribute is computed
 // by removing the attributes of the error used to define headers and
 // parameters.
-func httpErrorResponseBody(a *HTTPEndpointExpr, v *HTTPErrorExpr) *AttributeExpr {
-	name := a.Name() + "_" + v.ErrorExpr.Name
-	return buildHTTPResponseBody(name, v.ErrorExpr.AttributeExpr, v.Response, a.Service)
+func httpErrorResponseBody(e *HTTPEndpointExpr, v *HTTPErrorExpr) *AttributeExpr {
+	name := e.Name() + "_" + v.ErrorExpr.Name
+	return buildHTTPResponseBody(name, v.ErrorExpr.AttributeExpr, v.Response, e.Service)
 }
 
 func buildHTTPResponseBody(name string, attr *AttributeExpr, resp *HTTPResponseExpr, svc *HTTPServiceExpr) *AttributeExpr {
diff --git a/expr/http_endpoint.go b/expr/http_endpoint.go
index 99e6a3a5..52834bd0 100644
--- a/expr/http_endpoint.go
+++ b/expr/http_endpoint.go
@@ -577,6 +577,21 @@ func (e *HTTPEndpointExpr) Validate() error {
 func (e *HTTPEndpointExpr) Finalize() {
 	// Initialize Authorization header implicitly defined via security DSL
 	// prior to computing headers and body.
+	for att, mapped := range defaultRequestHeaderAttributes(e) {
+		if !mapped {
+			continue
+		}
+		attr := e.MethodExpr.Payload.Find(att)
+		e.Headers.Type.(*Object).Set(att, attr)
+		if e.MethodExpr.Payload.IsRequired(att) {
+			if e.Headers.Validation == nil {
+				e.Headers.Validation = &ValidationExpr{}
+			}
+			e.Headers.Validation.AddRequired(att)
+		}
+	}
+
+	// Compute security scheme attribute name and corresponding HTTP location
 	if reqLen := len(e.MethodExpr.Requirements); reqLen > 0 {
 		e.Requirements = make([]*SecurityExpr, 0, reqLen)
 		for _, req := range e.MethodExpr.Requirements {
@@ -600,15 +615,7 @@ func (e *HTTPEndpointExpr) Finalize() {
 				sch.Name, sch.In = findKey(e, field)
 				if sch.Name == "" {
 					sch.Name = "Authorization"
-					attr := e.MethodExpr.Payload.Find(field)
-					e.Headers.Type.(*Object).Set(field, attr)
-					e.Headers.Map(sch.Name, field)
-					if e.MethodExpr.Payload.IsRequired(field) {
-						if e.Headers.Validation == nil {
-							e.Headers.Validation = &ValidationExpr{}
-						}
-						e.Headers.Validation.AddRequired(field)
-					}
+					e.Headers.Map("Authorization", field)
 				}
 			}
 			e.Requirements = append(e.Requirements, dupReq)


Test patch:
diff --git a/codegen/service/security_test.go b/codegen/service/security_test.go
index 1d544903..0ce19345 100644
--- a/codegen/service/security_test.go
+++ b/codegen/service/security_test.go
@@ -70,3 +70,30 @@ func TestSecureEndpoint(t *testing.T) {
 		})
 	}
 }
+
+func TestSecureWithSkipRequestBodyEncodeDecode(t *testing.T) {
+	cases := []struct {
+		Name string
+		DSL  func()
+		Code string
+	}{
+		{"with-basicauth", testdata.EndpointWithBasicAuthAndSkipRequestBodyEncodeDecodeDSL, testdata.EndpointWithBasicAuthAndSkipRequestBodyEncodeDecodeCode},
+	}
+	for _, c := range cases {
+		t.Run(c.Name, func(t *testing.T) {
+			codegen.RunDSL(t, c.DSL)
+			if len(expr.Root.Services) != 1 {
+				t.Fatalf("got %d services, expected 1", len(expr.Root.Services))
+			}
+			fs := EndpointFile("", expr.Root.Services[0])
+			if fs == nil {
+				t.Fatalf("got nil file, expected not nil")
+			}
+			sections := fs.SectionTemplates
+			code := codegen.SectionCode(t, sections[5])
+			if code != c.Code {
+				t.Errorf("invalid code, got:\n%s\ngot vs. expected:\n%s", code, codegen.Diff(t, code, c.Code))
+			}
+		})
+	}
+}
diff --git a/codegen/service/testdata/security_dsls.go b/codegen/service/testdata/security_dsls.go
index 2127b6b3..669bc998 100644
--- a/codegen/service/testdata/security_dsls.go
+++ b/codegen/service/testdata/security_dsls.go
@@ -164,6 +164,22 @@ var EndpointWithOAuth2DSL = func() {
 	})
 }
 
+var EndpointWithBasicAuthAndSkipRequestBodyEncodeDecodeDSL = func() {
+	Service("EndpointWithSkipRequestBodyEncodeDecode", func() {
+		Method("EndpointWithSkipRequestBodyEncodeDecode", func() {
+			Security(BasicAuth)
+			Payload(func() {
+				Username("user", String)
+				Password("pass", String)
+			})
+			HTTP(func() {
+				SkipRequestBodyEncodeDecode()
+				GET("/")
+			})
+		})
+	})
+}
+
 var SingleServiceDSL = func() {
 	Service("SingleService", func() {
 		Method("Method", func() {
diff --git a/codegen/service/testdata/security_functions.go b/codegen/service/testdata/security_functions.go
index 03e57db3..4297b416 100644
--- a/codegen/service/testdata/security_functions.go
+++ b/codegen/service/testdata/security_functions.go
@@ -152,3 +152,32 @@ func NewSecureWithOAuth2Endpoint(s Service, authOAuth2Fn security.AuthOAuth2Func
 	}
 }
 `
+
+var EndpointWithBasicAuthAndSkipRequestBodyEncodeDecodeCode = `// NewEndpointWithSkipRequestBodyEncodeDecodeEndpoint returns an endpoint
+// function that calls the method "EndpointWithSkipRequestBodyEncodeDecode" of
+// service "EndpointWithSkipRequestBodyEncodeDecode".
+func NewEndpointWithSkipRequestBodyEncodeDecodeEndpoint(s Service, authBasicFn security.AuthBasicFunc) goa.Endpoint {
+	return func(ctx context.Context, req interface{}) (interface{}, error) {
+		ep := req.(*EndpointWithSkipRequestBodyEncodeDecodeRequestData)
+		var err error
+		sc := security.BasicScheme{
+			Name:           "basic",
+			Scopes:         []string{"api:read", "api:write", "api:admin"},
+			RequiredScopes: []string{},
+		}
+		var user string
+		if ep.Payload.User != nil {
+			user = *ep.Payload.User
+		}
+		var pass string
+		if ep.Payload.Pass != nil {
+			pass = *ep.Payload.Pass
+		}
+		ctx, err = authBasicFn(ctx, user, pass, &sc)
+		if err != nil {
+			return nil, err
+		}
+		return nil, s.EndpointWithSkipRequestBodyEncodeDecode(ctx, ep.Payload, ep.Body)
+	}
+}
+`