From 47deeefe57766aa455151eef1678e46a335391d2 Mon Sep 17 00:00:00 2001 From: PJ Date: Tue, 26 Sep 2023 10:46:36 +0100 Subject: [PATCH] Optionally limit new users based on admin setting (#20149) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * create admin setting for new user status * limiting on signup working 🎉 * update registration specs * sort out OAuth registrations --- app/controllers/registrations_controller.rb | 22 ++--- app/helpers/admin/settings_helper.rb | 6 ++ app/lib/constants/settings/authentication.rb | 4 + app/models/settings/authentication.rb | 9 ++ app/models/user.rb | 15 +++ app/services/authentication/authenticator.rb | 1 + .../settings/forms/_authentication.html.erb | 9 ++ .../settings/forms/_user_experience.html.erb | 2 +- config/locales/lib/en.yml | 3 + config/locales/lib/fr.yml | 3 + spec/models/user_spec.rb | 63 +++++++++++++ spec/requests/registrations_spec.rb | 94 ++++++++++++++++++- .../authentication/authenticator_spec.rb | 52 ++++++++++ 13 files changed, 265 insertions(+), 18 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 4c285e63f..48da47153 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -22,17 +22,17 @@ class RegistrationsController < Devise::RegistrationsController build_devise_resource if resource.persisted? - update_first_user_permissions(resource) + resource.set_initial_roles! - if ForemInstance.smtp_enabled? + if resource.creator? + prepare_new_forem_instance + sign_in(resource) + redirect_to new_admin_creator_setting_path + elsif ForemInstance.smtp_enabled? redirect_to confirm_email_path(email: resource.email) else sign_in(resource) - if resource.roles.includes(:creator).any? - redirect_to new_admin_creator_setting_path - else - redirect_to root_path - end + redirect_to root_path end else render action: "by_email" @@ -41,13 +41,7 @@ class RegistrationsController < Devise::RegistrationsController private - def update_first_user_permissions(resource) - return unless Settings::General.waiting_on_first_user - - resource.add_role(:creator) - resource.add_role(:super_admin) - resource.add_role(:trusted) - resource.skip_confirmation! + def prepare_new_forem_instance Settings::General.waiting_on_first_user = false Users::CreateMascotAccount.call Discover::RegisterWorker.perform_async # Register Forem instance on https://discover.forem.com diff --git a/app/helpers/admin/settings_helper.rb b/app/helpers/admin/settings_helper.rb index 1149f6bdf..3ebc4090f 100644 --- a/app/helpers/admin/settings_helper.rb +++ b/app/helpers/admin/settings_helper.rb @@ -7,5 +7,11 @@ module Admin def billboard_all_countries_for_editing ISO3166::Country.all.to_h { |country| [country.alpha2, country.common_name] } end + + def new_user_status_options + ::Settings::Authentication::NEW_USER_STATUSES.map do |status| + [status.humanize, status] + end + end end end diff --git a/app/lib/constants/settings/authentication.rb b/app/lib/constants/settings/authentication.rb index fec2411fd..f1b0608f5 100644 --- a/app/lib/constants/settings/authentication.rb +++ b/app/lib/constants/settings/authentication.rb @@ -66,6 +66,10 @@ module Constants description: I18n.t("lib.constants.settings.authentication.invite_only.description"), placeholder: "" }, + new_user_status: { + description: I18n.t("lib.constants.settings.authentication.new_user_status.description"), + placeholder: I18n.t("lib.constants.settings.authentication.new_user_status.placeholder") + }, recaptcha_site_key: { description: I18n.t("lib.constants.settings.authentication.recaptcha_site.description"), placeholder: I18n.t("lib.constants.settings.authentication.recaptcha_site.placeholder") diff --git a/app/models/settings/authentication.rb b/app/models/settings/authentication.rb index 46d5eff17..25dccb7e2 100644 --- a/app/models/settings/authentication.rb +++ b/app/models/settings/authentication.rb @@ -2,6 +2,8 @@ module Settings class Authentication < Base self.table_name = :settings_authentications + NEW_USER_STATUSES = %w[good_standing limited].freeze + setting :allow_email_password_login, type: :boolean, default: true setting :allow_email_password_registration, type: :boolean, default: false setting :allowed_registration_email_domains, type: :array, default: %w[], validates: { @@ -24,6 +26,9 @@ module Settings setting :google_oauth2_key, type: :string setting :google_oauth2_secret, type: :string setting :invite_only_mode, type: :boolean, default: false + setting :new_user_status, type: :string, default: "good_standing", validates: { + inclusion: { in: NEW_USER_STATUSES } + } setting :providers, type: :array, default: %w[] setting :require_captcha_for_email_password_registration, type: :boolean, default: false setting :twitter_key, type: :string, default: ApplicationConfig["TWITTER_KEY"] @@ -57,5 +62,9 @@ module Settings false end + + def self.limit_new_users? + new_user_status == "limited" + end end end diff --git a/app/models/user.rb b/app/models/user.rb index e983708aa..773ab837e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -277,6 +277,21 @@ class User < ApplicationRecord self.remember_created_at ||= Time.now.utc end + def set_initial_roles! + # Avoid overwriting roles for users who already exist but are e.g. logging in + # through a new identity provider + return unless valid? && previously_new_record? + + if Settings::General.waiting_on_first_user + add_role(:creator) + add_role(:super_admin) + add_role(:trusted) + elsif Settings::Authentication.limit_new_users? + add_role(:limited) + # Otherwise just leave the new user in good standing + end + end + def calculate_score # User score is used to mitigate spam by reducing visibility of flagged users # It can generally be used as a baseline for affecting certain functionality which diff --git a/app/services/authentication/authenticator.rb b/app/services/authentication/authenticator.rb index f35fa171f..6ebd60206 100644 --- a/app/services/authentication/authenticator.rb +++ b/app/services/authentication/authenticator.rb @@ -51,6 +51,7 @@ module Authentication else update_user(user) end + user.set_initial_roles! identity.user = user if identity.user_id.blank? new_identity = identity.new_record? diff --git a/app/views/admin/settings/forms/_authentication.html.erb b/app/views/admin/settings/forms/_authentication.html.erb index 71fa84bad..e3ec4ae30 100644 --- a/app/views/admin/settings/forms/_authentication.html.erb +++ b/app/views/admin/settings/forms/_authentication.html.erb @@ -207,6 +207,15 @@
<% end %> + +
+ <%= admin_config_label :new_user_status, model: Settings::Authentication %> + <%= admin_config_description Constants::Settings::Authentication.details[:new_user_status][:description] %> + <%= select_tag "settings_authentication[new_user_status]", + options_for_select(new_user_status_options, Settings::Authentication.new_user_status), + multiple: false, + class: "crayons-select" %> +
<%= render "update_setting_button", f: f %> diff --git a/app/views/admin/settings/forms/_user_experience.html.erb b/app/views/admin/settings/forms/_user_experience.html.erb index 5194d28e9..a12c9c5e6 100644 --- a/app/views/admin/settings/forms/_user_experience.html.erb +++ b/app/views/admin/settings/forms/_user_experience.html.erb @@ -5,7 +5,7 @@ User Experience and Brand
-
+
<%= admin_config_label :feed_style, model: Settings::UserExperience %> <%= admin_config_description Constants::Settings::UserExperience.details[:feed_style][:description] %> <%= select_tag "settings_user_experience[feed_style]", diff --git a/config/locales/lib/en.yml b/config/locales/lib/en.yml index 1a607f6c0..08dca1668 100644 --- a/config/locales/lib/en.yml +++ b/config/locales/lib/en.yml @@ -37,6 +37,9 @@ en: description: The "Client Secret" portion of the OAuth 2.0 page on the Google Cloud Platform portal invite_only: description: Only users invited by email can join this community. + new_user_status: + description: What status do you want to apply to new users? + placeholder: Good standing providers: description: How can users sign in? recaptcha_secret: diff --git a/config/locales/lib/fr.yml b/config/locales/lib/fr.yml index 7f58957ab..b43fc64ed 100644 --- a/config/locales/lib/fr.yml +++ b/config/locales/lib/fr.yml @@ -37,6 +37,9 @@ fr: description: The "Client Secret" portion of the OAuth 2.0 page on the Google Cloud Platform portal invite_only: description: Only users invited by email can join this community. + new_user_status: + description: What status do you want to apply to new users? + placeholder: Good standing providers: description: How can users sign in? recaptcha_secret: diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b04bfe6ea..fb89f40b8 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -663,6 +663,69 @@ RSpec.describe User do end end + describe "#set_initial_roles!" do + it "adds creator roles if waiting on first user" do + allow(Settings::General).to receive(:waiting_on_first_user).and_return(true) + + user = create(:user) + user.set_initial_roles! + + expect(user).to be_creator + expect(user).to be_super_admin + expect(user).to be_trusted + expect(user).not_to be_limited + end + + it "does not add any roles if not waiting on first user" do + user = create(:user) + user.set_initial_roles! + + expect(user).not_to be_creator + expect(user).not_to be_super_admin + expect(user).not_to be_trusted + expect(user).not_to be_limited + end + + it "adds the limited role to a new user if the new user status setting is limited" do + allow(Settings::Authentication).to receive(:new_user_status).and_return("limited") + + user = create(:user) + user.set_initial_roles! + + expect(user).not_to be_creator + expect(user).not_to be_super_admin + expect(user).not_to be_trusted + expect(user).to be_limited + end + + it "does not change any roles if the user is not a new user" do + create(:user, :trusted, email: "trusted-user@forem.test") + + # Now considered an already existing record to ActiveRecord + user = described_class.find_by(email: "trusted-user@forem.test") + expect(UserRole.count).to eq(1) + + expect { user.set_initial_roles! }.not_to change(UserRole, :count) + expect(user).to be_trusted + end + + it "does not change any roles if the user is not valid" do + user = create(:user, :tag_moderator) + expect(UserRole.count).to eq(1) + + user.username = "" + expect { user.set_initial_roles! }.not_to change(UserRole, :count) + expect(user).to be_tag_moderator + end + + it "does nothing if the user has not been persisted" do + user = build(:user) + expect(UserRole.count).to eq(0) + + expect { user.set_initial_roles! }.not_to change(UserRole, :count) + end + end + describe "#calculate_score" do it "calculates a score" do user.update_column(:badge_achievements_count, 3) diff --git a/spec/requests/registrations_spec.rb b/spec/requests/registrations_spec.rb index 9935ab8a9..7662a38a3 100644 --- a/spec/requests/registrations_spec.rb +++ b/spec/requests/registrations_spec.rb @@ -232,15 +232,46 @@ RSpec.describe "Registrations" do expect(User.all.size).to be 1 end - it "marks as registered" do + it "registers a user in good standing" do post "/users", params: { user: { name: "test #{rand(10)}", username: "haha_#{rand(10)}", email: "yoooo#{rand(100)}@yo.co", password: "PaSSw0rd_yo000", password_confirmation: "PaSSw0rd_yo000" } } - expect(User.last.registered).to be true - expect(User.last.registered_at).not_to be_nil + + new_user = User.last + expect(new_user.registered).to be true + expect(new_user.registered_at).not_to be_nil + expect(new_user).not_to be_limited + end + + it "limits the user if the admins have set new user status to limited" do + allow(Settings::Authentication).to receive(:new_user_status).and_return("limited") + + user = build(:user) + user_attributes = user.slice(:name, :username, :email) + + post "/users", params: + { user: { **user_attributes, password: "Passw0rd!", password_confirmation: "Passw0rd!" } } + + new_user = User.last + expect(new_user.registered).to be true + expect(new_user.registered_at).not_to be_nil + expect(new_user).to be_limited + end + + it "logs in user and redirects to the root path" do + user = build(:user) + user_attributes = user.slice(:name, :username, :email) + + post "/users", params: + { user: { **user_attributes, password: "Passw0rd!", password_confirmation: "Passw0rd!" } } + + new_user = User.last + expect(new_user).to have_attributes(user_attributes) + expect(controller.current_user).to eq(new_user) + expect(response).to redirect_to(root_path) end it "does not create user with password confirmation mismatch" do @@ -264,6 +295,44 @@ RSpec.describe "Registrations" do end end + context "when email registration is allowed and confirmation is required" do + before do + allow(Settings::Authentication).to receive(:allow_email_password_registration).and_return(true) + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) + allow_any_instance_of(ProfileImageUploader).to receive(:download!) + end + + it "registers the user but does not log them in" do + user = build(:user) + user_attributes = user.slice(:name, :username, :email) + + post "/users", params: + { user: { **user_attributes, password: "Passw0rd!", password_confirmation: "Passw0rd!" } } + + new_user = User.last + expect(new_user.registered).to be true + expect(new_user.registered_at).not_to be_nil + expect(new_user).not_to be_limited + expect(controller.current_user).to be_nil + expect(response).to redirect_to(confirm_email_path(email: user.email)) + end + + it "also limits the user first if the admins have set new user status to limited" do + allow(Settings::Authentication).to receive(:new_user_status).and_return("limited") + + user = build(:user) + user_attributes = user.slice(:name, :username, :email) + + post "/users", params: + { user: { **user_attributes, password: "Passw0rd!", password_confirmation: "Passw0rd!" } } + + new_user = User.last + expect(new_user).to be_limited + expect(controller.current_user).to be_nil + expect(response).to redirect_to(confirm_email_path(email: user.email)) + end + end + context "when email registration allowed and email allow list empty" do before do allow_any_instance_of(ProfileImageUploader).to receive(:download!) @@ -379,6 +448,20 @@ RSpec.describe "Registrations" do expect(User.first.email).to eq user_email end + it "logs in user and redirects them to the creator settings path" do + user = build(:user) + user_attributes = user.slice(:name, :username, :email) + + post "/users", params: + { user: { **user_attributes, password: "Passw0rd!", password_confirmation: "Passw0rd!" } } + + new_user = User.first + expect(new_user.registered).to be true + expect(new_user.registered_at).not_to be_nil + expect(controller.current_user).to eq(new_user) + expect(response).to redirect_to(new_admin_creator_setting_path) + end + it "makes user super admin and config admin" do post "/users", params: { user: { name: "test #{rand(10)}", @@ -388,6 +471,8 @@ RSpec.describe "Registrations" do password_confirmation: "PaSSw0rd_yo000" } } expect(User.first.super_admin?).to be true expect(User.first.trusted?).to be true + expect(User.first.creator?).to be true + expect(User.first.limited?).to be false end it "creates mascot user" do @@ -471,6 +556,9 @@ RSpec.describe "Registrations" do password: "PaSSw0rd_yo000", password_confirmation: "PaSSw0rd_yo000" } } expect(User.first.super_admin?).to be true + expect(User.first.trusted?).to be true + expect(User.first.creator?).to be true + expect(User.first.limited?).to be false end end end diff --git a/spec/services/authentication/authenticator_spec.rb b/spec/services/authentication/authenticator_spec.rb index 863e8d26f..ef0b8e39c 100644 --- a/spec/services/authentication/authenticator_spec.rb +++ b/spec/services/authentication/authenticator_spec.rb @@ -19,6 +19,48 @@ RSpec.describe Authentication::Authenticator, type: :service do end end + shared_examples "preserves existing user roles" do + it "does not make any changes to roles if the user already exists" do + existing_user = create(:user, :trusted) + auth_payload.info.email = existing_user.email + + expect do + user = service.call + expect(user).to eq(existing_user) + expect(user).to be_trusted + expect(user).not_to be_limited + end.not_to change(existing_user.roles, :count) + end + end + + shared_examples "appropriate new user status" do + it "registers a user in good standing by default" do + user = service.call + + expect(user.registered).to be(true) + expect(user.registered_at).not_to be_nil + expect(user).not_to be_limited + end + + include_examples "preserves existing user roles" + + context "when new user status admin setting is limited" do + before do + allow(Settings::Authentication).to receive(:new_user_status).and_return("limited") + end + + it "registers the user as limited" do + user = service.call + + expect(user.registered).to be(true) + expect(user.registered_at).not_to be_nil + expect(user).to be_limited + end + + include_examples "preserves existing user roles" + end + end + context "when authenticating through an unknown provider" do it "raises ProviderNotFound" do auth_payload = OmniAuth.config.mock_auth[:github].merge(provider: "okta") @@ -34,6 +76,8 @@ RSpec.describe Authentication::Authenticator, type: :service do include_context "spam handling" + include_context "appropriate new user status" + describe "new user" do it "creates a new user" do expect do @@ -229,6 +273,8 @@ RSpec.describe Authentication::Authenticator, type: :service do include_context "spam handling" + include_context "appropriate new user status" + describe "new user" do it "creates a new user" do expect do @@ -422,6 +468,8 @@ RSpec.describe Authentication::Authenticator, type: :service do include_context "spam handling" + include_context "appropriate new user status" + describe "new user" do it "creates a new user" do expect do @@ -507,6 +555,8 @@ RSpec.describe Authentication::Authenticator, type: :service do include_context "spam handling" + include_context "appropriate new user status" + describe "new user" do it "creates a new user" do expect do @@ -685,6 +735,8 @@ RSpec.describe Authentication::Authenticator, type: :service do include_context "spam handling" + include_context "appropriate new user status" + describe "new user" do it "creates a new user" do expect do