diff --git a/client/client_groupchat.go b/client/client_groupchat.go index 65ccb006..a43985a8 100644 --- a/client/client_groupchat.go +++ b/client/client_groupchat.go @@ -341,12 +341,15 @@ func (c *Client) handleGCInvite(ru *RemoteUser, invite rpc.RMGroupInvite) error var iid uint64 err := c.dbUpdate(func(tx clientdb.ReadWriteTx) error { var err error - _, err = c.db.GetGC(tx, invite.ID) - if !errors.Is(err, clientdb.ErrNotFound) { - if err == nil { - err = fmt.Errorf("can't accept gc invite: gc %q already exists", - invite.ID.String()) - } + // A failure other than "not found" is a real error. Otherwise store + // the invite even when the GC already exists locally: a re-invite is + // how an admin re-adds a user whose local copy is stale (e.g. after + // restoring an old backup). The stale roster is refreshed by the + // normal RMGroupList update (maybeUpdateGC) once the invite is + // accepted, which already verifies the sender's admin permission and + // rejects generation backtracks. + if _, err = c.db.GetGC(tx, invite.ID); err != nil && + !errors.Is(err, clientdb.ErrNotFound) { return err } @@ -1243,9 +1246,18 @@ func (c *Client) handleGCMessage(ru *RemoteUser, gcm rpc.RMGroupMessage, ts time } if !found { - // The sender is not in the GC list we have. - c.log.Warnf("Received message in GC %q from non-member %s", - rgcm.GCAlias, ru) + // The sender is not in our member list. If they were idle-kicked + // and are messaging the GC again, re-add them; otherwise drop it. + if readded, rerr := c.maybeReAddIdleKickedMember(ru, gcm.ID); rerr != nil { + c.log.Warnf("Unable to re-add idle-kicked member %s to GC %s: %v", + ru, gcm.ID, rerr) + } else if readded { + c.log.Infof("Re-added idle-kicked member %s to GC %s after their message", + ru, gcm.ID) + } else { + c.log.Warnf("Received message in GC %q from non-member %s", + rgcm.GCAlias, ru) + } return nil } @@ -1453,6 +1465,11 @@ func (c *Client) GCKick(gcID zkidentity.ShortID, uid UserID, reason string) erro return err } + // Track idle kicks so the member can be auto re-added if they return. + if reason == gcIdleKickReason { + c.recordIdleKick(gcID, uid) + } + rmgk := rpc.RMGroupKick{ Member: uid, Reason: reason, @@ -1521,9 +1538,115 @@ func (c *Client) handleGCKick(ru *RemoteUser, rmgk rpc.RMGroupKick, ts time.Time rmgk.Reason, !rmgk.Parted) c.notifyUpdatedGC(ru, oldGC.Metadata, rmgk.NewGroupList, ts) + // If an admin idle-kicked another user, record it so the local client + // (when it is also an admin) can auto re-add them when they return. + if !rmgk.Parted && rmgk.Reason == gcIdleKickReason && kickedUid != c.PublicID() { + c.recordIdleKick(rmgk.NewGroupList.ID, kickedUid) + } + return nil } +// gcIdleKickReason is the reason string used when auto-removing an idle user +// from a GC. It flags an idle kick so the member can be auto re-added if they +// later message the GC again. +const gcIdleKickReason = "User is idle for too long" + +// recordIdleKick persists (only when auto-remove-idle is enabled) that uid was +// kicked from gcid for idleness, so it can be auto re-added on return. +func (c *Client) recordIdleKick(gcid zkidentity.ShortID, uid clientintf.UserID) { + if c.cfg.AutoRemoveIdleUsersInterval <= 0 { + return + } + err := c.dbUpdate(func(tx clientdb.ReadWriteTx) error { + return c.db.AddToGCKickedMembers(tx, gcid, uid, true) + }) + if err != nil { + c.log.Warnf("Unable to record idle-kicked member %s in GC %s: %v", + uid, gcid, err) + } +} + +// maybeReAddIdleKickedMember re-adds ru to gcID when the local client admins the +// GC, ru was idle-kicked from it, ru is not blocked, and auto-remove-idle is +// enabled. The re-add appends the member, bumps the generation and broadcasts +// the updated group list, which the returning member applies via the normal +// group-list update (no invite needed - KX survives the kick). Returns whether +// it re-added the member. +func (c *Client) maybeReAddIdleKickedMember(ru *RemoteUser, gcID zkidentity.ShortID) (bool, error) { + if c.cfg.AutoRemoveIdleUsersInterval <= 0 { + return false, nil + } + + uid := ru.ID() + var newMeta rpc.RMGroupList + var readded bool + err := c.dbUpdate(func(tx clientdb.ReadWriteTx) error { + gc, err := c.db.GetGC(tx, gcID) + if err != nil { + return err + } + + // Only an admin can change the roster. + if err := c.uidHasGCPerm(&gc.Metadata, c.PublicID()); err != nil { + return nil + } + + // Only re-add members that were idle-kicked. + kicked, err := c.db.GetGCKickedMembers(tx, gcID) + if err != nil { + return err + } + if !kicked.IsIdleKicked(uid) { + return nil + } + + // Never re-add a blocked user; just drop the stale record. + gcbl, err := c.db.GetGCBlockList(tx, gcID) + if err != nil { + return err + } + if c.db.IsBlocked(tx, uid) || gcbl.IsBlocked(uid) { + return c.db.RemoveFromGCKickedMembers(tx, gcID, uid) + } + + // If somehow already a member, just clear the record. + for i := range gc.Metadata.Members { + if gc.Metadata.Members[i] == uid { + return c.db.RemoveFromGCKickedMembers(tx, gcID, uid) + } + } + + // Re-add: append, bump generation, save, clear the record. + gc.Metadata.Members = append(gc.Metadata.Members, uid) + gc.Metadata.Generation += 1 + gc.Metadata.Timestamp = time.Now().Unix() + if err := c.db.SaveGC(tx, gc); err != nil { + return err + } + if err := c.db.RemoveFromGCKickedMembers(tx, gcID, uid); err != nil { + return err + } + c.db.LogGCMsg(tx, gc.Name(), gcID, true, "", + fmt.Sprintf("Re-added %s after they returned from being idle-kicked", + strescape.Nick(ru.Nick())), time.Now()) + newMeta = gc.Metadata + readded = true + return nil + }) + if err != nil || !readded { + return false, err + } + + // Broadcast the updated member list. The re-added member applies it via + // the normal group-list update (maybeUpdateGC). + if err := c.sendToGCMembers(gcID, newMeta.Members, "sendlist", newMeta, nil); err != nil { + return true, err + } + c.ntfns.notifyOnAddedGCMembers(newMeta, []clientintf.UserID{uid}) + return true, nil +} + // PartFromGC withdraws the local client from the given GC. func (c *Client) PartFromGC(gcID zkidentity.ShortID, reason string) error { var gc clientdb.GroupChat diff --git a/client/client_kx.go b/client/client_kx.go index fd2167a3..1df64d42 100644 --- a/client/client_kx.go +++ b/client/client_kx.go @@ -984,7 +984,7 @@ func (c *Client) unsubIdleUsers() error { gcid := gcid uid := uid go func() { - err := c.GCKick(gcid, uid, "User is idle for too long") + err := c.GCKick(gcid, uid, gcIdleKickReason) if err != nil { c.log.Warnf("Unable to remove user %s from GC %s: %v", uid, gcid, err) diff --git a/client/clientdb/gc.go b/client/clientdb/gc.go index 524eacb6..bd5ab7cc 100644 --- a/client/clientdb/gc.go +++ b/client/clientdb/gc.go @@ -19,9 +19,10 @@ import ( ) const ( - gcAliasesFile = "gcaliases.json" - invitesTable = "invites" - gcBlockListExt = ".blocklist" + gcAliasesFile = "gcaliases.json" + invitesTable = "invites" + gcBlockListExt = ".blocklist" + gcKickedMembersExt = ".kickedmembers" ) type GCInvite struct { @@ -391,9 +392,13 @@ func (db *DB) DeleteGC(tx ReadWriteTx, gcID zkidentity.ShortID) error { if err := os.Remove(filename); err != nil { return err } - blockListFname := filename + gcBlockListExt - if fileExists(blockListFname) { - return os.Remove(blockListFname) + for _, ext := range []string{gcBlockListExt, gcKickedMembersExt} { + auxFname := filename + ext + if fileExists(auxFname) { + if err := os.Remove(auxFname); err != nil { + return err + } + } } return nil } @@ -546,6 +551,70 @@ func (db *DB) GetGCBlockList(tx ReadTx, gcid zkidentity.ShortID) (GCBlockList, e } +// GCKickedMembers maps the UID of a member removed from a GC to whether the +// removal was due to idleness. Used to auto re-invite idle-kicked members when +// they message the GC again. +type GCKickedMembers map[string]bool + +// IsIdleKicked returns true if the given UID is recorded as having been kicked +// from the GC for idleness. +func (gckm GCKickedMembers) IsIdleKicked(uid UserID) bool { + return gckm[uid.String()] +} + +// AddToGCKickedMembers records that the given UID was kicked from the GC. idle +// flags whether the removal was due to idleness. +func (db *DB) AddToGCKickedMembers(tx ReadWriteTx, gcid zkidentity.ShortID, uid UserID, idle bool) error { + filename := filepath.Join(db.root, groupchatDir, gcid.String()+ + gcKickedMembersExt) + + var entries GCKickedMembers + err := db.readJsonFile(filename, &entries) + if err != nil && !errors.Is(err, ErrNotFound) { + return err + } + + if entries == nil { + entries = make(GCKickedMembers) + } + entries[uid.String()] = idle + + return db.saveJsonFile(filename, &entries) +} + +// RemoveFromGCKickedMembers clears the kick record for the given UID in the GC. +func (db *DB) RemoveFromGCKickedMembers(tx ReadWriteTx, gcid zkidentity.ShortID, uid UserID) error { + filename := filepath.Join(db.root, groupchatDir, gcid.String()+ + gcKickedMembersExt) + + var entries GCKickedMembers + err := db.readJsonFile(filename, &entries) + if errors.Is(err, ErrNotFound) { + // Nothing recorded. NOP. + return nil + } + if err != nil { + return err + } + + delete(entries, uid.String()) + return db.saveJsonFile(filename, entries) +} + +// GetGCKickedMembers returns the kicked-members record of the specified GC. +// Returns nil if there is none. +func (db *DB) GetGCKickedMembers(tx ReadTx, gcid zkidentity.ShortID) (GCKickedMembers, error) { + filename := filepath.Join(db.root, groupchatDir, gcid.String()+ + gcKickedMembersExt) + + var entries GCKickedMembers + err := db.readJsonFile(filename, &entries) + if errors.Is(err, ErrNotFound) { + return nil, nil + } + return entries, err +} + // CacheReceivedGCM stores a cached received GC message. func (db *DB) CacheReceivedGCM(tx ReadWriteTx, rgcm clientintf.ReceivedGCMsg) error { filename := filepath.Join(db.root, cachedGCMsDir, rgcm.MsgID.String()) diff --git a/internal/e2etests/gc_test.go b/internal/e2etests/gc_test.go index bdb19a45..3a96613d 100644 --- a/internal/e2etests/gc_test.go +++ b/internal/e2etests/gc_test.go @@ -1320,3 +1320,74 @@ func TestGCInviteExpiration(t *testing.T) { assert.NilErr(t, bob.AcceptGroupChatInvite(iid3)) assert.ChanWritten(t, bobJoinedGCChan) } + +// TestGCAutoReinviteIdleKickedMember verifies that, when auto-remove-idle is +// enabled, a GC admin re-adds a member it previously idle-kicked once that +// member sends a new GC message. This models a member that was offline when it +// was idle-kicked, so it never saw the kick and still believes it is a member. +func TestGCAutoReinviteIdleKickedMember(t *testing.T) { + t.Parallel() + + tcfg := testScaffoldCfg{} + ts := newTestScaffold(t, tcfg) + + // Alice is the admin. Leaving AutoRemoveIdleUsersInterval at its default + // (non-zero) value is what enables the auto re-invite feature. + alice := ts.newClient("alice") + bob := ts.newClient("bob") + + ts.kxUsers(alice, bob) + + // Alice creates a GC and Bob joins. + gcID, err := alice.NewGroupChat("test_gc") + assert.NilErr(t, err) + bobAcceptedChan := bob.acceptNextGCInvite(gcID) + assert.NilErr(t, alice.InviteToGroupChat(gcID, bob.PublicID())) + assert.NilErrFromChan(t, bobAcceptedChan) + assertClientInGC(t, bob, gcID) + assertClientSeesInGC(t, alice, gcID, bob.PublicID()) + + // Pause Bob right before he applies the incoming kick, so that (like an + // offline member) he never processes it, keeps the GC locally and still + // believes he is a member. The received notification fires on the RV + // manager goroutine before the kick handler runs, so blocking here keeps + // Bob from removing the GC. + bobGotKickChan := make(chan struct{}, 1) + bobContChan := make(chan struct{}) + bob.handleSync(client.OnRMReceived(func(_ *client.RemoteUser, _ *rpc.RMHeader, p interface{}, _ time.Time) { + if _, ok := p.(rpc.RMGroupKick); ok { + bobGotKickChan <- struct{}{} + <-bobContChan + } + })) + + // Track Alice re-adding members (the behavior under test). Registered + // after the join so it only captures the re-invite. + wantUIDs := []clientintf.UserID{bob.PublicID()} + aliceReAddedChan := make(chan []clientintf.UserID, 4) + alice.handle(client.OnAddedGCMembersNtfn(func(_ rpc.RMGroupList, uids []clientintf.UserID) { + aliceReAddedChan <- uids + })) + + // Alice idle-kicks Bob. The reason must match the one the idle unsub + // machinery uses for the kick to be tracked for re-invite. + const idleReason = "User is idle for too long" + assert.NilErr(t, alice.GCKick(gcID, bob.PublicID(), idleReason)) + + // Bob receives the kick but is paused before applying it, so he still + // holds the GC. + assert.ChanWritten(t, bobGotKickChan) + + // Bob, still believing he is a member, sends a new message. Alice should + // re-add Bob and broadcast the updated member list instead of dropping + // the message. + assert.NilErr(t, bob.GCMessage(gcID, "i am back", rpc.MessageModeNormal, nil)) + + // Alice re-adds Bob and lists him as a member again. + assert.ChanWrittenWithVal(t, aliceReAddedChan, wantUIDs) + assertClientSeesInGC(t, alice, gcID, bob.PublicID()) + + // Let Bob finish processing the kick to avoid leaking the paused + // goroutine. + close(bobContChan) +}