Merge pull request #54 from mtrmac/cleanups

signature cleanups

signature/json.go

@@ -0,0 +1,51 @@
+package signature
+
+import "fmt"
+
+// jsonFormatError is returned when JSON does not match expected format.
+type jsonFormatError string
+
+func (err jsonFormatError) Error() string {
+	return string(err)
+}
+
+// validateExactMapKeys returns an error if the keys of m are not exactly expectedKeys, which must be pairwise distinct
+func validateExactMapKeys(m map[string]interface{}, expectedKeys ...string) error {
+	if len(m) != len(expectedKeys) {
+		return jsonFormatError("Unexpected keys in a JSON object")
+	}
+
+	for _, k := range expectedKeys {
+		if _, ok := m[k]; !ok {
+			return jsonFormatError(fmt.Sprintf("Key %s missing in a JSON object", k))
+		}
+	}
+	// Assuming expectedKeys are pairwise distinct, we know m contains len(expectedKeys) different values in expectedKeys.
+	return nil
+}
+
+// mapField returns a member fieldName of m, if it is a JSON map, or an error.
+func mapField(m map[string]interface{}, fieldName string) (map[string]interface{}, error) {
+	untyped, ok := m[fieldName]
+	if !ok {
+		return nil, jsonFormatError(fmt.Sprintf("Field %s missing", fieldName))
+	}
+	v, ok := untyped.(map[string]interface{})
+	if !ok {
+		return nil, jsonFormatError(fmt.Sprintf("Field %s is not a JSON object", fieldName))
+	}
+	return v, nil
+}
+
+// stringField returns a member fieldName of m, if it is a string, or an error.
+func stringField(m map[string]interface{}, fieldName string) (string, error) {
+	untyped, ok := m[fieldName]
+	if !ok {
+		return "", jsonFormatError(fmt.Sprintf("Field %s missing", fieldName))
+	}
+	v, ok := untyped.(string)
+	if !ok {
+		return "", jsonFormatError(fmt.Sprintf("Field %s is not a JSON object", fieldName))
+	}
+	return v, nil
+}

signature/json_test.go

@@ -0,0 +1,76 @@
+package signature
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type mSI map[string]interface{} // To minimize typing the long name
+
+// A short-hand way to get a JSON object field value or panic. No error handling done, we know
+// what we are working with, a panic in a test is good enough, and fitting test cases on a single line
+// is a priority.
+func x(m mSI, fields ...string) mSI {
+	for _, field := range fields {
+		// Not .(mSI) because type assertion of an unnamed type to a named type always fails (the types
+		// are not "identical"), but the assignment is fine because they are "assignable".
+		m = m[field].(map[string]interface{})
+	}
+	return m
+}
+
+func TestValidateExactMapKeys(t *testing.T) {
+	// Empty map and keys
+	err := validateExactMapKeys(mSI{})
+	assert.NoError(t, err)
+
+	// Success
+	err = validateExactMapKeys(mSI{"a": nil, "b": 1}, "b", "a")
+	assert.NoError(t, err)
+
+	// Extra map keys
+	err = validateExactMapKeys(mSI{"a": nil, "b": 1}, "a")
+	assert.Error(t, err)
+
+	// Extra expected keys
+	err = validateExactMapKeys(mSI{"a": 1}, "b", "a")
+	assert.Error(t, err)
+
+	// Unexpected key values
+	err = validateExactMapKeys(mSI{"a": 1}, "b")
+	assert.Error(t, err)
+}
+
+func TestMapField(t *testing.T) {
+	// Field not found
+	_, err := mapField(mSI{"a": mSI{}}, "b")
+	assert.Error(t, err)
+
+	// Field has a wrong type
+	_, err = mapField(mSI{"a": 1}, "a")
+	assert.Error(t, err)
+
+	// Success
+	// FIXME? We can't use mSI as the type of child, that type apparently can't be converted to the raw map type.
+	child := map[string]interface{}{"b": mSI{}}
+	m, err := mapField(mSI{"a": child, "b": nil}, "a")
+	require.NoError(t, err)
+	assert.Equal(t, child, m)
+}
+
+func TestStringField(t *testing.T) {
+	// Field not found
+	_, err := stringField(mSI{"a": "x"}, "b")
+	assert.Error(t, err)
+
+	// Field has a wrong type
+	_, err = stringField(mSI{"a": 1}, "a")
+	assert.Error(t, err)
+
+	// Success
+	s, err := stringField(mSI{"a": "x", "b": nil}, "a")
+	require.NoError(t, err)
+	assert.Equal(t, "x", s)
+}

