acllist: reject a new record with no applicable content

commonspace/acl/aclclient/aclspaceclient.go

@@ -111,6 +111,10 @@ func (c *aclSpaceClient) RequestSelfRemove(ctx context.Context) (err error) {
 }
 
 func (c *aclSpaceClient) ChangePermissions(ctx context.Context, permChange list.PermissionChangesPayload) (err error) {
+	if len(permChange.Changes) == 0 {
+		// nothing to change: a record without changes would be refused
+		return nil
+	}
 	c.acl.Lock()
 	res, err := c.acl.RecordBuilder().BuildPermissionChanges(permChange)
 	if err != nil {
@@ -145,8 +149,14 @@ func (c *aclSpaceClient) RemoveAccounts(ctx context.Context, payload list.Accoun
 
 func (c *aclSpaceClient) RevokeAllInvites(ctx context.Context) (err error) {
 	c.acl.Lock()
+	inviteIds := c.acl.AclState().InviteIds()
+	if len(inviteIds) == 0 {
+		// nothing to revoke: a record without content would be refused
+		c.acl.Unlock()
+		return nil
+	}
 	payload := list.BatchRequestPayload{
-		InviteRevokes: c.acl.AclState().InviteIds(),
+		InviteRevokes: inviteIds,
 	}
 	res, err := c.acl.RecordBuilder().BuildBatchRequest(payload)
 	if err != nil {

commonspace/acl/aclclient/aclspaceclient_test.go

@@ -78,6 +78,43 @@ func TestAclSpaceClient_RevokeAndRotate(t *testing.T) {
 	})
 }
 
+func TestAclSpaceClient_RevokeAllInvites(t *testing.T) {
+	t.Run("revokes every invite", func(t *testing.T) {
+		fx := newFixture(t)
+		defer fx.finish(t)
+		require.NoError(t, fx.exec.Execute("a.invite_anyone::invite1,r"))
+		require.NoError(t, fx.exec.Execute("a.invite_anyone::invite2,rw"))
+		require.Len(t, fx.acl.AclState().Invites(), 2)
+
+		fx.nodeClient.EXPECT().AclAddRecord(ctx, fx.spaceState.SpaceId, gomock.Any()).DoAndReturn(
+			func(ctx context.Context, spaceId string, rec *consensusproto.RawRecord) (*consensusproto.RawRecordWithId, error) {
+				return marshallRecord(t, rec), nil
+			})
+		require.NoError(t, fx.RevokeAllInvites(ctx))
+		require.Empty(t, fx.acl.AclState().Invites())
+	})
+
+	// a record with nothing to revoke would carry no content and be refused; e.g. a retry after the
+	// revocation landed but the caller's cleanup failed must still succeed
+	t.Run("without invites sends nothing", func(t *testing.T) {
+		fx := newFixture(t)
+		defer fx.finish(t)
+		head := fx.acl.AclState().LastRecordId()
+		require.NoError(t, fx.RevokeAllInvites(ctx))
+		require.Equal(t, head, fx.acl.AclState().LastRecordId())
+	})
+}
+
+// a caller that drops the changes a member already has may be left with none, e.g. a retry of a change
+// that already landed; that must succeed without sending a record, which would carry no content
+func TestAclSpaceClient_ChangePermissionsWithoutChanges(t *testing.T) {
+	fx := newFixture(t)
+	defer fx.finish(t)
+	head := fx.acl.AclState().LastRecordId()
+	require.NoError(t, fx.ChangePermissions(ctx, list.PermissionChangesPayload{}))
+	require.Equal(t, head, fx.acl.AclState().LastRecordId())
+}
+
 func TestAclSpaceClient_StopSharing(t *testing.T) {
 	t.Run("not empty", func(t *testing.T) {
 		fx := newFixture(t)

commonspace/object/acl/list/aclrecordbuilder.go

@@ -338,7 +338,7 @@ func (a *aclRecordBuilder) preflightCheck(rawRecord *consensusproto.RawRecord) (
 		return
 	}
 	cp := a.state.Copy()
-	cp.contentValidator.(*contentValidator).verifier = recordverifier.NewValidateFull()
+	cp.contentValidator = newAdmissionValidator(cp.keyStore, cp)
 	return cp.ApplyRecord(aclRec)
 }
 

commonspace/object/acl/list/aclstate.go

@@ -39,6 +39,7 @@ var (
 	ErrOwnerNotFound             = errors.New("owner not found")
 	ErrAddRecordOneToOne         = errors.New("adding a record to one-to-one space is forbidden")
 	ErrEmptyAclRecordData        = errors.New("acl record has neither model nor data")
+	ErrNoAclContent              = errors.New("acl record has no content")
 	ErrReadKeyChangeNotAlone     = errors.New("a batch read key change can only accompany invite revokes and declines")
 )
 
@@ -398,6 +399,9 @@ func (st *AclState) saveKeysFromRoot(id string, root *aclrecordproto.AclRoot) (e
 
 func (st *AclState) applyChangeData(record *AclRecord) (err error) {
 	model := record.Model.(*aclrecordproto.AclData)
+	if err = st.contentValidator.ValidateAclData(model); err != nil {
+		return err
+	}
 	for _, ch := range model.GetAclContent() {
 		if err = st.applyChangeContent(ch, record); err != nil {
 			log.Info("error while applying changes", zap.Error(err))
@@ -483,7 +487,7 @@ func (st *AclState) applyChangeContent(ch *aclrecordproto.AclContentValue, recor
 		return st.applySpaceOptionsChange(ch.GetSpaceOptionsChange(), record)
 	default:
 		log.Errorf("got unexpected content type: %s", record.Id)
-		return nil
+		return st.contentValidator.ValidateUnexpectedContent()
 	}
 }
 

commonspace/object/acl/list/list.go

@@ -239,7 +239,7 @@ func (a *aclList) ValidateRawRecord(rawRec *consensusproto.RawRecord, afterValid
 		return
 	}
 	stateCopy := a.aclState.Copy()
-	stateCopy.contentValidator = newContentValidator(stateCopy.keyStore, stateCopy, recordverifier.NewValidateFull())
+	stateCopy.contentValidator = newAdmissionValidator(stateCopy.keyStore, stateCopy)
 	err = stateCopy.ApplyRecord(record)
 	if err != nil || afterValid == nil {
 		return

commonspace/object/acl/list/nocontent_test.go

@@ -0,0 +1,165 @@
+package list
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/anyproto/any-sync/commonspace/headsync/headstorage"
+	"github.com/anyproto/any-sync/commonspace/object/accountdata"
+	"github.com/anyproto/any-sync/commonspace/object/acl/aclrecordproto"
+	"github.com/anyproto/any-sync/commonspace/object/acl/list/listtest"
+	"github.com/anyproto/any-sync/commonspace/object/acl/recordverifier"
+	"github.com/anyproto/any-sync/consensus/consensusproto"
+	"github.com/anyproto/any-sync/util/crypto"
+)
+
+// Permission checks live in the per-type apply handlers, so a record whose content reaches none of them
+// would be applied without checking its author. These records carry no applicable content and are signed
+// by an identity that is not in the acl at all.
+var noApplicableContent = []struct {
+	name string
+	data []byte
+	err  error
+}{
+	// AclData{AclContent: [{}]}: one content value with the oneof unset
+	{name: "empty content value", data: []byte{0x0a, 0x00}, err: ErrUnexpectedContentType},
+	// one content value whose only field (99, length-delimited, empty) this build does not know — how a
+	// content type added in a later release decodes
+	{name: "unknown content type", data: []byte{0x0a, 0x03, 0x9a, 0x06, 0x00}, err: ErrUnexpectedContentType},
+	{name: "no data", data: nil, err: ErrNoAclContent},
+	// AclData carrying only an unknown field (99, varint): non-empty bytes, no content values
+	{name: "only an unknown field", data: []byte{0x98, 0x06, 0x01}, err: ErrNoAclContent},
+	// one permissionChanges (field 10) with no changes: its handler checks the author per change
+	{name: "empty permission changes", data: []byte{0x0a, 0x02, 0x52, 0x00}, err: ErrNoAclContent},
+}
+
+func signAclRecord(t *testing.T, keys *accountdata.AccountKeys, prevId string, data []byte) (*consensusproto.RawRecord, *consensusproto.RawRecordWithId) {
+	identity, err := keys.SignKey.GetPublic().Marshall()
+	require.NoError(t, err)
+	payload, err := (&consensusproto.Record{
+		PrevId:    prevId,
+		Identity:  identity,
+		Data:      data,
+		Timestamp: time.Now().Unix(),
+	}).MarshalVT()
+	require.NoError(t, err)
+	signature, err := keys.SignKey.Sign(payload)
+	require.NoError(t, err)
+	raw := &consensusproto.RawRecord{Payload: payload, Signature: signature}
+	return raw, listtest.WrapAclRecord(raw)
+}
+
+func TestAclList_ValidateRawRecordRejectsNoApplicableContent(t *testing.T) {
+	for _, tc := range noApplicableContent {
+		t.Run(tc.name, func(t *testing.T) {
+			fx := newFixture(t)
+			stranger, err := accountdata.NewRandom()
+			require.NoError(t, err)
+			head := fx.ownerAcl.AclState().LastRecordId()
+
+			raw, _ := signAclRecord(t, stranger, head, tc.data)
+			require.ErrorIs(t, fx.ownerAcl.ValidateRawRecord(raw, nil), tc.err)
+			require.Equal(t, head, fx.ownerAcl.AclState().LastRecordId())
+		})
+	}
+}
+
+// A valid change does not carry an inapplicable one past admission, in either order, and the caller's
+// check never sees the partly applied state.
+func TestAclList_ValidateRawRecordRejectsMixedContent(t *testing.T) {
+	fx := newFixture(t)
+	head := fx.ownerAcl.AclState().LastRecordId()
+	_, inviteKey, err := crypto.GenerateRandomEd25519KeyPair()
+	require.NoError(t, err)
+	protoInviteKey, err := inviteKey.Marshall()
+	require.NoError(t, err)
+	invite := &aclrecordproto.AclContentValue{Value: &aclrecordproto.AclContentValue_Invite{Invite: &aclrecordproto.AclAccountInvite{
+		InviteKey:  protoInviteKey,
+		InviteType: aclrecordproto.AclInviteType_RequestToJoin,
+	}}}
+	marshal := func(content ...*aclrecordproto.AclContentValue) []byte {
+		data, err := (&aclrecordproto.AclData{AclContent: content}).MarshalVT()
+		require.NoError(t, err)
+		return data
+	}
+
+	raw, _ := signAclRecord(t, fx.ownerKeys, head, marshal(invite))
+	require.NoError(t, fx.ownerAcl.ValidateRawRecord(raw, nil))
+
+	for name, data := range map[string][]byte{
+		"valid then empty":                    marshal(invite, &aclrecordproto.AclContentValue{}),
+		"empty then valid":                    marshal(&aclrecordproto.AclContentValue{}, invite),
+		"valid then empty permission changes": marshal(invite, &aclrecordproto.AclContentValue{Value: &aclrecordproto.AclContentValue_PermissionChanges{PermissionChanges: &aclrecordproto.AclAccountPermissionChanges{}}}),
+	} {
+		t.Run(name, func(t *testing.T) {
+			raw, _ := signAclRecord(t, fx.ownerKeys, head, data)
+			err := fx.ownerAcl.ValidateRawRecord(raw, func(*AclState) error {
+				t.Fatal("afterValid ran for a rejected record")
+				return nil
+			})
+			require.Error(t, err)
+			require.Equal(t, head, fx.ownerAcl.AclState().LastRecordId())
+			require.Empty(t, fx.ownerAcl.AclState().Invites())
+		})
+	}
+}
+
+// An acl can hold such records: a network that did not check for them at admission accepted them, and a
+// content type added in a later release reaches older clients the same way. Ingesting, rebuilding and
+// migrating the log must not fail on them, whichever verifier the list uses — node stats, migration and
+// some client paths build from storage with a validating one.
+func TestAclList_StoredNoApplicableContentStillBuilds(t *testing.T) {
+	verifiers := map[string]recordverifier.AcceptorVerifier{
+		"validating":     recordverifier.NewValidateFull(),
+		"non-validating": noValidateVerifier{},
+	}
+	for _, tc := range noApplicableContent {
+		for vName, verifier := range verifiers {
+			t.Run(tc.name+"/"+vName, func(t *testing.T) {
+				fx := newFixture(t)
+				stranger, err := accountdata.NewRandom()
+				require.NoError(t, err)
+
+				_, withId := signAclRecord(t, stranger, fx.ownerAcl.AclState().LastRecordId(), tc.data)
+				require.NoError(t, fx.ownerAcl.AddRawRecord(withId))
+				require.Equal(t, withId.Id, fx.ownerAcl.AclState().LastRecordId())
+
+				rebuilt, err := BuildAclListWithIdentity(fx.ownerKeys, fx.ownerAcl.storage, verifier)
+				require.NoError(t, err)
+				require.Equal(t, withId.Id, rebuilt.AclState().LastRecordId())
+				require.True(t, rebuilt.AclState().Permissions(stranger.SignKey.GetPublic()).NoPermissions())
+
+				// the owner still builds on top of it
+				_, err = rebuilt.RecordBuilder().BuildInvite()
+				require.NoError(t, err)
+
+				// and a fresh list ingests the whole log, as the storage migration does
+				ctx := context.Background()
+				store := createStore(ctx, t)
+				headStorage, err := headstorage.New(ctx, store)
+				require.NoError(t, err)
+				storage, err := CreateStorage(ctx, fx.ownerAcl.Root(), headStorage, store)
+				require.NoError(t, err)
+				migrated, err := BuildAclListWithIdentity(fx.ownerKeys, storage, verifier)
+				require.NoError(t, err)
+				var records []*consensusproto.RawRecordWithId
+				for _, rec := range fx.ownerAcl.Records()[1:] {
+					raw, err := fx.ownerAcl.storage.Get(ctx, rec.Id)
+					require.NoError(t, err)
+					records = append(records, raw.RawRecordWithId())
+				}
+				require.NoError(t, migrated.AddRawRecords(records))
+				require.Equal(t, withId.Id, migrated.AclState().LastRecordId())
+			})
+		}
+	}
+}
+
+func TestAclRecordBuilder_EmptyBatchRequest(t *testing.T) {
+	fx := newFixture(t)
+	_, err := fx.ownerAcl.RecordBuilder().BuildBatchRequest(BatchRequestPayload{})
+	require.ErrorIs(t, err, ErrNoAclContent)
+}

commonspace/object/acl/list/validator.go

@@ -10,6 +10,8 @@ import (
 
 type ContentValidator interface {
 	ValidateAclRecordContents(ch *AclRecord) (err error)
+	ValidateAclData(data *aclrecordproto.AclData) (err error)
+	ValidateUnexpectedContent() (err error)
 	ValidatePermissionChange(ch *aclrecordproto.AclAccountPermissionChange, authorIdentity crypto.PubKey) (err error)
 	ValidatePermissionChanges(ch *aclrecordproto.AclAccountPermissionChanges, authorIdentity crypto.PubKey) (err error)
 	ValidateOwnershipChange(ch *aclrecordproto.AclOwnershipChange, authorIdentity crypto.PubKey) (err error)
@@ -32,6 +34,10 @@ type contentValidator struct {
 	keyStore crypto.KeyStorage
 	aclState *AclState
 	verifier recordverifier.AcceptorVerifier
+	// admission marks validation of a record that is not in the log yet (ValidateRawRecord, the builder's
+	// preflight). Only there is a record without applicable content rejected: once the network has
+	// accepted one, replaying it (build, migration, sync) must not fail.
+	admission bool
 }
 
 func newContentValidator(keyStore crypto.KeyStorage, aclState *AclState, verifier recordverifier.AcceptorVerifier) ContentValidator {
@@ -42,6 +48,17 @@ func newContentValidator(keyStore crypto.KeyStorage, aclState *AclState, verifie
 	}
 }
 
+// newAdmissionValidator validates a record before it enters the log: in full, and also rejecting content
+// that no per-type validator would check.
+func newAdmissionValidator(keyStore crypto.KeyStorage, aclState *AclState) ContentValidator {
+	return &contentValidator{
+		keyStore:  keyStore,
+		aclState:  aclState,
+		verifier:  recordverifier.NewValidateFull(),
+		admission: true,
+	}
+}
+
 func (c *contentValidator) ValidatePermissionChanges(ch *aclrecordproto.AclAccountPermissionChanges, authorIdentity crypto.PubKey) (err error) {
 	if !c.verifier.ShouldValidate() {
 		return nil
@@ -178,6 +195,34 @@ func (c *contentValidator) ValidateAclRecordContents(ch *AclRecord) (err error)
 	return
 }
 
+// ValidateAclData rejects a new record that carries no content: nothing in it reaches a per-type
+// validator, so applying it would move the head without checking its author.
+func (c *contentValidator) ValidateAclData(data *aclrecordproto.AclData) (err error) {
+	if !c.admission {
+		return nil
+	}
+	if len(data.GetAclContent()) == 0 {
+		return ErrNoAclContent
+	}
+	for _, content := range data.GetAclContent() {
+		// a permission-change batch checks its author per change, so an empty one checks nothing
+		if changes := content.GetPermissionChanges(); changes != nil && len(changes.GetChanges()) == 0 {
+			return ErrNoAclContent
+		}
+	}
+	return nil
+}
+
+// ValidateUnexpectedContent covers a content value no apply case handles: an unset oneof, or a type added
+// after this build. No per-type validator checks its author, so a new record carrying one is rejected. A
+// stored one applies as a no-op, so a client keeps loading an acl that carries a type it predates.
+func (c *contentValidator) ValidateUnexpectedContent() (err error) {
+	if !c.admission {
+		return nil
+	}
+	return ErrUnexpectedContentType
+}
+
 func (c *contentValidator) validateAclRecordContent(ch *aclrecordproto.AclContentValue, authorIdentity crypto.PubKey) (err error) {
 	switch {
 	case ch.GetPermissionChange() != nil: