From 1b7ce13d39e75b0a4bafec84e585ac5f89a6ccf9 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Mon, 23 Oct 2023 16:35:14 +0200 Subject: [PATCH 1/5] Implement MSC3987 --- clientapi/clientapi_test.go | 2 +- internal/pushrules/action_test.go | 2 -- internal/pushrules/default_override.go | 10 ++++------ internal/pushrules/default_pushrules_test.go | 6 +++--- internal/pushrules/util.go | 5 +---- internal/pushrules/util_test.go | 5 ++--- userapi/consumers/roomserver.go | 4 ++-- userapi/consumers/roomserver_test.go | 10 +++------- 8 files changed, 16 insertions(+), 28 deletions(-) diff --git a/clientapi/clientapi_test.go b/clientapi/clientapi_test.go index 82ec9fea2b..f2d617cb94 100644 --- a/clientapi/clientapi_test.go +++ b/clientapi/clientapi_test.go @@ -1418,7 +1418,7 @@ func TestPushRules(t *testing.T) { validateFunc: func(t *testing.T, respBody *bytes.Buffer) { actions := gjson.GetBytes(respBody.Bytes(), "actions").Array() // only a basic check - assert.Equal(t, 1, len(actions)) + assert.Equal(t, 0, len(actions)) }, }, { diff --git a/internal/pushrules/action_test.go b/internal/pushrules/action_test.go index 72db9c998a..d1eb16ef18 100644 --- a/internal/pushrules/action_test.go +++ b/internal/pushrules/action_test.go @@ -13,8 +13,6 @@ func TestActionJSON(t *testing.T) { Want Action }{ {Action{Kind: NotifyAction}}, - {Action{Kind: DontNotifyAction}}, - {Action{Kind: CoalesceAction}}, {Action{Kind: SetTweakAction}}, {Action{Kind: SetTweakAction, Tweak: SoundTweak, Value: "default"}}, diff --git a/internal/pushrules/default_override.go b/internal/pushrules/default_override.go index f97427b719..376dd567dd 100644 --- a/internal/pushrules/default_override.go +++ b/internal/pushrules/default_override.go @@ -30,7 +30,7 @@ var ( RuleID: MRuleMaster, Default: true, Enabled: false, - Actions: []*Action{{Kind: DontNotifyAction}}, + Actions: []*Action{}, } mRuleSuppressNoticesDefinition = Rule{ RuleID: MRuleSuppressNotices, @@ -43,7 +43,7 @@ var ( Pattern: pointer("m.notice"), }, }, - Actions: []*Action{{Kind: DontNotifyAction}}, + Actions: []*Action{}, } mRuleMemberEventDefinition = Rule{ RuleID: MRuleMemberEvent, @@ -56,7 +56,7 @@ var ( Pattern: pointer("m.room.member"), }, }, - Actions: []*Action{{Kind: DontNotifyAction}}, + Actions: []*Action{}, } mRuleContainsDisplayNameDefinition = Rule{ RuleID: MRuleContainsDisplayName, @@ -152,9 +152,7 @@ var ( Pattern: pointer("m.reaction"), }, }, - Actions: []*Action{ - {Kind: DontNotifyAction}, - }, + Actions: []*Action{}, } ) diff --git a/internal/pushrules/default_pushrules_test.go b/internal/pushrules/default_pushrules_test.go index dea8298429..506fe3cc82 100644 --- a/internal/pushrules/default_pushrules_test.go +++ b/internal/pushrules/default_pushrules_test.go @@ -21,12 +21,12 @@ func TestDefaultRules(t *testing.T) { // Default override rules { name: ".m.rule.master", - inputBytes: []byte(`{"rule_id":".m.rule.master","default":true,"enabled":false,"actions":["dont_notify"]}`), + inputBytes: []byte(`{"rule_id":".m.rule.master","default":true,"enabled":false,"actions":[]}`), want: mRuleMasterDefinition, }, { name: ".m.rule.suppress_notices", - inputBytes: []byte(`{"rule_id":".m.rule.suppress_notices","default":true,"enabled":true,"conditions":[{"kind":"event_match","key":"content.msgtype","pattern":"m.notice"}],"actions":["dont_notify"]}`), + inputBytes: []byte(`{"rule_id":".m.rule.suppress_notices","default":true,"enabled":true,"conditions":[{"kind":"event_match","key":"content.msgtype","pattern":"m.notice"}],"actions":[]}`), want: mRuleSuppressNoticesDefinition, }, { @@ -36,7 +36,7 @@ func TestDefaultRules(t *testing.T) { }, { name: ".m.rule.member_event", - inputBytes: []byte(`{"rule_id":".m.rule.member_event","default":true,"enabled":true,"conditions":[{"kind":"event_match","key":"type","pattern":"m.room.member"}],"actions":["dont_notify"]}`), + inputBytes: []byte(`{"rule_id":".m.rule.member_event","default":true,"enabled":true,"conditions":[{"kind":"event_match","key":"type","pattern":"m.room.member"}],"actions":[]}`), want: mRuleMemberEventDefinition, }, { diff --git a/internal/pushrules/util.go b/internal/pushrules/util.go index de8fe5cd0f..e2821b57a5 100644 --- a/internal/pushrules/util.go +++ b/internal/pushrules/util.go @@ -16,10 +16,7 @@ func ActionsToTweaks(as []*Action) (ActionKind, map[string]interface{}, error) { for _, a := range as { switch a.Kind { - case DontNotifyAction: - // Don't bother processing any further - return DontNotifyAction, nil, nil - + case DontNotifyAction: // Ignored case SetTweakAction: if tweaks == nil { tweaks = map[string]interface{}{} diff --git a/internal/pushrules/util_test.go b/internal/pushrules/util_test.go index 89f8243d9b..83eee7ede6 100644 --- a/internal/pushrules/util_test.go +++ b/internal/pushrules/util_test.go @@ -17,17 +17,16 @@ func TestActionsToTweaks(t *testing.T) { {"empty", nil, UnknownAction, nil}, {"zero", []*Action{{}}, UnknownAction, nil}, {"onlyPrimary", []*Action{{Kind: NotifyAction}}, NotifyAction, nil}, - {"onlyPrimaryDontNotify", []*Action{{Kind: DontNotifyAction}}, DontNotifyAction, nil}, + {"onlyPrimaryDontNotify", []*Action{}, UnknownAction, nil}, {"onlyTweak", []*Action{{Kind: SetTweakAction, Tweak: HighlightTweak}}, UnknownAction, map[string]interface{}{"highlight": nil}}, {"onlyTweakWithValue", []*Action{{Kind: SetTweakAction, Tweak: SoundTweak, Value: "default"}}, UnknownAction, map[string]interface{}{"sound": "default"}}, { "all", []*Action{ - {Kind: CoalesceAction}, {Kind: SetTweakAction, Tweak: HighlightTweak}, {Kind: SetTweakAction, Tweak: SoundTweak, Value: "default"}, }, - CoalesceAction, + UnknownAction, map[string]interface{}{"highlight": nil, "sound": "default"}, }, } diff --git a/userapi/consumers/roomserver.go b/userapi/consumers/roomserver.go index 6da41f8a10..fca7412983 100644 --- a/userapi/consumers/roomserver.go +++ b/userapi/consumers/roomserver.go @@ -538,8 +538,8 @@ func (s *OutputRoomEventConsumer) notifyLocal(ctx context.Context, event *rstype if err != nil { return fmt.Errorf("pushrules.ActionsToTweaks: %w", err) } - // TODO: support coalescing. - if a != pushrules.NotifyAction && a != pushrules.CoalesceAction { + + if a != pushrules.NotifyAction { log.WithFields(log.Fields{ "event_id": event.EventID(), "room_id": event.RoomID().String(), diff --git a/userapi/consumers/roomserver_test.go b/userapi/consumers/roomserver_test.go index 49dd5b2381..8db2b2af4e 100644 --- a/userapi/consumers/roomserver_test.go +++ b/userapi/consumers/roomserver_test.go @@ -81,12 +81,8 @@ func Test_evaluatePushRules(t *testing.T) { { name: "m.reaction doesn't notify", eventContent: `{"type":"m.reaction","room_id":"!room:example.com"}`, - wantAction: pushrules.DontNotifyAction, - wantActions: []*pushrules.Action{ - { - Kind: pushrules.DontNotifyAction, - }, - }, + wantAction: "", + wantActions: []*pushrules.Action{}, }, { name: "m.room.message notifies", @@ -136,7 +132,7 @@ func Test_evaluatePushRules(t *testing.T) { 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 { + if tc.wantNotify && gotAction != pushrules.NotifyAction { t.Fatalf("expected to notify but didn't") } }) From 395bf9d35fc7c0cf6aae70bcbd439ca567881b8a Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Mon, 23 Oct 2023 16:40:55 +0200 Subject: [PATCH 2/5] Use const instead of plain string --- userapi/consumers/roomserver_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/userapi/consumers/roomserver_test.go b/userapi/consumers/roomserver_test.go index 8db2b2af4e..7b7c086183 100644 --- a/userapi/consumers/roomserver_test.go +++ b/userapi/consumers/roomserver_test.go @@ -81,7 +81,7 @@ func Test_evaluatePushRules(t *testing.T) { { name: "m.reaction doesn't notify", eventContent: `{"type":"m.reaction","room_id":"!room:example.com"}`, - wantAction: "", + wantAction: pushrules.UnknownAction, wantActions: []*pushrules.Action{}, }, { From 5ced1a11e7c0630617906bb59bf75ecb3d369987 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Tue, 24 Oct 2023 09:43:30 +0200 Subject: [PATCH 3/5] Add missing ACL rule to defaults --- internal/pushrules/default_override.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/pushrules/default_override.go b/internal/pushrules/default_override.go index 376dd567dd..cb825af5fd 100644 --- a/internal/pushrules/default_override.go +++ b/internal/pushrules/default_override.go @@ -10,6 +10,7 @@ func defaultOverrideRules(userID string) []*Rule { &mRuleRoomNotifDefinition, &mRuleTombstoneDefinition, &mRuleReactionDefinition, + &mRuleACLsDefinition, } } From 6abc40a896a4256b584c2701eb9251c7d75b71f2 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Tue, 24 Oct 2023 10:43:23 +0200 Subject: [PATCH 4/5] Empty actions are now allowed --- internal/pushrules/validate.go | 3 --- internal/pushrules/validate_test.go | 1 - 2 files changed, 4 deletions(-) diff --git a/internal/pushrules/validate.go b/internal/pushrules/validate.go index b54ec3fb0c..19ba6550b9 100644 --- a/internal/pushrules/validate.go +++ b/internal/pushrules/validate.go @@ -18,9 +18,6 @@ func ValidateRule(kind Kind, rule *Rule) []error { errs = append(errs, fmt.Errorf("invalid rule ID: %s", rule.RuleID)) } - if len(rule.Actions) == 0 { - errs = append(errs, fmt.Errorf("missing actions")) - } for _, action := range rule.Actions { errs = append(errs, validateAction(action)...) } diff --git a/internal/pushrules/validate_test.go b/internal/pushrules/validate_test.go index 966e462598..23fa66b88a 100644 --- a/internal/pushrules/validate_test.go +++ b/internal/pushrules/validate_test.go @@ -15,7 +15,6 @@ func TestValidateRuleNegatives(t *testing.T) { {Name: "emptyRuleID", Kind: OverrideKind, Rule: Rule{}, WantErrString: "invalid rule ID"}, {Name: "invalidKind", Kind: Kind("something else"), Rule: Rule{}, WantErrString: "invalid rule kind"}, {Name: "ruleIDBackslash", Kind: OverrideKind, Rule: Rule{RuleID: "#foo\\:example.com"}, WantErrString: "invalid rule ID"}, - {Name: "noActions", Kind: OverrideKind, Rule: Rule{}, WantErrString: "missing actions"}, {Name: "invalidAction", Kind: OverrideKind, Rule: Rule{Actions: []*Action{{}}}, WantErrString: "invalid rule action kind"}, {Name: "invalidCondition", Kind: OverrideKind, Rule: Rule{Conditions: []*Condition{{}}}, WantErrString: "invalid rule condition kind"}, {Name: "overrideNoCondition", Kind: OverrideKind, Rule: Rule{}, WantErrString: "missing rule conditions"}, From 13da5c38b1e4105dae2654e50767fc94b0158006 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Tue, 24 Oct 2023 11:17:21 +0200 Subject: [PATCH 5/5] Make Sytest happy again --- internal/pushrules/validate.go | 3 +++ internal/pushrules/validate_test.go | 1 + 2 files changed, 4 insertions(+) diff --git a/internal/pushrules/validate.go b/internal/pushrules/validate.go index 19ba6550b9..4cc4793454 100644 --- a/internal/pushrules/validate.go +++ b/internal/pushrules/validate.go @@ -18,6 +18,9 @@ func ValidateRule(kind Kind, rule *Rule) []error { errs = append(errs, fmt.Errorf("invalid rule ID: %s", rule.RuleID)) } + if rule.Actions == nil { + errs = append(errs, fmt.Errorf("missing actions")) + } for _, action := range rule.Actions { errs = append(errs, validateAction(action)...) } diff --git a/internal/pushrules/validate_test.go b/internal/pushrules/validate_test.go index 23fa66b88a..966e462598 100644 --- a/internal/pushrules/validate_test.go +++ b/internal/pushrules/validate_test.go @@ -15,6 +15,7 @@ func TestValidateRuleNegatives(t *testing.T) { {Name: "emptyRuleID", Kind: OverrideKind, Rule: Rule{}, WantErrString: "invalid rule ID"}, {Name: "invalidKind", Kind: Kind("something else"), Rule: Rule{}, WantErrString: "invalid rule kind"}, {Name: "ruleIDBackslash", Kind: OverrideKind, Rule: Rule{RuleID: "#foo\\:example.com"}, WantErrString: "invalid rule ID"}, + {Name: "noActions", Kind: OverrideKind, Rule: Rule{}, WantErrString: "missing actions"}, {Name: "invalidAction", Kind: OverrideKind, Rule: Rule{Actions: []*Action{{}}}, WantErrString: "invalid rule action kind"}, {Name: "invalidCondition", Kind: OverrideKind, Rule: Rule{Conditions: []*Condition{{}}}, WantErrString: "invalid rule condition kind"}, {Name: "overrideNoCondition", Kind: OverrideKind, Rule: Rule{}, WantErrString: "missing rule conditions"},