feat: Disallow construction of invalid FQMNs (#10)

suborbital · Aug 18, 2022 · 3dc432f · 3dc432f
1 parent 1b0fa92
commit 3dc432f
Show file tree

Hide file tree

Showing 5 changed files with 58 additions and 9 deletions.
diff --git a/appsource/server/serversource.go b/appsource/server/serversource.go
@@ -89,7 +89,12 @@ func (a *AppSourceVKRouter) GetModuleHandler() vk.HandlerFunc {
 		namespace := ctx.Params.ByName("namespace")
 		mod := ctx.Params.ByName("mod")
 
-		fqmnString := fqmn.FromParts(ident, namespace, mod, ref)
+		fqmnString, err := fqmn.FromParts(ident, namespace, mod, ref)
+		if err != nil {
+			ctx.Log.Error(errors.Wrap(err, "failed fqmn FromParts"))
+
+			return nil, vk.E(http.StatusInternalServerError, "something went wrong")
+		}
 
 		runnable, err := a.appSource.GetModule(fqmnString)
 		if err != nil {

diff --git a/fqmn/fqmn.go b/fqmn/fqmn.go
@@ -41,6 +41,7 @@ type FQMN struct {
 }
 
 var ErrFQMNParseFailure = errors.New("FQMN failed to parse")
+var ErrFQMNConstructionFailure = errors.New("All FQMN must be defined")
 
 var errWrongPrefix = errors.Wrap(ErrFQMNParseFailure, "FQMN must begin with 'fqmn://', '/name', or '/ref'")
 var errMustBeFullyQualified = errors.Wrap(ErrFQMNParseFailure, "FQMN text format must contain an tenant, ref, namespace, and module name")
@@ -200,6 +201,9 @@ func (f FQMN) URLPath() string {
 }
 
 // FromParts returns an FQMN from the provided parts
-func FromParts(tenant, namespace, module, ref string) string {
-	return fmt.Sprintf("fqmn://%s/%s/%s@%s", tenant, namespace, module, ref)
+func FromParts(tenant, namespace, module, ref string) (string, error) {
+	if tenant == "" || namespace == "" || module == "" || ref == "" {
+		return "", ErrFQMNConstructionFailure
+	}
+	return fmt.Sprintf("fqmn://%s/%s/%s@%s", tenant, namespace, module, ref), nil
 }
diff --git a/fqmn/fqmn_test.go b/fqmn/fqmn_test.go
@@ -141,9 +141,46 @@ func (s *FQMNSuite) TestFromParts() {
 			"fqmn://com.suborbital.something/default/foobar@asdf",
 			nil,
 		},
+		{
+			"",
+			"default",
+			"asdf",
+			"com.suborbital.something",
+			"",
+			ErrFQMNConstructionFailure,
+		},
+		{
+			"foobar",
+			"",
+			"asdf",
+			"com.suborbital.something",
+			"",
+			ErrFQMNConstructionFailure,
+		},
+		{
+			"foobar",
+			"default",
+			"",
+			"com.suborbital.something",
+			"",
+			ErrFQMNConstructionFailure,
+		},
+		{
+			"foobar",
+			"default",
+			"asdf",
+			"",
+			"",
+			ErrFQMNConstructionFailure,
+		},
 	} {
 		s.Run(tt.fqmn, func() {
-			fqmn := FromParts(tt.tenant, tt.namespace, tt.name, tt.ref)
+			fqmn, err := FromParts(tt.tenant, tt.namespace, tt.name, tt.ref)
+
+			if err != nil {
+				s.Assertions.ErrorIs(err, tt.error)
+				return
+			}
 
 			s.Assertions.Equal(fqmn, tt.fqmn)
 		})

diff --git a/tenant/config.go b/tenant/config.go
@@ -139,11 +139,14 @@ func (c *Config) calculateFQMNs() {
 			mod.Namespace = fqmn.NamespaceDefault
 		}
 
-		c.Modules[i].FQMN = c.FQMNForFunc(mod.Namespace, mod.Name, mod.Ref)
+		// We deliberately ignore returned errors.
+		// The module will not be runnable, but it's not a problem for the
+		// system as a whole.
+		c.Modules[i].FQMN, _ = c.FQMNForFunc(mod.Namespace, mod.Name, mod.Ref)
 	}
 }
 
-func (c *Config) FQMNForFunc(namespace, fn, ref string) string {
+func (c *Config) FQMNForFunc(namespace, fn, ref string) (string, error) {
 	return fqmn.FromParts(c.Identifier, namespace, fn, ref)
 }
 

diff --git a/tenant/config_test.go b/tenant/config_test.go
@@ -226,7 +226,7 @@ func TestConfigFQMNs(t *testing.T) {
 		return
 	}
 
-	FQMN1 := conf.FQMNForFunc(mod1.Namespace, mod1.Name, "asdf")
+	FQMN1, _ := conf.FQMNForFunc(mod1.Namespace, mod1.Name, "asdf")
 
 	if FQMN1 != "fqmn://dev.suborbital.appname/default/getUser@asdf" {
 		t.Error("FQMN1 should be 'fqmn://dev.suborbital.appname/default/getUser@asdf', got", FQMN1)
@@ -242,7 +242,7 @@ func TestConfigFQMNs(t *testing.T) {
 		return
 	}
 
-	FQMN2 := conf.FQMNForFunc(mod2.Namespace, mod2.Name, "asdf")
+	FQMN2, _ := conf.FQMNForFunc(mod2.Namespace, mod2.Name, "asdf")
 
 	if FQMN2 != "fqmn://dev.suborbital.appname/db/getUserDetails@asdf" {
 		t.Error("FQMN2 should be 'fqmn://dev.suborbital.appname/db/getUserDetails@asdf', got", FQMN2)
@@ -258,7 +258,7 @@ func TestConfigFQMNs(t *testing.T) {
 		return
 	}
 
-	FQMN3 := conf.FQMNForFunc(mod3.Namespace, mod3.Name, "asdf")
+	FQMN3, _ := conf.FQMNForFunc(mod3.Namespace, mod3.Name, "asdf")
 
 	if FQMN3 != "fqmn://dev.suborbital.appname/api/returnUser@asdf" {
 		t.Error("FQMN3 should be 'fqmn://dev.suborbital.appname/api/returnUser@asdf', got", FQMN3)