Skip to content

Commit

Permalink
fix(GODT-1616): close not sending unilateral updates.
Browse files Browse the repository at this point in the history
  • Loading branch information
@cuthix @LBeernaertProton
cuthix authored and LBeernaertProton committed Sep 19, 2022
1 parent 5bf5ac4 commit 47e2fb7
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 40 deletions.
18 changes: 8 additions & 10 deletions internal/state/responders.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"sync"

"github.com/ProtonMail/gluon/imap"
"github.com/ProtonMail/gluon/internal/contexts"
"github.com/ProtonMail/gluon/internal/db"
"github.com/ProtonMail/gluon/internal/db/ent"
"github.com/ProtonMail/gluon/internal/ids"
Expand Down Expand Up @@ -52,7 +53,7 @@ func NewMessageIDAndMailboxIDResponderStateUpdate(messageID imap.InternalMessage

type Responder interface {
// handle generates responses in the context of the given snapshot.
handle(snap *snapshot, stateID StateID) ([]response.Response, responderDBUpdate, error)
handle(ctx context.Context, snap *snapshot, stateID StateID) ([]response.Response, responderDBUpdate, error)

// getMessageID returns the message ID that this Responder targets.
getMessageID() imap.InternalMessageID
Expand Down Expand Up @@ -90,7 +91,7 @@ type targetedExists struct {
targetStateID StateID
}

func (u *targetedExists) handle(snap *snapshot, stateID StateID) ([]response.Response, responderDBUpdate, error) {
func (u *targetedExists) handle(_ context.Context, snap *snapshot, stateID StateID) ([]response.Response, responderDBUpdate, error) {
if snap.hasMessage(u.resp.messageID.InternalID) {
return nil, nil, nil
}
Expand Down Expand Up @@ -229,17 +230,15 @@ func (e *ExistsStateUpdate) String() string {

type expunge struct {
messageID imap.InternalMessageID
asClose bool
}

func NewExpunge(messageID imap.InternalMessageID, asClose bool) *expunge {
func NewExpunge(messageID imap.InternalMessageID) *expunge {
return &expunge{
messageID: messageID,
asClose: asClose,
}
}

func (u *expunge) handle(snap *snapshot, _ StateID) ([]response.Response, responderDBUpdate, error) {
func (u *expunge) handle(ctx context.Context, snap *snapshot, _ StateID) ([]response.Response, responderDBUpdate, error) {
if !snap.hasMessage(u.messageID) {
return nil, nil, nil
}
Expand All @@ -254,7 +253,7 @@ func (u *expunge) handle(snap *snapshot, _ StateID) ([]response.Response, respon
}

// When handling a CLOSE command, EXPUNGE responses are not sent.
if u.asClose {
if contexts.IsClose(ctx) {
return nil, nil, nil
}

Expand All @@ -266,9 +265,8 @@ func (u *expunge) getMessageID() imap.InternalMessageID {
}

func (u *expunge) String() string {
return fmt.Sprintf("Expung: message = %v closed = %v",
return fmt.Sprintf("Expung: message = %v",
u.messageID.ShortID(),
u.asClose,
)
}

Expand Down Expand Up @@ -299,7 +297,7 @@ func NewFetch(messageID imap.InternalMessageID, flags imap.FlagSet, asUID, asSil
}
}

func (u *fetch) handle(snap *snapshot, _ StateID) ([]response.Response, responderDBUpdate, error) {
func (u *fetch) handle(_ context.Context, snap *snapshot, _ StateID) ([]response.Response, responderDBUpdate, error) {
if !snap.hasMessage(u.messageID) {
return nil, nil, nil
}
Expand Down
4 changes: 2 additions & 2 deletions internal/state/state.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ func (state *State) flushResponses(ctx context.Context, permitExpunge bool) ([]r
for _, responder := range state.popResponders(permitExpunge) {
logrus.WithField("state", state.StateID).WithField("Origin", "Flush").Tracef("Applying responder: %v", responder.String())

res, dbUpdate, err := responder.handle(state.snap, state.StateID)
res, dbUpdate, err := responder.handle(ctx, state.snap, state.StateID)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -567,7 +567,7 @@ func (state *State) PushResponder(ctx context.Context, tx *ent.Tx, responder ...
for _, responder := range responder {
logrus.WithField("state", state.StateID).WithField("Origin", "Push").Tracef("Applying responder: %v", responder.String())

res, dbUpdate, err := responder.handle(state.snap, state.StateID)
res, dbUpdate, err := responder.handle(ctx, state.snap, state.StateID)
if err != nil {
return err
}
Expand Down
5 changes: 2 additions & 3 deletions internal/state/updates_mailbox.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"

"github.com/ProtonMail/gluon/imap"
"github.com/ProtonMail/gluon/internal/contexts"
"github.com/ProtonMail/gluon/internal/db"
"github.com/ProtonMail/gluon/internal/db/ent"
"github.com/ProtonMail/gluon/internal/ids"
Expand Down Expand Up @@ -46,7 +45,7 @@ func MoveMessagesFromMailbox(
}

for _, messageID := range messageIDs {
stateUpdates = append(stateUpdates, NewMessageIDAndMailboxIDResponderStateUpdate(messageID, mboxFromID, NewExpunge(messageID, contexts.IsClose(ctx))))
stateUpdates = append(stateUpdates, NewMessageIDAndMailboxIDResponderStateUpdate(messageID, mboxFromID, NewExpunge(messageID)))
}

return messageUIDs, stateUpdates, nil
Expand Down Expand Up @@ -80,7 +79,7 @@ func RemoveMessagesFromMailbox(ctx context.Context, tx *ent.Tx, mboxID imap.Inte
}

stateUpdates := xslices.Map(messageIDs, func(id imap.InternalMessageID) Update {
return NewMessageIDAndMailboxIDResponderStateUpdate(id, mboxID, NewExpunge(id, contexts.IsClose(ctx)))
return NewMessageIDAndMailboxIDResponderStateUpdate(id, mboxID, NewExpunge(id))
})

return stateUpdates, nil
Expand Down
80 changes: 55 additions & 25 deletions tests/close_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,40 +14,70 @@ func TestClose(t *testing.T) {
// This test is still useful as we have no way of checking the fetch responses after the store command.
// Additionally, we also need to ensure that there are no unilateral EXPUNGE messages returned from the server after close.
// There is currently no way to check for this with the go imap client.
runOneToOneTestWithAuth(t, defaultServerOptions(t), func(c *testConnection, _ *testSession) {
c.C("b001 CREATE saved-messages")
c.S("b001 OK CREATE")
runManyToOneTestWithAuth(t, defaultServerOptions(t), []int{1, 2}, func(c map[int]*testConnection, _ *testSession) {
c[1].C("b001 CREATE saved-messages")
c[1].S("b001 OK CREATE")

c.doAppend(`saved-messages`, `To: 1@pm.me`, `\Seen`).expect("OK")
c.doAppend(`saved-messages`, `To: 2@pm.me`).expect("OK")
c.doAppend(`saved-messages`, `To: 3@pm.me`, `\Seen`).expect("OK")
c.doAppend(`saved-messages`, `To: 4@pm.me`).expect("OK")
c.doAppend(`saved-messages`, `To: 5@pm.me`, `\Seen`).expect("OK")
c[1].doAppend(`saved-messages`, `To: 1@pm.me`, `\Seen`).expect("OK")
c[1].doAppend(`saved-messages`, `To: 2@pm.me`).expect("OK")
c[1].doAppend(`saved-messages`, `To: 3@pm.me`, `\Seen`).expect("OK")
c[1].doAppend(`saved-messages`, `To: 4@pm.me`).expect("OK")
c[1].doAppend(`saved-messages`, `To: 5@pm.me`, `\Seen`).expect("OK")

c.C(`A002 SELECT saved-messages`)
c.Se(`A002 OK [READ-WRITE] SELECT`)
c[1].C(`A002 SELECT saved-messages`)
c[1].Se(`A002 OK [READ-WRITE] SELECT`)

c[2].C(`B001 SELECT saved-messages`)
c[2].Se(`B001 OK [READ-WRITE] SELECT`)
c[2].C(`B002 FETCH 1:* (UID FLAGS)`)
c[2].S(
`* 1 FETCH (UID 1 FLAGS (\Seen))`,
`* 2 FETCH (UID 2 FLAGS ())`,
`* 3 FETCH (UID 3 FLAGS (\Seen))`,
`* 4 FETCH (UID 4 FLAGS ())`,
`* 5 FETCH (UID 5 FLAGS (\Seen))`,
)
c[2].OK("B002")

// TODO: Match flags in any order.
c.C(`A003 STORE 1 +FLAGS (\Deleted)`)
c.S(`* 1 FETCH (FLAGS (\Deleted \Recent \Seen))`)
c.Sx("A003 OK.*")
c[1].C(`A003 STORE 1 +FLAGS (\Deleted)`)
c[1].S(`* 1 FETCH (FLAGS (\Deleted \Recent \Seen))`)
c[1].Sx("A003 OK.*")

c.C(`A004 STORE 2 +FLAGS (\Deleted)`)
c.S(`* 2 FETCH (FLAGS (\Deleted \Recent))`)
c.Sx("A004 OK.*")
c[1].C(`A004 STORE 2 +FLAGS (\Deleted)`)
c[1].S(`* 2 FETCH (FLAGS (\Deleted \Recent))`)
c[1].Sx("A004 OK.*")

c.C(`A005 STORE 3 +FLAGS (\Deleted)`)
c.S(`* 3 FETCH (FLAGS (\Deleted \Recent \Seen))`)
c.Sx("A005 OK.*")
c[1].C(`A005 STORE 3 +FLAGS (\Deleted)`)
c[1].S(`* 3 FETCH (FLAGS (\Deleted \Recent \Seen))`)
c[1].Sx("A005 OK.*")

c[2].C("B003 NOOP")
c[2].S(
`* 1 FETCH (FLAGS (\Deleted \Seen))`,
`* 2 FETCH (FLAGS (\Deleted))`,
`* 3 FETCH (FLAGS (\Deleted \Seen))`,
)
c[2].OK("B003")

// TODO: GOMSRV-106 - Ensure this also works for cases where multiple clients have the same mailbox open
c.C(`A202 CLOSE`)
c.S("A202 OK CLOSE")
c[2].C("B004 UID EXPUNGE 1")
c[2].Se(`* 1 EXPUNGE`)
c[2].OK("B004")

c[1].C(`A202 CLOSE`)
c[1].S("A202 OK CLOSE")

c[2].C("B003 NOOP")
c[2].S(
`* 1 EXPUNGE`,
`* 1 EXPUNGE`,
)
c[2].OK("B003")

// There are 2 messages in saved-messages.
c.C(`A006 STATUS saved-messages (MESSAGES)`)
c.S(`* STATUS "saved-messages" (MESSAGES 2)`)
c.S(`A006 OK STATUS`)
c[1].C(`A006 STATUS saved-messages (MESSAGES)`)
c[1].S(`* STATUS "saved-messages" (MESSAGES 2)`)
c[1].S(`A006 OK STATUS`)
})
}

Expand Down

0 comments on commit 47e2fb7

Please sign in to comment.