Rename the validation functions and add more tests

Signed-off-by: Ying Li <ying.li@docker.com>

tuf/data/root.go

@@ -24,10 +24,10 @@ type Root struct {
 	ConsistentSnapshot bool                 `json:"consistent_snapshot"`
 }
 
-// isValidRoot returns an error, or nil, depending on whether the content of the struct
+// isValidRootStructure returns an error, or nil, depending on whether the content of the struct
 // is valid for root metadata.  This does not check signatures or expiry, just that
 // the metadata content is valid.
-func isValidRoot(r Root) error {
+func isValidRootStructure(r Root) error {
 	expectedType := TUFTypes[CanonicalRootRole]
 	if r.Type != expectedType {
 		return ErrInvalidMeta{
@@ -114,10 +114,10 @@ func (r *SignedRoot) MarshalJSON() ([]byte, error) {
 // that it is a valid SignedRoot
 func RootFromSigned(s *Signed) (*SignedRoot, error) {
 	r := Root{}
-	if err := json.Unmarshal(s.Signed, &r); err != nil {
+	if err := defaultSerializer.Unmarshal(s.Signed, &r); err != nil {
 		return nil, err
 	}
-	if err := isValidRoot(r); err != nil {
+	if err := isValidRootStructure(r); err != nil {
 		return nil, err
 	}
 	sigs := make([]Signature, len(s.Signatures))

tuf/data/root_test.go

@@ -16,10 +16,14 @@ type errorSerializer struct {
 	canonicalJSON
 }
 
-func (e errorSerializer) MarshalCanonical(from interface{}) ([]byte, error) {
+func (e errorSerializer) MarshalCanonical(interface{}) ([]byte, error) {
 	return nil, fmt.Errorf("bad")
 }
 
+func (e errorSerializer) Unmarshal([]byte, interface{}) error {
+	return fmt.Errorf("bad")
+}
+
 func validRootTemplate() *SignedRoot {
 	return &SignedRoot{
 		Signed: Root{
@@ -124,6 +128,17 @@ func TestRootMarshalJSONMarshallingErrorsPropagated(t *testing.T) {
 	require.EqualError(t, err, "bad")
 }
 
+func TestRootFromSignedUnmarshallingErrorsPropagated(t *testing.T) {
+	signed, err := validRootTemplate().ToSigned()
+	require.NoError(t, err)
+
+	setDefaultSerializer(errorSerializer{})
+	defer setDefaultSerializer(canonicalJSON{})
+
+	_, err = RootFromSigned(signed)
+	require.EqualError(t, err, "bad")
+}
+
 // RootFromSigned succeeds if the root is valid, and copies the signatures
 // rather than assigns them
 func TestRootFromSignedCopiesSignatures(t *testing.T) {

tuf/data/snapshot.go

@@ -24,10 +24,10 @@ type Snapshot struct {
 	Meta    Files     `json:"meta"`
 }
 
-// isValidSnapshot returns an error, or nil, depending on whether the content of the struct
-// is valid for snapshot metadata.  This does not check signatures or expiry, just that
+// isValidSnapshotStructure returns an error, or nil, depending on whether the content of the
+// struct is valid for snapshot metadata.  This does not check signatures or expiry, just that
 // the metadata content is valid.
-func isValidSnapshot(s Snapshot) error {
+func isValidSnapshotStructure(s Snapshot) error {
 	expectedType := TUFTypes[CanonicalSnapshotRole]
 	if s.Type != expectedType {
 		return ErrInvalidMeta{
@@ -140,10 +140,10 @@ func (sp *SignedSnapshot) MarshalJSON() ([]byte, error) {
 // SnapshotFromSigned fully unpacks a Signed object into a SignedSnapshot
 func SnapshotFromSigned(s *Signed) (*SignedSnapshot, error) {
 	sp := Snapshot{}
-	if err := json.Unmarshal(s.Signed, &sp); err != nil {
+	if err := defaultSerializer.Unmarshal(s.Signed, &sp); err != nil {
 		return nil, err
 	}
-	if err := isValidSnapshot(sp); err != nil {
+	if err := isValidSnapshotStructure(sp); err != nil {
 		return nil, err
 	}
 	sigs := make([]Signature, len(s.Signatures))

tuf/data/snapshot_test.go

@@ -17,6 +17,7 @@ func validSnapshotTemplate() *SignedSnapshot {
 			Type: "Snapshot", Version: 1, Expires: time.Now(), Meta: Files{
 				CanonicalRootRole:    FileMeta{},
 				CanonicalTargetsRole: FileMeta{},
+				"targets/a":          FileMeta{},
 			}},
 		Signatures: []Signature{
 			{KeyID: "key1", Method: "method1", Signature: []byte("hello")},
@@ -101,6 +102,17 @@ func TestSnapshotMarshalJSONMarshallingErrorsPropagated(t *testing.T) {
 	require.EqualError(t, err, "bad")
 }
 
+func TestSnapshotFromSignedUnmarshallingErrorsPropagated(t *testing.T) {
+	signed, err := validSnapshotTemplate().ToSigned()
+	require.NoError(t, err)
+
+	setDefaultSerializer(errorSerializer{})
+	defer setDefaultSerializer(canonicalJSON{})
+
+	_, err = SnapshotFromSigned(signed)
+	require.EqualError(t, err, "bad")
+}
+
 // SnapshotFromSigned succeeds if the snapshot is valid, and copies the signatures
 // rather than assigns them
 func TestSnapshotFromSignedCopiesSignatures(t *testing.T) {
@@ -168,7 +180,7 @@ func TestSnapshotGetMeta(t *testing.T) {
 	require.IsType(t, &FileMeta{}, f)
 
 	// now one that doesn't exist
-	f, err = ts.GetMeta("targets/a")
+	f, err = ts.GetMeta("targets/a/b")
 	require.Error(t, err)
 	require.IsType(t, ErrMissingMeta{}, err)
 	require.Nil(t, f)

tuf/data/targets.go

@@ -23,10 +23,14 @@ type Targets struct {
 	Delegations Delegations `json:"delegations,omitempty"`
 }
 
-// isValidTargets returns an error, or nil, depending on whether the content of the struct
+// isValidTargetsStructure returns an error, or nil, depending on whether the content of the struct
 // is valid for targets metadata.  This does not check signatures or expiry, just that
 // the metadata content is valid.
-func isValidTargets(t Targets, roleName string) error {
+func isValidTargetsStructure(t Targets, roleName string) error {
+	if roleName != CanonicalTargetsRole && !IsDelegation(roleName) {
+		return ErrInvalidRole{Role: roleName}
+	}
+
 	// even if it's a delegated role, the metadata type is "Targets"
 	expectedType := TUFTypes[CanonicalTargetsRole]
 	if t.Type != expectedType {
@@ -145,10 +149,10 @@ func (t *SignedTargets) MarshalJSON() ([]byte, error) {
 // a role name (so it can validate the SignedTargets object)
 func TargetsFromSigned(s *Signed, roleName string) (*SignedTargets, error) {
 	t := Targets{}
-	if err := json.Unmarshal(s.Signed, &t); err != nil {
+	if err := defaultSerializer.Unmarshal(s.Signed, &t); err != nil {
 		return nil, err
 	}
-	if err := isValidTargets(t, roleName); err != nil {
+	if err := isValidTargetsStructure(t, roleName); err != nil {
 		return nil, err
 	}
 	sigs := make([]Signature, len(s.Signatures))

tuf/data/targets_test.go

@@ -108,6 +108,17 @@ func TestTargetsMarshalJSONMarshallingErrorsPropagated(t *testing.T) {
 	require.EqualError(t, err, "bad")
 }
 
+func TestTargetsFromSignedUnmarshallingErrorsPropagated(t *testing.T) {
+	signed, err := validTargetsTemplate().ToSigned()
+	require.NoError(t, err)
+
+	setDefaultSerializer(errorSerializer{})
+	defer setDefaultSerializer(canonicalJSON{})
+
+	_, err = TargetsFromSigned(signed, CanonicalTargetsRole)
+	require.EqualError(t, err, "bad")
+}
+
 // TargetsFromSigned succeeds if the targets is valid, and copies the signatures
 // rather than assigns them
 func TestTargetsFromSignedCopiesSignatures(t *testing.T) {
@@ -127,7 +138,7 @@ func TestTargetsFromSignedCopiesSignatures(t *testing.T) {
 
 // If the targets metadata contains delegations which are invalid, the targets metadata
 // fails to validate and thus fails to convert into a SignedTargets
-func TestTargetsBaseFromSignedValidatesDelegations(t *testing.T) {
+func TestTargetsFromSignedValidatesDelegations(t *testing.T) {
 	for _, roleName := range []string{CanonicalTargetsRole, path.Join(CanonicalTargetsRole, "a")} {
 		targets := validTargetsTemplate()
 		delgRole, err := NewRole(path.Join(roleName, "b"), 1, []string{"key1"}, nil, nil)
@@ -142,6 +153,8 @@ func TestTargetsBaseFromSignedValidatesDelegations(t *testing.T) {
 		require.Error(t, err)
 		require.IsType(t, ErrInvalidMeta{}, err)
 
+		delgRole.Threshold = 1
+
 		// Keys that aren't in the list of keys
 		delgRole.KeyIDs = []string{"keys11"}
 		s, err = targets.ToSigned()
@@ -202,3 +215,15 @@ func TestTargetsFromSignedValidatesRoleType(t *testing.T) {
 		require.Equal(t, "Targets", sTargets.Signed.Type)
 	}
 }
+
+// The rolename passed to TargetsFromSigned must be a valid targets role name
+func TestTargetsFromSignedValidatesRoleName(t *testing.T) {
+	for _, roleName := range []string{"TARGETS", "root/a"} {
+		tg := validTargetsTemplate()
+		s, err := tg.ToSigned()
+		require.NoError(t, err)
+
+		_, err = TargetsFromSigned(s, roleName)
+		require.IsType(t, ErrInvalidRole{}, err)
+	}
+}

tuf/data/timestamp.go

@@ -23,10 +23,10 @@ type Timestamp struct {
 	Meta    Files     `json:"meta"`
 }
 
-// isValidTimestamp returns an error, or nil, depending on whether the content of the struct
+// isValidTimestampStructure returns an error, or nil, depending on whether the content of the struct
 // is valid for timestamp metadata.  This does not check signatures or expiry, just that
 // the metadata content is valid.
-func isValidTimestamp(t Timestamp) error {
+func isValidTimestampStructure(t Timestamp) error {
 	expectedType := TUFTypes[CanonicalTimestampRole]
 	if t.Type != expectedType {
 		return ErrInvalidMeta{
@@ -105,10 +105,10 @@ func (ts *SignedTimestamp) MarshalJSON() ([]byte, error) {
 // SignedTimestamp
 func TimestampFromSigned(s *Signed) (*SignedTimestamp, error) {
 	ts := Timestamp{}
-	if err := json.Unmarshal(s.Signed, &ts); err != nil {
+	if err := defaultSerializer.Unmarshal(s.Signed, &ts); err != nil {
 		return nil, err
 	}
-	if err := isValidTimestamp(ts); err != nil {
+	if err := isValidTimestampStructure(ts); err != nil {
 		return nil, err
 	}
 	sigs := make([]Signature, len(s.Signatures))

tuf/data/timestamp_test.go

@@ -100,6 +100,17 @@ func TestTimestampMarshalJSONMarshallingErrorsPropagated(t *testing.T) {
 	require.EqualError(t, err, "bad")
 }
 
+func TestTimestampFromSignedUnmarshallingErrorsPropagated(t *testing.T) {
+	signed, err := validTimestampTemplate().ToSigned()
+	require.NoError(t, err)
+
+	setDefaultSerializer(errorSerializer{})
+	defer setDefaultSerializer(canonicalJSON{})
+
+	_, err = TimestampFromSigned(signed)
+	require.EqualError(t, err, "bad")
+}
+
 // TimestampFromSigned succeeds if the timestamp is valid, and copies the signatures
 // rather than assigns them
 func TestTimestampFromSignedCopiesSignatures(t *testing.T) {