signature/mechanism.go

@@ -1,3 +1,5 @@
+// Note: Consider the API unstable until the code supports at least three different image formats or transports.
+
 package signature
 
 import (

signature/signature.go

@@ -36,6 +36,9 @@ type privateSignature struct {
 	Signature
 }
 
+// Compile-time check that privateSignature implements json.Marshaler
+var _ json.Marshaler = (*privateSignature)(nil)
+
 // MarshalJSON implements the json.Marshaler interface.
 func (s privateSignature) MarshalJSON() ([]byte, error) {
 	return s.marshalJSONWithVariables(time.Now().UTC().Unix(), signatureCreatorID)
@@ -62,49 +65,23 @@ func (s privateSignature) marshalJSONWithVariables(timestamp int64, creatorID st
 	return json.Marshal(signature)
 }
 
-// validateExactMapKeys returns an error if the keys of m are not exactly expectedKeys, which must be pairwise distinct
+// Compile-time check that privateSignature implements json.Unmarshaler
-func validateExactMapKeys(m map[string]interface{}, expectedKeys ...string) error {
+var _ json.Unmarshaler = (*privateSignature)(nil)
-	if len(m) != len(expectedKeys) {
-		return InvalidSignatureError{msg: "Unexpected keys in a JSON object"}
-	}
-
-	for _, k := range expectedKeys {
-		if _, ok := m[k]; !ok {
-			return InvalidSignatureError{msg: fmt.Sprintf("Key %s missing in a JSON object", k)}
-		}
-	}
-	// Assuming expectedKeys are pairwise distinct, we know m contains len(expectedKeys) different values in expectedKeys.
-	return nil
-}
-
-// mapField returns a member fieldName of m, if it is a JSON map, or an error.
-func mapField(m map[string]interface{}, fieldName string) (map[string]interface{}, error) {
-	untyped, ok := m[fieldName]
-	if !ok {
-		return nil, InvalidSignatureError{msg: fmt.Sprintf("Field %s missing", fieldName)}
-	}
-	v, ok := untyped.(map[string]interface{})
-	if !ok {
-		return nil, InvalidSignatureError{msg: fmt.Sprintf("Field %s is not a JSON object", fieldName)}
-	}
-	return v, nil
-}
-
-// stringField returns a member fieldName of m, if it is a string, or an error.
-func stringField(m map[string]interface{}, fieldName string) (string, error) {
-	untyped, ok := m[fieldName]
-	if !ok {
-		return "", InvalidSignatureError{msg: fmt.Sprintf("Field %s missing", fieldName)}
-	}
-	v, ok := untyped.(string)
-	if !ok {
-		return "", InvalidSignatureError{msg: fmt.Sprintf("Field %s is not a JSON object", fieldName)}
-	}
-	return v, nil
-}
 
 // UnmarshalJSON implements the json.Unmarshaler interface
 func (s *privateSignature) UnmarshalJSON(data []byte) error {
+	err := s.strictUnmarshalJSON(data)
+	if err != nil {
+		if _, ok := err.(jsonFormatError); ok {
+			err = InvalidSignatureError{msg: err.Error()}
+		}
+	}
+	return err
+}
+
+// strictUnmarshalJSON is UnmarshalJSON, except that it may return the internal jsonFormatError error type.
+// Splitting it into a separate function allows us to do the jsonFormatError → InvalidSignatureError in a single place, the caller.
+func (s *privateSignature) strictUnmarshalJSON(data []byte) error {
 	var untyped interface{}
 	if err := json.Unmarshal(data, &untyped); err != nil {
 		return err

signature/signature_test.go

@@ -39,76 +39,8 @@ func TestMarshalJSON(t *testing.T) {
 	assert.NoError(t, err)
 }
 
-type mSI map[string]interface{} // To minimize typing the long name
-
-func TestValidateExactMapKeys(t *testing.T) {
-	// Empty map and keys
-	err := validateExactMapKeys(mSI{})
-	assert.NoError(t, err)
-
-	// Success
-	err = validateExactMapKeys(mSI{"a": nil, "b": 1}, "b", "a")
-	assert.NoError(t, err)
-
-	// Extra map keys
-	err = validateExactMapKeys(mSI{"a": nil, "b": 1}, "a")
-	assert.Error(t, err)
-
-	// Extra expected keys
-	err = validateExactMapKeys(mSI{"a": 1}, "b", "a")
-	assert.Error(t, err)
-
-	// Unexpected key values
-	err = validateExactMapKeys(mSI{"a": 1}, "b")
-	assert.Error(t, err)
-}
-
-func TestMapField(t *testing.T) {
-	// Field not found
-	_, err := mapField(mSI{"a": mSI{}}, "b")
-	assert.Error(t, err)
-
-	// Field has a wrong type
-	_, err = mapField(mSI{"a": 1}, "a")
-	assert.Error(t, err)
-
-	// Success
-	// FIXME? We can't use mSI as the type of child, that type apparently can't be converted to the raw map type.
-	child := map[string]interface{}{"b": mSI{}}
-	m, err := mapField(mSI{"a": child, "b": nil}, "a")
-	require.NoError(t, err)
-	assert.Equal(t, child, m)
-}
-
-func TestStringField(t *testing.T) {
-	// Field not found
-	_, err := stringField(mSI{"a": "x"}, "b")
-	assert.Error(t, err)
-
-	// Field has a wrong type
-	_, err = stringField(mSI{"a": 1}, "a")
-	assert.Error(t, err)
-
-	// Success
-	s, err := stringField(mSI{"a": "x", "b": nil}, "a")
-	require.NoError(t, err)
-	assert.Equal(t, "x", s)
-}
-
-// A short-hand way to get a JSON object field value or panic. No error handling done, we know
-// what we are working with, a panic in a test is good enough, and fitting test cases on a single line
-// is a priority.
-func x(m mSI, fields ...string) mSI {
-	for _, field := range fields {
-		// Not .(mSI) because type assertion of an unnamed type to a named type always fails (the types
-		// are not "identical"), but the assignment is fine because they are "assignable".
-		m = m[field].(map[string]interface{})
-	}
-	return m
-}
-
 // Return the result of modifying validJSON with fn and unmarshaling it into *sig
-func tryUnmarshalModified(t *testing.T, sig *privateSignature, validJSON []byte, modifyFn func(mSI)) error {
+func tryUnmarshalModifiedSignature(t *testing.T, sig *privateSignature, validJSON []byte, modifyFn func(mSI)) error {
 	var tmp mSI
 	err := json.Unmarshal(validJSON, &tmp)
 	require.NoError(t, err)
@@ -118,6 +50,7 @@ func tryUnmarshalModified(t *testing.T, sig *privateSignature, validJSON []byte,
 	testJSON, err := json.Marshal(tmp)
 	require.NoError(t, err)
 
+	*sig = privateSignature{}
 	return json.Unmarshal(testJSON, sig)
 }
 
@@ -184,7 +117,7 @@ func TestUnmarshalJSON(t *testing.T) {
 		func(v mSI) { x(v, "critical", "identity")["docker-reference"] = 1 },
 	}
 	for _, fn := range breakFns {
-		err = tryUnmarshalModified(t, &s, validJSON, fn)
+		err = tryUnmarshalModifiedSignature(t, &s, validJSON, fn)
 		assert.Error(t, err)
 	}
 
@@ -196,17 +129,12 @@ func TestUnmarshalJSON(t *testing.T) {
 		func(v mSI) { delete(x(v, "optional"), "creator") },
 	}
 	for _, fn := range allowedModificationFns {
-		s = privateSignature{}
+		err = tryUnmarshalModifiedSignature(t, &s, validJSON, fn)
-		err = tryUnmarshalModified(t, &s, validJSON, fn)
 		require.NoError(t, err)
 		assert.Equal(t, validSig, s)
 	}
 }
 
-type savedEnvironment struct {
-	vars []string
-}
-
 func TestSign(t *testing.T) {
 	mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
 	require.NoError(t, err)