From 64472d9aab85bf56fb345570459c372b78a14b8c Mon Sep 17 00:00:00 2001 From: Till <2353100+S7evinK@users.noreply.github.com> Date: Fri, 9 Sep 2022 13:56:33 +0200 Subject: [PATCH] Update getting pushrules, add tests, tweak pushrules (#2705) This PR - adds tests for `evaluatePushrules` - removes the need for the UserAPI on the `OutputStreamEventConsumer` (for easier testing) - adds a method to get the pushrules from the database - adds a new default pushrule for `m.reaction` events (and some other tweaks) --- internal/pushrules/default_content.go | 6 + internal/pushrules/default_override.go | 23 +++- internal/pushrules/default_underride.go | 30 +++- internal/pushrules/evaluate_test.go | 32 +++-- internal/pushrules/util.go | 2 +- userapi/consumers/syncapi_streamevent.go | 13 +- userapi/consumers/syncapi_streamevent_test.go | 129 ++++++++++++++++++ userapi/internal/api.go | 57 +------- userapi/storage/interface.go | 2 + userapi/storage/shared/storage.go | 38 +++++- userapi/userapi.go | 5 +- 11 files changed, 255 insertions(+), 82 deletions(-) create mode 100644 userapi/consumers/syncapi_streamevent_test.go diff --git a/internal/pushrules/default_content.go b/internal/pushrules/default_content.go index 158afd18b..8982dd587 100644 --- a/internal/pushrules/default_content.go +++ b/internal/pushrules/default_content.go @@ -16,6 +16,12 @@ func mRuleContainsUserNameDefinition(localpart string) *Rule { Default: true, Enabled: true, Pattern: localpart, + Conditions: []*Condition{ + { + Kind: EventMatchCondition, + Key: "content.body", + }, + }, Actions: []*Action{ {Kind: NotifyAction}, { diff --git a/internal/pushrules/default_override.go b/internal/pushrules/default_override.go index 6f66fd66a..a9788df2f 100644 --- a/internal/pushrules/default_override.go +++ b/internal/pushrules/default_override.go @@ -7,8 +7,9 @@ func defaultOverrideRules(userID string) []*Rule { mRuleInviteForMeDefinition(userID), &mRuleMemberEventDefinition, &mRuleContainsDisplayNameDefinition, - &mRuleTombstoneDefinition, &mRuleRoomNotifDefinition, + &mRuleTombstoneDefinition, + &mRuleReactionDefinition, } } @@ -20,6 +21,7 @@ const ( MRuleContainsDisplayName = ".m.rule.contains_display_name" MRuleTombstone = ".m.rule.tombstone" MRuleRoomNotif = ".m.rule.roomnotif" + MRuleReaction = ".m.rule.reaction" ) var ( @@ -96,7 +98,7 @@ var ( { Kind: SetTweakAction, Tweak: HighlightTweak, - Value: false, + Value: true, }, }, } @@ -120,10 +122,25 @@ var ( { Kind: SetTweakAction, Tweak: HighlightTweak, - Value: false, + Value: true, }, }, } + mRuleReactionDefinition = Rule{ + RuleID: MRuleReaction, + Default: true, + Enabled: true, + Conditions: []*Condition{ + { + Kind: EventMatchCondition, + Key: "type", + Pattern: "m.reaction", + }, + }, + Actions: []*Action{ + {Kind: DontNotifyAction}, + }, + } ) func mRuleInviteForMeDefinition(userID string) *Rule { diff --git a/internal/pushrules/default_underride.go b/internal/pushrules/default_underride.go index de72bd526..8da449a19 100644 --- a/internal/pushrules/default_underride.go +++ b/internal/pushrules/default_underride.go @@ -10,8 +10,8 @@ const ( var defaultUnderrideRules = []*Rule{ &mRuleCallDefinition, - &mRuleEncryptedRoomOneToOneDefinition, &mRuleRoomOneToOneDefinition, + &mRuleEncryptedRoomOneToOneDefinition, &mRuleMessageDefinition, &mRuleEncryptedDefinition, } @@ -59,6 +59,11 @@ var ( }, Actions: []*Action{ {Kind: NotifyAction}, + { + Kind: SetTweakAction, + Tweak: SoundTweak, + Value: "default", + }, { Kind: SetTweakAction, Tweak: HighlightTweak, @@ -88,6 +93,11 @@ var ( Tweak: HighlightTweak, Value: false, }, + { + Kind: SetTweakAction, + Tweak: HighlightTweak, + Value: false, + }, }, } mRuleMessageDefinition = Rule{ @@ -101,7 +111,14 @@ var ( Pattern: "m.room.message", }, }, - Actions: []*Action{{Kind: NotifyAction}}, + Actions: []*Action{ + {Kind: NotifyAction}, + { + Kind: SetTweakAction, + Tweak: HighlightTweak, + Value: false, + }, + }, } mRuleEncryptedDefinition = Rule{ RuleID: MRuleEncrypted, @@ -114,6 +131,13 @@ var ( Pattern: "m.room.encrypted", }, }, - Actions: []*Action{{Kind: NotifyAction}}, + Actions: []*Action{ + {Kind: NotifyAction}, + { + Kind: SetTweakAction, + Tweak: HighlightTweak, + Value: false, + }, + }, } ) diff --git a/internal/pushrules/evaluate_test.go b/internal/pushrules/evaluate_test.go index 50e703365..eabd02415 100644 --- a/internal/pushrules/evaluate_test.go +++ b/internal/pushrules/evaluate_test.go @@ -24,24 +24,28 @@ func TestRuleSetEvaluatorMatchEvent(t *testing.T) { Default: false, Enabled: true, } + defaultRuleset := DefaultGlobalRuleSet("test", "test") tsts := []struct { Name string RuleSet RuleSet Want *Rule + Event *gomatrixserverlib.Event }{ - {"empty", RuleSet{}, nil}, - {"defaultCanWin", RuleSet{Override: []*Rule{defaultEnabled}}, defaultEnabled}, - {"userWins", RuleSet{Override: []*Rule{defaultEnabled, userEnabled}}, userEnabled}, - {"defaultOverrideWins", RuleSet{Override: []*Rule{defaultEnabled}, Underride: []*Rule{userEnabled}}, defaultEnabled}, - {"overrideContent", RuleSet{Override: []*Rule{userEnabled}, Content: []*Rule{userEnabled2}}, userEnabled}, - {"overrideRoom", RuleSet{Override: []*Rule{userEnabled}, Room: []*Rule{userEnabled2}}, userEnabled}, - {"overrideSender", RuleSet{Override: []*Rule{userEnabled}, Sender: []*Rule{userEnabled2}}, userEnabled}, - {"overrideUnderride", RuleSet{Override: []*Rule{userEnabled}, Underride: []*Rule{userEnabled2}}, userEnabled}, + {"empty", RuleSet{}, nil, ev}, + {"defaultCanWin", RuleSet{Override: []*Rule{defaultEnabled}}, defaultEnabled, ev}, + {"userWins", RuleSet{Override: []*Rule{defaultEnabled, userEnabled}}, userEnabled, ev}, + {"defaultOverrideWins", RuleSet{Override: []*Rule{defaultEnabled}, Underride: []*Rule{userEnabled}}, defaultEnabled, ev}, + {"overrideContent", RuleSet{Override: []*Rule{userEnabled}, Content: []*Rule{userEnabled2}}, userEnabled, ev}, + {"overrideRoom", RuleSet{Override: []*Rule{userEnabled}, Room: []*Rule{userEnabled2}}, userEnabled, ev}, + {"overrideSender", RuleSet{Override: []*Rule{userEnabled}, Sender: []*Rule{userEnabled2}}, userEnabled, ev}, + {"overrideUnderride", RuleSet{Override: []*Rule{userEnabled}, Underride: []*Rule{userEnabled2}}, userEnabled, ev}, + {"reactions don't notify", *defaultRuleset, &mRuleReactionDefinition, mustEventFromJSON(t, `{"type":"m.reaction"}`)}, + {"receipts don't notify", *defaultRuleset, nil, mustEventFromJSON(t, `{"type":"m.receipt"}`)}, } for _, tst := range tsts { t.Run(tst.Name, func(t *testing.T) { - rse := NewRuleSetEvaluator(nil, &tst.RuleSet) - got, err := rse.MatchEvent(ev) + rse := NewRuleSetEvaluator(fakeEvaluationContext{3}, &tst.RuleSet) + got, err := rse.MatchEvent(tst.Event) if err != nil { t.Fatalf("MatchEvent failed: %v", err) } @@ -128,7 +132,7 @@ func TestConditionMatches(t *testing.T) { } for _, tst := range tsts { t.Run(tst.Name, func(t *testing.T) { - got, err := conditionMatches(&tst.Cond, mustEventFromJSON(t, tst.EventJSON), &fakeEvaluationContext{}) + got, err := conditionMatches(&tst.Cond, mustEventFromJSON(t, tst.EventJSON), &fakeEvaluationContext{2}) if err != nil { t.Fatalf("conditionMatches failed: %v", err) } @@ -139,10 +143,10 @@ func TestConditionMatches(t *testing.T) { } } -type fakeEvaluationContext struct{} +type fakeEvaluationContext struct{ memberCount int } -func (fakeEvaluationContext) UserDisplayName() string { return "Dear User" } -func (fakeEvaluationContext) RoomMemberCount() (int, error) { return 2, nil } +func (fakeEvaluationContext) UserDisplayName() string { return "Dear User" } +func (f fakeEvaluationContext) RoomMemberCount() (int, error) { return f.memberCount, nil } func (fakeEvaluationContext) HasPowerLevel(userID, levelKey string) (bool, error) { return userID == "@poweruser:example.com" && levelKey == "powerlevel", nil } diff --git a/internal/pushrules/util.go b/internal/pushrules/util.go index 027d35ef6..8ab4eab94 100644 --- a/internal/pushrules/util.go +++ b/internal/pushrules/util.go @@ -11,7 +11,7 @@ import ( // kind and a tweaks map. Returns a nil map if it would have been // empty. func ActionsToTweaks(as []*Action) (ActionKind, map[string]interface{}, error) { - var kind ActionKind + kind := UnknownAction tweaks := map[string]interface{}{} for _, a := range as { diff --git a/userapi/consumers/syncapi_streamevent.go b/userapi/consumers/syncapi_streamevent.go index 3ac6f58d0..f3b2bf27f 100644 --- a/userapi/consumers/syncapi_streamevent.go +++ b/userapi/consumers/syncapi_streamevent.go @@ -29,7 +29,6 @@ import ( type OutputStreamEventConsumer struct { ctx context.Context cfg *config.UserAPI - userAPI api.UserInternalAPI rsAPI rsapi.UserRoomserverAPI jetstream nats.JetStreamContext durable string @@ -45,7 +44,6 @@ func NewOutputStreamEventConsumer( js nats.JetStreamContext, store storage.Database, pgClient pushgateway.Client, - userAPI api.UserInternalAPI, rsAPI rsapi.UserRoomserverAPI, syncProducer *producers.SyncAPI, ) *OutputStreamEventConsumer { @@ -57,7 +55,6 @@ func NewOutputStreamEventConsumer( durable: cfg.Matrix.JetStream.Durable("UserAPISyncAPIStreamEventConsumer"), topic: cfg.Matrix.JetStream.Prefixed(jetstream.OutputStreamEvent), pgClient: pgClient, - userAPI: userAPI, rsAPI: rsAPI, syncProducer: syncProducer, } @@ -305,7 +302,7 @@ func (s *OutputStreamEventConsumer) notifyLocal(ctx context.Context, event *goma "event_id": event.EventID(), "room_id": event.RoomID(), "localpart": mem.Localpart, - }).Tracef("Push rule evaluation rejected the event") + }).Debugf("Push rule evaluation rejected the event") return nil } @@ -348,7 +345,7 @@ func (s *OutputStreamEventConsumer) notifyLocal(ctx context.Context, event *goma "localpart": mem.Localpart, "num_urls": len(devicesByURLAndFormat), "num_unread": userNumUnreadNotifs, - }).Tracef("Notifying single member") + }).Debugf("Notifying single member") // Push gateways are out of our control, and we cannot risk // looking up the server on a misbehaving push gateway. Each user @@ -422,8 +419,8 @@ func (s *OutputStreamEventConsumer) evaluatePushRules(ctx context.Context, event return nil, fmt.Errorf("user %s is ignored", sender) } } - var res api.QueryPushRulesResponse - if err = s.userAPI.QueryPushRules(ctx, &api.QueryPushRulesRequest{UserID: mem.UserID}, &res); err != nil { + ruleSets, err := s.db.QueryPushRules(ctx, mem.Localpart) + if err != nil { return nil, err } @@ -434,7 +431,7 @@ func (s *OutputStreamEventConsumer) evaluatePushRules(ctx context.Context, event roomID: event.RoomID(), roomSize: roomSize, } - eval := pushrules.NewRuleSetEvaluator(ec, &res.RuleSets.Global) + eval := pushrules.NewRuleSetEvaluator(ec, &ruleSets.Global) rule, err := eval.MatchEvent(event.Event) if err != nil { return nil, err diff --git a/userapi/consumers/syncapi_streamevent_test.go b/userapi/consumers/syncapi_streamevent_test.go new file mode 100644 index 000000000..48ea0fe11 --- /dev/null +++ b/userapi/consumers/syncapi_streamevent_test.go @@ -0,0 +1,129 @@ +package consumers + +import ( + "context" + "testing" + + "github.com/matrix-org/gomatrixserverlib" + "github.com/stretchr/testify/assert" + + "github.com/matrix-org/dendrite/internal/pushrules" + "github.com/matrix-org/dendrite/setup/config" + "github.com/matrix-org/dendrite/test" + "github.com/matrix-org/dendrite/userapi/storage" +) + +func mustCreateDatabase(t *testing.T, dbType test.DBType) (storage.Database, func()) { + t.Helper() + connStr, close := test.PrepareDBConnectionString(t, dbType) + db, err := storage.NewUserAPIDatabase(nil, &config.DatabaseOptions{ + ConnectionString: config.DataSource(connStr), + }, "", 4, 0, 0, "") + if err != nil { + t.Fatalf("failed to create new user db: %v", err) + } + return db, close +} + +func mustCreateEvent(t *testing.T, content string) *gomatrixserverlib.HeaderedEvent { + t.Helper() + ev, err := gomatrixserverlib.NewEventFromTrustedJSON([]byte(content), false, gomatrixserverlib.RoomVersionV10) + if err != nil { + t.Fatalf("failed to create event: %v", err) + } + return ev.Headered(gomatrixserverlib.RoomVersionV10) +} + +func Test_evaluatePushRules(t *testing.T) { + ctx := context.Background() + + test.WithAllDatabases(t, func(t *testing.T, dbType test.DBType) { + db, close := mustCreateDatabase(t, dbType) + defer close() + consumer := OutputStreamEventConsumer{db: db} + + testCases := []struct { + name string + eventContent string + wantAction pushrules.ActionKind + wantActions []*pushrules.Action + wantNotify bool + }{ + { + name: "m.receipt doesn't notify", + eventContent: `{"type":"m.receipt"}`, + wantAction: pushrules.UnknownAction, + wantActions: nil, + }, + { + name: "m.reaction doesn't notify", + eventContent: `{"type":"m.reaction"}`, + wantAction: pushrules.DontNotifyAction, + wantActions: []*pushrules.Action{ + { + Kind: pushrules.DontNotifyAction, + }, + }, + }, + { + name: "m.room.message notifies", + eventContent: `{"type":"m.room.message"}`, + wantNotify: true, + wantAction: pushrules.NotifyAction, + wantActions: []*pushrules.Action{ + {Kind: pushrules.NotifyAction}, + { + Kind: pushrules.SetTweakAction, + Tweak: pushrules.HighlightTweak, + Value: false, + }, + }, + }, + { + name: "m.room.message highlights", + eventContent: `{"type":"m.room.message", "content": {"body": "test"} }`, + wantNotify: true, + wantAction: pushrules.NotifyAction, + wantActions: []*pushrules.Action{ + {Kind: pushrules.NotifyAction}, + { + Kind: pushrules.SetTweakAction, + Tweak: pushrules.SoundTweak, + Value: "default", + }, + { + Kind: pushrules.SetTweakAction, + Tweak: pushrules.HighlightTweak, + Value: true, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + actions, err := consumer.evaluatePushRules(ctx, mustCreateEvent(t, tc.eventContent), &localMembership{ + UserID: "@test:localhost", + Localpart: "test", + Domain: "localhost", + }, 10) + if err != nil { + t.Fatalf("failed to evaluate push rules: %v", err) + } + assert.Equal(t, tc.wantActions, actions) + gotAction, _, err := pushrules.ActionsToTweaks(actions) + if err != nil { + t.Fatalf("failed to get actions: %v", err) + } + if gotAction != tc.wantAction { + t.Fatalf("expected action to be '%s', got '%s'", tc.wantAction, gotAction) + } + // this is taken from `notifyLocal` + if tc.wantNotify && gotAction != pushrules.NotifyAction && gotAction != pushrules.CoalesceAction { + t.Fatalf("expected to notify but didn't") + } + }) + + } + }) +} diff --git a/userapi/internal/api.go b/userapi/internal/api.go index 2f28ee906..dcbb73614 100644 --- a/userapi/internal/api.go +++ b/userapi/internal/api.go @@ -30,7 +30,6 @@ import ( "github.com/matrix-org/dendrite/clientapi/userutil" "github.com/matrix-org/dendrite/internal/eventutil" - "github.com/matrix-org/dendrite/internal/pushrules" "github.com/matrix-org/dendrite/internal/sqlutil" keyapi "github.com/matrix-org/dendrite/keyserver/api" rsapi "github.com/matrix-org/dendrite/roomserver/api" @@ -760,57 +759,15 @@ func (a *UserInternalAPI) PerformPushRulesPut( } func (a *UserInternalAPI) QueryPushRules(ctx context.Context, req *api.QueryPushRulesRequest, res *api.QueryPushRulesResponse) error { - userReq := api.QueryAccountDataRequest{ - UserID: req.UserID, - DataType: pushRulesAccountDataType, + localpart, _, err := gomatrixserverlib.SplitID('@', req.UserID) + if err != nil { + return fmt.Errorf("failed to split user ID %q for push rules", req.UserID) } - var userRes api.QueryAccountDataResponse - if err := a.QueryAccountData(ctx, &userReq, &userRes); err != nil { - return err + pushRules, err := a.DB.QueryPushRules(ctx, localpart) + if err != nil { + return fmt.Errorf("failed to query push rules: %w", err) } - bs, ok := userRes.GlobalAccountData[pushRulesAccountDataType] - if ok { - // Legacy Dendrite users will have completely empty push rules, so we should - // detect that situation and set some defaults. - var rules struct { - G struct { - Content []json.RawMessage `json:"content"` - Override []json.RawMessage `json:"override"` - Room []json.RawMessage `json:"room"` - Sender []json.RawMessage `json:"sender"` - Underride []json.RawMessage `json:"underride"` - } `json:"global"` - } - if err := json.Unmarshal([]byte(bs), &rules); err == nil { - count := len(rules.G.Content) + len(rules.G.Override) + - len(rules.G.Room) + len(rules.G.Sender) + len(rules.G.Underride) - ok = count > 0 - } - } - if !ok { - // If we didn't find any default push rules then we should just generate some - // fresh ones. - localpart, _, err := gomatrixserverlib.SplitID('@', req.UserID) - if err != nil { - return fmt.Errorf("failed to split user ID %q for push rules", req.UserID) - } - pushRuleSets := pushrules.DefaultAccountRuleSets(localpart, a.ServerName) - prbs, err := json.Marshal(pushRuleSets) - if err != nil { - return fmt.Errorf("failed to marshal default push rules: %w", err) - } - if err := a.DB.SaveAccountData(ctx, localpart, "", pushRulesAccountDataType, json.RawMessage(prbs)); err != nil { - return fmt.Errorf("failed to save default push rules: %w", err) - } - res.RuleSets = pushRuleSets - return nil - } - var data pushrules.AccountRuleSets - if err := json.Unmarshal([]byte(bs), &data); err != nil { - util.GetLogger(ctx).WithError(err).Error("json.Unmarshal of push rules failed") - return err - } - res.RuleSets = &data + res.RuleSets = pushRules return nil } diff --git a/userapi/storage/interface.go b/userapi/storage/interface.go index f7cd1810a..fbac463e2 100644 --- a/userapi/storage/interface.go +++ b/userapi/storage/interface.go @@ -20,6 +20,7 @@ import ( "errors" "github.com/matrix-org/dendrite/clientapi/auth/authtypes" + "github.com/matrix-org/dendrite/internal/pushrules" "github.com/matrix-org/dendrite/userapi/api" "github.com/matrix-org/dendrite/userapi/storage/tables" "github.com/matrix-org/dendrite/userapi/types" @@ -53,6 +54,7 @@ type AccountData interface { // If no account data could be found, returns nil // Returns an error if there was an issue with the retrieval GetAccountDataByType(ctx context.Context, localpart, roomID, dataType string) (data json.RawMessage, err error) + QueryPushRules(ctx context.Context, localpart string) (*pushrules.AccountRuleSets, error) } type Device interface { diff --git a/userapi/storage/shared/storage.go b/userapi/storage/shared/storage.go index 0cf713dac..d968b47a9 100644 --- a/userapi/storage/shared/storage.go +++ b/userapi/storage/shared/storage.go @@ -26,10 +26,11 @@ import ( "strings" "time" - "github.com/matrix-org/dendrite/userapi/types" "github.com/matrix-org/gomatrixserverlib" "golang.org/x/crypto/bcrypt" + "github.com/matrix-org/dendrite/userapi/types" + "github.com/matrix-org/dendrite/clientapi/auth/authtypes" "github.com/matrix-org/dendrite/internal/pushrules" "github.com/matrix-org/dendrite/internal/sqlutil" @@ -177,6 +178,41 @@ func (d *Database) createAccount( return account, nil } +func (d *Database) QueryPushRules( + ctx context.Context, + localpart string, +) (*pushrules.AccountRuleSets, error) { + data, err := d.AccountDatas.SelectAccountDataByType(ctx, localpart, "", "m.push_rules") + if err != nil { + return nil, err + } + + // If we didn't find any default push rules then we should just generate some + // fresh ones. + if len(data) == 0 { + pushRuleSets := pushrules.DefaultAccountRuleSets(localpart, d.ServerName) + prbs, err := json.Marshal(pushRuleSets) + if err != nil { + return nil, fmt.Errorf("failed to marshal default push rules: %w", err) + } + err = d.Writer.Do(d.DB, nil, func(txn *sql.Tx) error { + if dbErr := d.AccountDatas.InsertAccountData(ctx, txn, localpart, "", "m.push_rules", prbs); dbErr != nil { + return fmt.Errorf("failed to save default push rules: %w", dbErr) + } + return nil + }) + + return pushRuleSets, err + } + + var pushRules pushrules.AccountRuleSets + if err := json.Unmarshal(data, &pushRules); err != nil { + return nil, err + } + + return &pushRules, nil +} + // SaveAccountData saves new account data for a given user and a given room. // If the account data is not specific to a room, the room ID should be an empty string // If an account data already exists for a given set (user, room, data type), it will diff --git a/userapi/userapi.go b/userapi/userapi.go index 2e86d6aa7..23855a89f 100644 --- a/userapi/userapi.go +++ b/userapi/userapi.go @@ -18,6 +18,8 @@ import ( "time" "github.com/gorilla/mux" + "github.com/sirupsen/logrus" + "github.com/matrix-org/dendrite/internal/pushgateway" keyapi "github.com/matrix-org/dendrite/keyserver/api" rsapi "github.com/matrix-org/dendrite/roomserver/api" @@ -31,7 +33,6 @@ import ( "github.com/matrix-org/dendrite/userapi/producers" "github.com/matrix-org/dendrite/userapi/storage" "github.com/matrix-org/dendrite/userapi/util" - "github.com/sirupsen/logrus" ) // AddInternalRoutes registers HTTP handlers for the internal API. Invokes functions @@ -90,7 +91,7 @@ func NewInternalAPI( } eventConsumer := consumers.NewOutputStreamEventConsumer( - base.ProcessContext, cfg, js, db, pgClient, userAPI, rsAPI, syncProducer, + base.ProcessContext, cfg, js, db, pgClient, rsAPI, syncProducer, ) if err := eventConsumer.Start(); err != nil { logrus.WithError(err).Panic("failed to start user API streamed event consumer")