Fix bugs in checkUserLevels (#306)

* Be more useful when shouting about power level issues * Don't use default power levels when considering user power levels, don't create duplicate entries * Add tests * Fix name * Don't set state default level
matrix-org · May 6, 2022 · 8958f9d · 8958f9d
1 parent f3951a6
commit 8958f9d
Show file tree

Hide file tree

Showing 2 changed files with 103 additions and 30 deletions.
diff --git a/eventauth.go b/eventauth.go
@@ -620,38 +620,21 @@ func checkEventLevels(senderLevel int64, oldPowerLevels, newPowerLevels PowerLev
 // checkUserLevels checks that the changes in user levels are allowed.
 func checkUserLevels(senderLevel int64, senderID string, oldPowerLevels, newPowerLevels PowerLevelContent) error {
 	type levelPair struct {
-		old    int64
-		new    int64
-		userID string
+		old int64
+		new int64
 	}
 
 	// Build a list of user levels to check.
-	// This differs slightly in behaviour from the code in synapse because it will use the
-	// default value if a level is not present in one of the old or new events.
-
-	// First add the user default level.
-	userLevelChecks := []levelPair{
-		{oldPowerLevels.UsersDefault, newPowerLevels.UsersDefault, ""},
-	}
-
-	// Then add checks for each user key in the new levels.
+	userLevelChecks := map[string]levelPair{}
 	for userID := range newPowerLevels.Users {
-		userLevelChecks = append(userLevelChecks, levelPair{
-			oldPowerLevels.UserLevel(userID), newPowerLevels.UserLevel(userID), userID,
-		})
-	}
-
-	// Then add checks for each user key in the old levels.
-	// Some of these will be duplicates of the ones added using the keys from
-	// the new levels. But it doesn't hurt to run the checks twice for the same level.
-	for userID := range oldPowerLevels.Users {
-		userLevelChecks = append(userLevelChecks, levelPair{
-			oldPowerLevels.UserLevel(userID), newPowerLevels.UserLevel(userID), userID,
-		})
+		userLevelChecks[userID] = levelPair{
+			old: oldPowerLevels.Users[userID],
+			new: newPowerLevels.Users[userID],
+		}
 	}
 
 	// Check each of the levels in the list.
-	for _, level := range userLevelChecks {
+	for userID, level := range userLevelChecks {
 		// Check if the level is being changed.
 		if level.old == level.new {
 			// Levels are always allowed to stay the same.
@@ -669,14 +652,14 @@ func checkUserLevels(senderLevel int64, senderID string, oldPowerLevels, newPowe
 		// Check if the user is trying to set any of the levels to above their own.
 		if senderLevel < level.new {
 			return errorf(
-				"sender with level %d is not allowed change user level from %d to %d"+
+				"sender %q with level %d is not allowed change user %q level from %d to %d"+
 					" because the new level is above the level of the sender",
-				senderLevel, level.old, level.new,
+				senderID, senderLevel, userID, level.old, level.new,
 			)
 		}
 
 		// Check if the user is changing their own user level.
-		if level.userID == senderID {
+		if userID == senderID {
 			// Users are always allowed to reduce their own user level.
 			// We know that the user is reducing their level because of the previous checks.
 			continue
@@ -685,9 +668,9 @@ func checkUserLevels(senderLevel int64, senderID string, oldPowerLevels, newPowe
 		// Check if the user is changing the level that was above or the same as their own.
 		if senderLevel <= level.old {
 			return errorf(
-				"sender with level %d is not allowed to change user level from %d to %d"+
+				"sender %q with level %d is not allowed to change user %q level from %d to %d"+
 					" because the old level is equal to or above the level of the sender",
-				senderLevel, level.old, level.new,
+				senderID, senderLevel, userID, level.old, level.new,
 			)
 		}
 	}
@@ -1261,6 +1244,7 @@ func (m *membershipAllower) membershipAllowedOther() error { // nolint: gocyclo
 			}
 			return nil
 		}
+
 		return m.membershipFailed(
 			"sender has insufficient power to invite (sender level %d, target level %d, invite level %d)",
 			senderLevel, targetLevel, m.powerLevels.Invite,

diff --git a/eventauth_test.go b/eventauth_test.go
@@ -1040,6 +1040,95 @@ func TestAuthEvents(t *testing.T) {
 	}
 }
 
+var powerLevelTestRoom = &testAuthEvents{
+	CreateJSON: json.RawMessage(`{
+		"type": "m.room.create",
+		"state_key": "",
+		"sender": "@u1:a",
+		"room_id": "!r1:a",
+		"event_id": "$e1:a",
+		"content": {
+			"room_version": "1"
+		}
+	}`),
+	PowerLevelsJSON: json.RawMessage(`{
+		"type": "m.room.power_levels",
+		"state_key": "",
+		"sender": "@u1:a",
+		"room_id": "!r1:a",
+		"event_id": "$e3:a",
+		"content": {
+			"users_default": 100,
+			"users": {
+				"@u1:a": 100
+			},
+			"redact": 100
+		}
+	}`),
+	MemberJSON: map[string]json.RawMessage{
+		"@u1:a": json.RawMessage(`{
+			"type": "m.room.member",
+			"state_key": "@u1:a",
+			"sender": "@u1:a",
+			"room_id": "!r1:a",
+			"event_id": "$e2:a",
+			"content": {
+				"membership": "join"
+			}
+		}`),
+	},
+}
+
+func TestDemoteUserDefaultPowerLevelBelowOwn(t *testing.T) {
+	// User should be able to demote the user default level
+	// below their own effective level.
+	powerChangeShouldSucceed, err := NewEventFromTrustedJSON(RawJSON(`{
+		"type": "m.room.power_levels",
+		"state_key": "",
+		"sender": "@u1:a",
+		"room_id": "!r1:a",
+		"event_id": "$e5:a",
+		"content": {
+			"users_default": 50,
+			"users": {
+				"@u1:a": 100
+			},
+			"redact": 100
+		}
+	}`), false, RoomVersionV1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err = Allowed(powerChangeShouldSucceed, powerLevelTestRoom); err != nil {
+		t.Error("TestDemoteUserDefaultPowerLevel should have succeeded but it didn't:", err)
+	}
+}
+
+func TestPromoteUserDefaultLevelAboveOwn(t *testing.T) {
+	// User shouldn't be able to promote the user default
+	// level above their own effective level.
+	powerChangeShouldFail, err := NewEventFromTrustedJSON(RawJSON(`{
+		"type": "m.room.power_levels",
+		"state_key": "",
+		"sender": "@u2:a",
+		"room_id": "!r1:a",
+		"event_id": "$e5:a",
+		"content": {
+			"users_default": 500,
+			"users": {
+				"@u1:a": 100
+			},
+			"redact": 100
+		}
+	}`), false, RoomVersionV1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err = Allowed(powerChangeShouldFail, powerLevelTestRoom); err == nil {
+		t.Error("TestPromoteUserDefaultLevelAboveOwn event should have failed but it didn't")
+	}
+}
+
 func newMemberContent(
 	membership string, thirdPartyInvite *MemberThirdPartyInvite,
 ) MemberContent {