Repository: goadesign/goa
Pull request #2353 (2019-11-02 03:13:49)

Validate a method when no security scheme but a security attribute
This is backport of #2351 to v3.

raphael: Thank you!

Patch:
diff --git a/expr/attribute.go b/expr/attribute.go
index 7217bd4a..d4bafe76 100644
--- a/expr/attribute.go
+++ b/expr/attribute.go
@@ -2,6 +2,7 @@ package expr
 
 import (
 	"fmt"
+	"strings"
 
 	"goa.design/goa/v3/eval"
 )
@@ -389,6 +390,26 @@ func (a *AttributeExpr) HasTag(tag string) bool {
 	return false
 }
 
+// HasTagPrefix returns true if the attribute is an object that has an attribute with
+// the given tag prefix.
+func (a *AttributeExpr) HasTagPrefix(prefix string) bool {
+	if a == nil {
+		return false
+	}
+	obj := AsObject(a.Type)
+	if obj == nil {
+		return false
+	}
+	for _, at := range *obj {
+		for k := range at.Attribute.Meta {
+			if strings.HasPrefix(k, prefix) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // FieldTag returns the field tag if the attribute is a field.
 func (a *AttributeExpr) FieldTag() (tag string, found bool) {
 	if a == nil {
diff --git a/expr/method.go b/expr/method.go
index 49d212a1..ada71ddd 100644
--- a/expr/method.go
+++ b/expr/method.go
@@ -107,11 +107,18 @@ func (m *MethodExpr) Validate() error {
 	} else if len(m.Service.Requirements) > 0 {
 		requirements = m.Service.Requirements
 	}
+	var (
+		hasBasicAuth bool
+		hasAPIKey    bool
+		hasJWT       bool
+		hasOAuth     bool
+	)
 	for _, r := range requirements {
 		for _, s := range r.Schemes {
 			verr.Merge(s.Validate())
 			switch s.Kind {
 			case BasicAuthKind:
+				hasBasicAuth = true
 				if !hasTag(m.Payload, "security:username") {
 					verr.Add(m, "payload of method %q of service %q does not define a username attribute, use Username to define one", m.Name, m.Service.Name)
 				}
@@ -119,14 +126,17 @@ func (m *MethodExpr) Validate() error {
 					verr.Add(m, "payload of method %q of service %q does not define a password attribute, use Password to define one", m.Name, m.Service.Name)
 				}
 			case APIKeyKind:
+				hasAPIKey = true
 				if !hasTag(m.Payload, "security:apikey:"+s.SchemeName) {
 					verr.Add(m, "payload of method %q of service %q does not define an API key attribute, use APIKey to define one", m.Name, m.Service.Name)
 				}
 			case JWTKind:
+				hasJWT = true
 				if !hasTag(m.Payload, "security:token") {
 					verr.Add(m, "payload of method %q of service %q does not define a JWT attribute, use Token to define one", m.Name, m.Service.Name)
 				}
 			case OAuth2Kind:
+				hasOAuth = true
 				if !hasTag(m.Payload, "security:accesstoken") {
 					verr.Add(m, "payload of method %q of service %q does not define a OAuth2 access token attribute, use AccessToken to define one", m.Name, m.Service.Name)
 				}
@@ -149,6 +159,29 @@ func (m *MethodExpr) Validate() error {
 			}
 		}
 	}
+	if !hasBasicAuth {
+		if hasTag(m.Payload, "security:username") {
+			verr.Add(m, "payload of method %q of service %q defines a username attribute, but no basic auth security scheme exist", m.Name, m.Service.Name)
+		}
+		if hasTag(m.Payload, "security:password") {
+			verr.Add(m, "payload of method %q of service %q defines a password attribute, but no basic auth security scheme exist", m.Name, m.Service.Name)
+		}
+	}
+	if !hasAPIKey {
+		if hasTagPrefix(m.Payload, "security:apikey") {
+			verr.Add(m, "payload of method %q of service %q defines an API key attribute, but no APIKey security scheme exist", m.Name, m.Service.Name)
+		}
+	}
+	if !hasJWT {
+		if hasTag(m.Payload, "security:token") {
+			verr.Add(m, "payload of method %q of service %q defines a JWT token attribute, but no JWT auth security scheme exist", m.Name, m.Service.Name)
+		}
+	}
+	if !hasOAuth {
+		if hasTag(m.Payload, "security:accesstoken") {
+			verr.Add(m, "payload of method %q of service %q defines a OAuth2 access token attribute, but no OAuth2 security scheme exist", m.Name, m.Service.Name)
+		}
+	}
 	if m.StreamingPayload.Type != Empty {
 		verr.Merge(m.StreamingPayload.Validate("streaming_payload", m))
 	}
@@ -203,6 +236,26 @@ func hasTag(p *AttributeExpr, tag string) bool {
 	return false
 }
 
+// hasTag is a helper function that traverses the given attribute and all its
+// bases recursively looking for an attribute with the given tag meta prefix. This
+// recursion is only needed for attributes that have not been finalized yet.
+func hasTagPrefix(p *AttributeExpr, prefix string) bool {
+	if p.HasTagPrefix(prefix) {
+		return true
+	}
+	for _, base := range p.Bases {
+		ut, ok := base.(UserType)
+		if !ok {
+			continue
+		}
+		return hasTagPrefix(ut.Attribute(), prefix)
+	}
+	if ut, ok := p.Type.(UserType); ok {
+		return hasTagPrefix(ut.Attribute(), prefix)
+	}
+	return false
+}
+
 // Finalize makes sure the method payload and result types are set. It also
 // projects the result if it is a result type and a view is explicitly set in
 // the design or a result type having at most one view.


Test patch:
diff --git a/expr/attribute_test.go b/expr/attribute_test.go
index 0639e898..b54932bf 100644
--- a/expr/attribute_test.go
+++ b/expr/attribute_test.go
@@ -613,6 +613,64 @@ func TestAttributeExprHasTag(t *testing.T) {
 	}
 }
 
+func TestAttributeExprHasTagPrefix(t *testing.T) {
+	var (
+		tag    = "security:apikey:api_key"
+		prefix = "security:apikey"
+	)
+	cases := map[string]struct {
+		attribute *AttributeExpr
+		prefix    string
+		expected  bool
+	}{
+		"has tag prefix": {
+			attribute: &AttributeExpr{
+				Type: &Object{
+					&NamedAttributeExpr{
+						Name: "foo",
+						Attribute: &AttributeExpr{
+							Meta: MetaExpr{
+								tag: []string{"key"},
+							},
+						},
+					},
+				},
+			},
+			prefix:   prefix,
+			expected: true,
+		},
+		"attribute expr is nil": {
+			attribute: nil,
+			prefix:    prefix,
+			expected:  false,
+		},
+		"not object": {
+			attribute: &AttributeExpr{
+				Type: String,
+			},
+			prefix:   prefix,
+			expected: false,
+		},
+		"object but has no tag": {
+			attribute: &AttributeExpr{
+				Type: &Object{
+					&NamedAttributeExpr{
+						Name:      "foo",
+						Attribute: &AttributeExpr{},
+					},
+				},
+			},
+			prefix: prefix,
+		},
+	}
+
+	for k, tc := range cases {
+		if actual := tc.attribute.HasTagPrefix(tc.prefix); tc.expected != actual {
+			t.Errorf("%s: got %#v, expected %#v", k, actual, tc.expected)
+		}
+	}
+}
+
 func TestAttributeExprHasDefaultValue(t *testing.T) {
 	var (
 		object = &Object{
diff --git a/expr/method_test.go b/expr/method_test.go
index 423b2a18..0d97b847 100644
--- a/expr/method_test.go
+++ b/expr/method_test.go
@@ -24,7 +24,12 @@ flow authorization_code: invalid authorization URL "http://^authorization": pars
 flow authorization_code: invalid refresh URL "http://refresh^": parse http://refresh^: invalid character "^" in host name
 service "InvalidSecuritySchemesService" method "InheritedSecureMethod": payload of method "InheritedSecureMethod" of service "InvalidSecuritySchemesService" does not define a OAuth2 access token attribute, use AccessToken to define one
 service "InvalidSecuritySchemesService" method "InheritedSecureMethod": payload of method "InheritedSecureMethod" of service "InvalidSecuritySchemesService" does not define an API key attribute, use APIKey to define one
-service "InvalidSecuritySchemesService" method "InheritedSecureMethod": security scope "not:found" not found in any of the security schemes.`,
+service "InvalidSecuritySchemesService" method "InheritedSecureMethod": security scope "not:found" not found in any of the security schemes.
+service "AnotherInvalidSecuritySchemesService" method "Method": payload of method "Method" of service "AnotherInvalidSecuritySchemesService" defines a username attribute, but no basic auth security scheme exist
+service "AnotherInvalidSecuritySchemesService" method "Method": payload of method "Method" of service "AnotherInvalidSecuritySchemesService" defines a password attribute, but no basic auth security scheme exist
+service "AnotherInvalidSecuritySchemesService" method "Method": payload of method "Method" of service "AnotherInvalidSecuritySchemesService" defines an API key attribute, but no APIKey security scheme exist
+service "AnotherInvalidSecuritySchemesService" method "Method": payload of method "Method" of service "AnotherInvalidSecuritySchemesService" defines a JWT token attribute, but no JWT auth security scheme exist
+service "AnotherInvalidSecuritySchemesService" method "Method": payload of method "Method" of service "AnotherInvalidSecuritySchemesService" defines a OAuth2 access token attribute, but no OAuth2 security scheme exist`,
 		},
 	}
 	for _, tc := range cases {
diff --git a/expr/testdata/endpoint_dsls.go b/expr/testdata/endpoint_dsls.go
index 30c587a8..2837e10b 100644
--- a/expr/testdata/endpoint_dsls.go
+++ b/expr/testdata/endpoint_dsls.go
@@ -262,6 +262,8 @@ var FinalizeEndpointBodyAsExtendedTypeDSL = func() {
 }
 
 var FinalizeEndpointBodyAsPropWithExtendedTypeDSL = func() {
+	var OAuth2 = OAuth2Security("authCode")
+
 	var EntityData = Type("EntityData", func() {
 		Attribute("name", String)
 	})
@@ -274,6 +276,7 @@ var FinalizeEndpointBodyAsPropWithExtendedTypeDSL = func() {
 
 	Service("Service", func() {
 		Method("Method", func() {
+			Security(OAuth2)
 			Payload(func() {
 				AccessToken("token", String)
 				Attribute("payload", Entity)
diff --git a/expr/testdata/method_validate_dsls.go b/expr/testdata/method_validate_dsls.go
index 35e9612a..96f34d75 100644
--- a/expr/testdata/method_validate_dsls.go
+++ b/expr/testdata/method_validate_dsls.go
@@ -39,4 +39,16 @@ var InvalidSecuritySchemesDSL = func() {
 			})
 		})
 	})
+	Service("AnotherInvalidSecuritySchemesService", func() {
+		Method("Method", func() {
+			Payload(func() {
+				Username("user", String)
+				Password("pass", String)
+				APIKey("key_key", "key", String)
+				Token("token", String)
+				AccessToken("access_token", String)
+			})
+			// invalid: missing security scheme
+		})
+	})
 }