Repository: ogen-go/ogen
Pull request #452 (2022-07-04 03:02:21)

refactor(gen): move security validation to parser


Patch:
diff --git a/gen/gen_security.go b/gen/gen_security.go
index 7f710814..8021753b 100644
--- a/gen/gen_security.go
+++ b/gen/gen_security.go
@@ -69,7 +69,6 @@ func (g *Generator) generateSecurity(ctx *genctx, spec openapi.SecurityRequireme
 		return sec, nil
 	}
 	security := spec.Security
-	flows := security.Flows
 
 	typeName, err := pascalNonEmpty(spec.Name)
 	if err != nil {
@@ -99,29 +98,6 @@ func (g *Generator) generateSecurity(ctx *genctx, spec openapi.SecurityRequireme
 		}
 		fallthrough
 	case "oauth2":
-		checkScopes := func(flow *openapi.OAuthFlow) error {
-			if flow == nil {
-				return nil
-			}
-			for _, scope := range spec.Scopes {
-				if _, ok := flow.Scopes[scope]; !ok {
-					return errors.Errorf("unknown scope %q", scope)
-				}
-			}
-			return nil
-		}
-
-		for flowName, flow := range map[string]*openapi.OAuthFlow{
-			"implicit":          flows.Implicit,
-			"password":          flows.Password,
-			"clientCredentials": flows.ClientCredentials,
-			"authorizationCode": flows.AuthorizationCode,
-		} {
-			if err := checkScopes(flow); err != nil {
-				return nil, errors.Wrapf(err, "flow %q", flowName)
-			}
-		}
-
 		return nil, &ErrNotImplemented{Name: "oauth2 security"}
 	case "apiKey":
 		return g.generateSecurityAPIKey(s, spec)
diff --git a/openapi/parser/parse_security.go b/openapi/parser/parse_security.go
index b2588d30..2b82d44a 100644
--- a/openapi/parser/parse_security.go
+++ b/openapi/parser/parse_security.go
@@ -1,26 +1,135 @@
 package parser
 
 import (
+	"net/url"
+	"strings"
+
 	"github.com/go-faster/errors"
 
 	"github.com/ogen-go/ogen"
 	"github.com/ogen-go/ogen/openapi"
 )
 
-func (p *parser) parseSecuritySchema(s *ogen.SecuritySchema, ctx *resolveCtx) (*ogen.SecuritySchema, error) {
+func (p *parser) parseSecurityScheme(
+	s *ogen.SecurityScheme,
+	scopes []string,
+	ctx *resolveCtx,
+) (*ogen.SecurityScheme, error) {
 	if s == nil {
-		return nil, errors.New("securitySchema object is empty or null")
+		return nil, errors.New("securityScheme is empty or null")
 	}
+
 	if ref := s.Ref; ref != "" {
-		sch, err := p.resolveSecuritySchema(ref, ctx)
+		sch, err := p.resolveSecurityScheme(ref, ctx)
 		if err != nil {
 			return nil, errors.Wrap(err, "resolve security schema")
 		}
 		return sch, nil
 	}
+
+	if err := func() error {
+		switch s.Type {
+		case "apiKey":
+			switch s.In {
+			case "query", "header", "cookie":
+			default:
+				return errors.Errorf(`invalid "in": %q`, s.In)
+			}
+			if s.Name == "" {
+				return errors.New(`"name" is required and MUST be a non-empty string`)
+			}
+			return nil
+		case "http":
+			// FIXME(tdakkota): spec is not clear about this, it says
+			// 	`The values used SHOULD be registered in the IANA Authentication Scheme registry.`
+			// 	Probably such validation is too strict.
+
+			// Values from https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml.
+			switch strings.ToLower(s.Scheme) {
+			case "basic",
+				"bearer",
+				"digest",
+				"hoba",
+				"mutual",
+				"negotiate",
+				"oauth",
+				"scram-sha-1",
+				"scram-sha-256",
+				"vapid":
+			default:
+				return errors.Errorf(`invalid "scheme": %q`, s.Scheme)
+			}
+			return nil
+		case "mutualTLS":
+			return nil
+		case "oauth2":
+			return validateOAuthFlows(scopes, s.Flows)
+		case "openIdConnect":
+			if _, err := url.ParseRequestURI(s.OpenIDConnectURL); err != nil {
+				return errors.Wrap(err, `"openIdConnectUrl" MUST be in the form of a URL`)
+			}
+			return nil
+		default:
+			return errors.Errorf("unknown security scheme type %q", s.Type)
+		}
+	}(); err != nil {
+		return nil, errors.Wrap(err, s.Type)
+	}
+
 	return s, nil
 }
 
+func validateOAuthFlows(scopes []string, flows *ogen.OAuthFlows) error {
+	if flows == nil {
+		return errors.New("oAuthFlows is empty or null")
+	}
+
+	check := func(flow *ogen.OAuthFlow, authURL, tokenURL bool) error {
+		if flow == nil {
+			return nil
+		}
+
+		if tokenURL {
+			if _, err := url.ParseRequestURI(flow.TokenURL); err != nil {
+				return errors.Wrap(err, `"tokenUrl" MUST be in the form of a URL`)
+			}
+		}
+		if authURL {
+			if _, err := url.ParseRequestURI(flow.AuthorizationURL); err != nil {
+				return errors.Wrap(err, `"authorizationUrl" MUST be in the form of a URL`)
+			}
+		}
+		if flow.RefreshURL != "" {
+			if _, err := url.ParseRequestURI(flow.RefreshURL); err != nil {
+				return errors.Wrap(err, `"refreshUrl" MUST be in the form of a URL`)
+			}
+		}
+
+		for _, scope := range scopes {
+			if _, ok := flow.Scopes[scope]; !ok {
+				return errors.Errorf("unknown scope %q", scope)
+			}
+		}
+		return nil
+	}
+
+	for flowName, v := range map[string]struct {
+		flow              *ogen.OAuthFlow
+		authURL, tokenURL bool
+	}{
+		"implicit":          {flows.Implicit, true, false},
+		"password":          {flows.Password, false, true},
+		"clientCredentials": {flows.ClientCredentials, false, true},
+		"authorizationCode": {flows.AuthorizationCode, true, true},
+	} {
+		if err := check(v.flow, v.authURL, v.tokenURL); err != nil {
+			return errors.Wrapf(err, "flow %q", flowName)
+		}
+	}
+
+	return nil
+}
+
 func cloneOAuthFlows(flows ogen.OAuthFlows) (r openapi.OAuthFlows) {
 	cloneFlow := func(flow *ogen.OAuthFlow) *openapi.OAuthFlow {
 		if flow == nil {
@@ -55,9 +164,9 @@ func (p *parser) parseSecurityRequirements(requirements ogen.SecurityRequirement
 				return nil, errors.Errorf("unknown security schema %q", requirementName)
 			}
 
-			spec, err := p.parseSecuritySchema(v, newResolveCtx(p.depthLimit))
+			spec, err := p.parseSecurityScheme(v, scopes, newResolveCtx(p.depthLimit))
 			if err != nil {
-				return nil, errors.Wrapf(err, "resolve %q", requirementName)
+				return nil, errors.Wrapf(err, "parse security scheme %q", requirementName)
 			}
 
 			var flows ogen.OAuthFlows
diff --git a/openapi/parser/parser.go b/openapi/parser/parser.go
index e10fbad6..c1dfa962 100644
--- a/openapi/parser/parser.go
+++ b/openapi/parser/parser.go
@@ -20,7 +20,7 @@ type parser struct {
 		parameters      map[string]*openapi.Parameter
 		headers         map[string]*openapi.Header
 		examples        map[string]*openapi.Example
-		securitySchemes map[string]*ogen.SecuritySchema
+		securitySchemes map[string]*ogen.SecurityScheme
 	}
 
 	external   jsonschema.ExternalResolver
@@ -44,14 +44,14 @@ func Parse(spec *ogen.Spec, s Settings) (*openapi.API, error) {
 			parameters      map[string]*openapi.Parameter
 			headers         map[string]*openapi.Header
 			examples        map[string]*openapi.Example
-			securitySchemes map[string]*ogen.SecuritySchema
+			securitySchemes map[string]*ogen.SecurityScheme
 		}{
 			requestBodies:   map[string]*openapi.RequestBody{},
 			responses:       map[string]*openapi.Response{},
 			parameters:      map[string]*openapi.Parameter{},
 			headers:         map[string]*openapi.Header{},
 			examples:        map[string]*openapi.Example{},
-			securitySchemes: map[string]*ogen.SecuritySchema{},
+			securitySchemes: map[string]*ogen.SecurityScheme{},
 		},
 		external: s.External,
 		schemas: map[string][]byte{
diff --git a/openapi/parser/resolve.go b/openapi/parser/resolve.go
index 8bc568da..251986bb 100644
--- a/openapi/parser/resolve.go
+++ b/openapi/parser/resolve.go
@@ -256,7 +256,7 @@ func (p *parser) resolveExample(ref string, ctx *resolveCtx) (*openapi.Example,
 	return ex, nil
 }
 
-func (p *parser) resolveSecuritySchema(ref string, ctx *resolveCtx) (*ogen.SecuritySchema, error) {
+func (p *parser) resolveSecurityScheme(ref string, ctx *resolveCtx) (*ogen.SecurityScheme, error) {
 	const prefix = "#/components/securitySchemes/"
 
 	if r, ok := p.refs.securitySchemes[ref]; ok {
@@ -271,7 +271,7 @@ func (p *parser) resolveSecuritySchema(ref string, ctx *resolveCtx) (*ogen.Secur
 		ctx.delete(key)
 	}()
 
-	var component *ogen.SecuritySchema
+	var component *ogen.SecurityScheme
 	if key.loc == "" && ctx.lastLoc() == "" {
 		name := strings.TrimPrefix(ref, prefix)
 		c, found := p.spec.Components.SecuritySchemes[name]
diff --git a/security_spec.go b/security_spec.go
index 0847d7d7..63d33cd7 100644
--- a/security_spec.go
+++ b/security_spec.go
@@ -1,7 +1,7 @@
 package ogen
 
-// SecuritySchema defines a security scheme that can be used by the operations.
-type SecuritySchema struct {
+// SecurityScheme defines a security scheme that can be used by the operations.
+type SecurityScheme struct {
 	Ref string `json:"ref,omitempty"`
 	// The type of the security scheme. Valid values are "apiKey", "http", "mutualTLS", "oauth2", "openIdConnect".
 	Type string `json:"type"`
@@ -38,13 +38,17 @@ type OAuthFlows struct {
 
 // OAuthFlow is configuration details for a supported OAuth Flow.
 type OAuthFlow struct {
-	// The authorization URL to be used for this flow. This MUST be in the form of a URL. The OAuth2 standard requires the use of TLS.
+	// The authorization URL to be used for this flow.
+	// This MUST be in the form of a URL. The OAuth2 standard requires the use of TLS.
 	AuthorizationURL string `json:"authorizationUrl"`
-	// The token URL to be used for this flow. This MUST be in the form of a URL. The OAuth2 standard requires the use of TLS.
+	// The token URL to be used for this flow.
+	// This MUST be in the form of a URL. The OAuth2 standard requires the use of TLS.
 	TokenURL string `json:"tokenUrl"`
-	// The URL to be used for obtaining refresh tokens. This MUST be in the form of a URL. The OAuth2 standard requires the use of TLS.
+	// The URL to be used for obtaining refresh tokens.
+	// This MUST be in the form of a URL. The OAuth2 standard requires the use of TLS.
 	RefreshURL string `json:"refreshUrl,omitempty"`
-	// The available scopes for the OAuth2 security scheme. A map between the scope name and a short description for it. The map MAY be empty.
+	// The available scopes for the OAuth2 security scheme.
+	// A map between the scope name and a short description for it. The map MAY be empty.
 	Scopes map[string]string `json:"scopes"`
 }
 
diff --git a/spec.go b/spec.go
index 0737fc9d..45f1489f 100644
--- a/spec.go
+++ b/spec.go
@@ -171,7 +171,7 @@ type Components struct {
 	Headers         map[string]*Header         `json:"headers,omitempty"`
 	Examples        map[string]*Example        `json:"examples,omitempty"`
 	RequestBodies   map[string]*RequestBody    `json:"requestBodies,omitempty"`
-	SecuritySchemes map[string]*SecuritySchema `json:"securitySchemes,omitempty"`
+	SecuritySchemes map[string]*SecurityScheme `json:"securitySchemes,omitempty"`
 	// Links           map[string]Link            `json:"links"`
 	// Callbacks       map[string]Callback        `json:"callback"`
 }


Test patch:
diff --git a/_testdata/negative/bad_http_code.json b/_testdata/negative/invalid_http_code.json
similarity index 100%
rename from _testdata/negative/bad_http_code.json
rename to _testdata/negative/invalid_http_code.json
diff --git a/_testdata/negative/invalid_securityScheme_name.json b/_testdata/negative/invalid_securityScheme_name.json
new file mode 100644
index 00000000..86b137cf
--- /dev/null
+++ b/_testdata/negative/invalid_securityScheme_name.json
@@ -0,0 +1,40 @@
+{
+  "openapi": "3.0.0",
+  "info": {
+    "title": "title",
+    "version": "v0.1.0"
+  },
+  "paths": {
+    "/foo": {
+      "get": {
+        "security": [
+          {
+            "api_key": [
+              "read"
+            ]
+          }
+        ],
+        "responses": {
+          "default": {
+            "description": "default",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "object"
+                }
+              }
+            }
+          }
+        }
+      }
+    }
+  },
+  "components": {
+    "securitySchemes": {
+      "api_key": {
+        "type": "apiKey",
+        "in": "query"
+      }
+    }
+  }
+}
diff --git a/_testdata/negative/null_securitySchema.json b/_testdata/negative/null_securityScheme.json
similarity index 100%
rename from _testdata/negative/null_securitySchema.json
rename to _testdata/negative/null_securityScheme.json
diff --git a/_testdata/negative/null_securityScheme_flows.json b/_testdata/negative/null_securityScheme_flows.json
new file mode 100644
index 00000000..13f60abb
--- /dev/null
+++ b/_testdata/negative/null_securityScheme_flows.json
@@ -0,0 +1,40 @@
+{
+  "openapi": "3.0.0",
+  "info": {
+    "title": "title",
+    "version": "v0.1.0"
+  },
+  "paths": {
+    "/foo": {
+      "get": {
+        "security": [
+          {
+            "api_key": [
+              "read"
+            ]
+          }
+        ],
+        "responses": {
+          "default": {
+            "description": "default",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "object"
+                }
+              }
+            }
+          }
+        }
+      }
+    }
+  },
+  "components": {
+    "securitySchemes": {
+      "api_key": {
+        "type": "oauth2",
+        "flows": null
+      }
+    }
+  }
+}
diff --git a/_testdata/negative/unknown_oauth2_scope.json b/_testdata/negative/unknown_oauth2_scope.json
new file mode 100644
index 00000000..9be14eb2
--- /dev/null
+++ b/_testdata/negative/unknown_oauth2_scope.json
@@ -0,0 +1,47 @@
+{
+  "openapi": "3.0.0",
+  "info": {
+    "title": "title",
+    "version": "v0.1.0"
+  },
+  "paths": {
+    "/foo": {
+      "get": {
+        "security": [
+          {
+            "api_key": [
+              "read"
+            ]
+          }
+        ],
+        "responses": {
+          "default": {
+            "description": "default",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "object"
+                }
+              }
+            }
+          }
+        }
+      }
+    }
+  },
+  "components": {
+    "securitySchemes": {
+      "api_key": {
+        "type": "oauth2",
+        "flows": {
+          "implicit": {
+            "authorizationUrl": "https://example.com/oauth/authorize",
+            "scopes": {
+              "foobar": "read access"
+            }
+          }
+        }
+      }
+    }
+  }
+}
diff --git a/_testdata/negative/unknown_securityScheme_in.json b/_testdata/negative/unknown_securityScheme_in.json
new file mode 100644
index 00000000..75e6e052
--- /dev/null
+++ b/_testdata/negative/unknown_securityScheme_in.json
@@ -0,0 +1,40 @@
+{
+  "openapi": "3.0.0",
+  "info": {
+    "title": "title",
+    "version": "v0.1.0"
+  },
+  "paths": {
+    "/foo": {
+      "get": {
+        "security": [
+          {
+            "api_key": [
+              "read"
+            ]
+          }
+        ],
+        "responses": {
+          "default": {
+            "description": "default",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "object"
+                }
+              }
+            }
+          }
+        }
+      }
+    }
+  },
+  "components": {
+    "securitySchemes": {
+      "api_key": {
+        "type": "apiKey",
+        "in": "amongus"
+      }
+    }
+  }
+}
diff --git a/_testdata/negative/unknown_securityScheme_scheme.json b/_testdata/negative/unknown_securityScheme_scheme.json
new file mode 100644
index 00000000..030c0fab
--- /dev/null
+++ b/_testdata/negative/unknown_securityScheme_scheme.json
@@ -0,0 +1,40 @@
+{
+  "openapi": "3.0.0",
+  "info": {
+    "title": "title",
+    "version": "v0.1.0"
+  },
+  "paths": {
+    "/foo": {
+      "get": {
+        "security": [
+          {
+            "api_key": [
+              "read"
+            ]
+          }
+        ],
+        "responses": {
+          "default": {
+            "description": "default",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "object"
+                }
+              }
+            }
+          }
+        }
+      }
+    }
+  },
+  "components": {
+    "securitySchemes": {
+      "api_key": {
+        "type": "http",
+        "scheme": "amongus"
+      }
+    }
+  }
+}
diff --git a/_testdata/negative/unknown_securityScheme_type.json b/_testdata/negative/unknown_securityScheme_type.json
new file mode 100644
index 00000000..e4374ead
--- /dev/null
+++ b/_testdata/negative/unknown_securityScheme_type.json
@@ -0,0 +1,37 @@
+{
+  "openapi": "3.0.0",
+  "info": {
+    "title": "title",
+    "version": "v0.1.0"
+  },
+  "paths": {
+    "/foo": {
+      "get": {
+        "security": [
+          {
+            "api_key": []
+          }
+        ],
+        "responses": {
+          "default": {
+            "description": "default",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "object"
+                }
+              }
+            }
+          }
+        }
+      }
+    }
+  },
+  "components": {
+    "securitySchemes": {
+      "api_key": {
+        "type": "amongus"
+      }
+    }
+  }
+}
diff --git a/openapi/parser/parse_security_test.go b/openapi/parser/parse_security_test.go
new file mode 100644
index 00000000..e20939a0
--- /dev/null
+++ b/openapi/parser/parse_security_test.go
@@ -0,0 +1,84 @@
+package parser
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/ogen-go/ogen"
+)
+
+func Test_validateOAuthFlows(t *testing.T) {
+	implicit := func(flow ogen.OAuthFlow) *ogen.OAuthFlows {
+		return &ogen.OAuthFlows{Implicit: &flow}
+	}
+	authCode := func(flow ogen.OAuthFlow) *ogen.OAuthFlows {
+		return &ogen.OAuthFlows{AuthorizationCode: &flow}
+	}
+	password := func(flow ogen.OAuthFlow) *ogen.OAuthFlows {
+		return &ogen.OAuthFlows{Password: &flow}
+	}
+	clientCreds := func(flow ogen.OAuthFlow) *ogen.OAuthFlows {
+		return &ogen.OAuthFlows{ClientCredentials: &flow}
+	}
+
+	tests := []struct {
+		scopes  []string
+		flows   *ogen.OAuthFlows
+		wantErr bool
+	}{
+		// OAuthFlows is required.
+		{nil, nil, true},
+		{nil, new(ogen.OAuthFlows), false},
+
+		// `implicit` requires `authorizationUrl`.
+		{nil, implicit(ogen.OAuthFlow{}), true},
+		// `authCode` requires `authorizationUrl` and `tokenUrl`.
+		{nil, authCode(ogen.OAuthFlow{}), true},
+		// `password` requires `tokenUrl`.
+		{nil, password(ogen.OAuthFlow{}), true},
+		// `clientCredentials` requires `tokenUrl`.
+		{nil, clientCreds(ogen.OAuthFlow{}), true},
+
+		// `authorizationUrl` must be a valid URL.
+		{nil, implicit(ogen.OAuthFlow{
+			AuthorizationURL: "-",
+		}), true},
+		// `tokenUrl` must be a valid URL.
+		{nil, password(ogen.OAuthFlow{
+			TokenURL: "-",
+		}), true},
+
+		// `refreshUrl` must be a valid URL.
+		{nil, implicit(ogen.OAuthFlow{
+			AuthorizationURL: "https://example.com/authorization",
+			RefreshURL:       "-",
+		}), true},
+
+		// `unknown_scope` must be defined in `flows`.
+		{[]string{"unknown_scope"}, implicit(ogen.OAuthFlow{
+			AuthorizationURL: "https://example.com/authorization",
+			RefreshURL:       "https://example.com/refresh",
+		}), true},
+		{[]string{"unknown_scope"}, implicit(ogen.OAuthFlow{
+			AuthorizationURL: "https://example.com/authorization",
+			RefreshURL:       "https://example.com/refresh",
+			Scopes: map[string]string{
+				"unknown_scope": "description",
+			},
+		}), false},
+	}
+	for i, tt := range tests {
+		tt := tt
+		t.Run(fmt.Sprintf("Test%d", i+1), func(t *testing.T) {
+			a := require.New(t)
+			err := validateOAuthFlows(tt.scopes, tt.flows)
+			if tt.wantErr {
+				a.Error(err)
+				return
+			}
+			a.NoError(err)
+		})
+	}
+}
diff --git a/openapi/parser/resolve_test.go b/openapi/parser/resolve_test.go
index 146e935f..90861901 100644
--- a/openapi/parser/resolve_test.go
+++ b/openapi/parser/resolve_test.go
@@ -78,9 +78,9 @@ func TestExternalReference(t *testing.T) {
 					Ref: "foo.json#/components/requestBodies/RemoteRequestBody",
 				},
 			},
-			SecuritySchemes: map[string]*ogen.SecuritySchema{
-				"LocalSecuritySchema": {
-					Ref: "foo.json#/components/securitySchemes/RemoteSecuritySchema",
+			SecuritySchemes: map[string]*ogen.SecurityScheme{
+				"LocalSecurityScheme": {
+					Ref: "foo.json#/components/securitySchemes/RemoteSecurityScheme",
 				},
 			},
 		},