From 7c96d7587e0d61557f6f3754792dd6565477b772 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 13 Jan 2025 17:30:36 +1000 Subject: [PATCH] FIX: Allow admins to use reserved usernames (#30262) It is possible for admins to rename users like `system` to some other username, but if they try to change it back they cannot, since `system` is a reserved username. This commit allows admins to change any user's username to a reserved username _as long as that username is not already in use_. --- app/controllers/users_controller.rb | 4 +- spec/requests/users_controller_spec.rb | 59 +++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a6825127f5b..2c970da1dc4 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -269,7 +269,7 @@ class UsersController < ApplicationController params.require(:new_username) if clashing_with_existing_route?(params[:new_username]) || - User.reserved_username?(params[:new_username]) + (User.reserved_username?(params[:new_username]) && !current_user.admin?) return render_json_error(I18n.t("login.reserved_username")) end @@ -614,7 +614,7 @@ class UsersController < ApplicationController # The special case where someone is changing the case of their own username return render_available_true if changing_case_of_own_username(target_user, username) - checker = UsernameCheckerService.new + checker = UsernameCheckerService.new(allow_reserved_username: current_user&.admin?) email = params[:email] || target_user.try(:email) render json: checker.check_username(username, email) end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 80e5afa3983..7425eaee0a7 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1846,7 +1846,7 @@ RSpec.describe UsersController do it "raises an error without a new_username param" do put "/u/#{user.username}/preferences/username.json", params: { username: user.username } - expect(response.status).to eq(400) + expect(response).to be_bad_request expect(user.reload.username).to eq(old_username) end @@ -1862,7 +1862,7 @@ RSpec.describe UsersController do it "raises an error when change_username fails" do put "/u/#{user.username}/preferences/username.json", params: { new_username: "@" } - expect(response.status).to eq(422) + expect(response).to be_unprocessable body = response.parsed_body @@ -1876,7 +1876,7 @@ RSpec.describe UsersController do it "should succeed in normal circumstances" do put "/u/#{user.username}/preferences/username.json", params: { new_username: new_username } - expect(response.status).to eq(200) + expect(response).to be_successful expect(user.reload.username).to eq(new_username) end @@ -1895,9 +1895,30 @@ RSpec.describe UsersController do SiteSetting.reserved_usernames = "reserved" put "/u/#{user.username}/preferences/username.json", params: { new_username: "reserved" } - body = response.parsed_body + expect(response).to be_unprocessable - expect(body["errors"].first).to include(I18n.t("login.reserved_username")) + expect(response.parsed_body["errors"].first).to include(I18n.t("login.reserved_username")) + end + + it "allows admins to change a username to one in the reserved list" do + sign_in(admin) + SiteSetting.reserved_usernames = "reserved" + + put "/u/#{user.username}/preferences/username.json", params: { new_username: "reserved" } + expect(response).to be_successful + + expect(response.parsed_body["username"]).to eq("reserved") + end + + it "does not allow admins to change a username to one in the reserved list if that user already exists" do + sign_in(admin) + SiteSetting.reserved_usernames = "reserved" + Fabricate(:user, username: "reserved") + + put "/u/#{user.username}/preferences/username.json", params: { new_username: "reserved" } + expect(response).to be_unprocessable + + expect(response.parsed_body["errors"].first).to include("Username must be unique") end it "should fail if the user is old" do @@ -1921,7 +1942,7 @@ RSpec.describe UsersController do put "/u/#{user.username}/preferences/username.json", params: { new_username: new_username } - expect(response.status).to eq(200) + expect(response).to be_successful expect( UserHistory.where( action: UserHistory.actions[:change_username], @@ -1947,7 +1968,7 @@ RSpec.describe UsersController do put "/u/#{user.username}/preferences/username.json", params: { new_username: new_username } - expect(response.status).to eq(422) + expect(response).to be_unprocessable expect(response.parsed_body["errors"].first).to include( I18n.t("errors.messages.auth_overrides_username"), ) @@ -1991,6 +2012,30 @@ RSpec.describe UsersController do include_examples "when username is unavailable" end + describe "reserved usernames" do + before { SiteSetting.reserved_usernames = SiteSetting.reserved_usernames + "|reserved" } + + context "when checking a reserved username" do + before { get "/u/check_username.json", params: { username: "reserved" } } + include_examples "when username is unavailable" + end + + context "when checking a reserved username as an admin" do + before { sign_in(admin) } + + context "when user already exists" do + fab!(:user) { Fabricate(:user, username: "reserved") } + before { get "/u/check_username.json", params: { username: "reserved" } } + include_examples "when username is unavailable" + end + + context "when user does not exist" do + before { get "/u/check_username.json", params: { username: "reserved" } } + include_examples "when username is available" + end + end + end + shared_examples "checking an invalid username" do it "should not return an available key but should return an error message" do expect(response.status).to eq(200)