diff --git a/commonspace/acl/aclclient/aclspaceclient.go b/commonspace/acl/aclclient/aclspaceclient.go index b073cf73..9153bf34 100644 --- a/commonspace/acl/aclclient/aclspaceclient.go +++ b/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 { diff --git a/commonspace/acl/aclclient/aclspaceclient_test.go b/commonspace/acl/aclclient/aclspaceclient_test.go index af810526..f7f39b65 100644 --- a/commonspace/acl/aclclient/aclspaceclient_test.go +++ b/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) diff --git a/commonspace/object/acl/list/aclrecordbuilder.go b/commonspace/object/acl/list/aclrecordbuilder.go index 034daadf..af6d1683 100644 --- a/commonspace/object/acl/list/aclrecordbuilder.go +++ b/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) } diff --git a/commonspace/object/acl/list/aclstate.go b/commonspace/object/acl/list/aclstate.go index 843782b0..8f461230 100644 --- a/commonspace/object/acl/list/aclstate.go +++ b/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() } } diff --git a/commonspace/object/acl/list/list.go b/commonspace/object/acl/list/list.go index 2d503dc6..82a605c7 100644 --- a/commonspace/object/acl/list/list.go +++ b/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 diff --git a/commonspace/object/acl/list/nocontent_test.go b/commonspace/object/acl/list/nocontent_test.go new file mode 100644 index 00000000..c4b9a5af --- /dev/null +++ b/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) +} diff --git a/commonspace/object/acl/list/validator.go b/commonspace/object/acl/list/validator.go index 173b5302..71567508 100644 --- a/commonspace/object/acl/list/validator.go +++ b/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: