From 738789f33661795638cbf4cf4959654f599bccbb Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Wed, 3 Apr 2013 12:23:28 -0400 Subject: [PATCH] Admins can't lock themselves out of a site by setting approval. --- .../users/omniauth_callbacks_controller.rb | 42 +++++++++++++----- app/controllers/users_controller.rb | 14 +++--- lib/guardian.rb | 11 +++++ spec/components/guardian_spec.rb | 44 +++++++++++++++++++ spec/controllers/users_controller_spec.rb | 5 ++- 5 files changed, 95 insertions(+), 21 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 7b320d0b45c..a1f8769779a 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -58,8 +58,12 @@ class Users::OmniauthCallbacksController < ApplicationController if user_info if user_info.user.active - log_on_user(user_info.user) - @data[:authenticated] = true + if Guardian.new(user_info.user).can_access_forum? + log_on_user(user_info.user) + @data[:authenticated] = true + else + @data[:awaiting_approval] = true + end else @data[:awaiting_activation] = true # send another email ? @@ -114,8 +118,14 @@ class Users::OmniauthCallbacksController < ApplicationController user.active = true user.save end - log_on_user(user) - @data[:authenticated] = true + + # If we have to approve users + if Guardian.new(user).can_access_forum? + log_on_user(user) + @data[:authenticated] = true + else + @data[:awaiting_approval] = true + end end else user = User.where(email: email).first @@ -156,11 +166,11 @@ class Users::OmniauthCallbacksController < ApplicationController user = user_open_id.user # If we have to approve users - if SiteSetting.must_approve_users? && !user.approved? - @data = {awaiting_approval: true} - else + if Guardian.new(user).can_access_forum? log_on_user(user) @data = {authenticated: true} + else + @data = {awaiting_approval: true} end else @@ -203,8 +213,14 @@ class Users::OmniauthCallbacksController < ApplicationController if user_info if user_info.user.active - log_on_user(user_info.user) - @data[:authenticated] = true + + if Guardian.new(user_info.user).can_access_forum? + log_on_user(user_info.user) + @data[:authenticated] = true + else + @data[:awaiting_approval] = true + end + else @data[:awaiting_activation] = true # send another email ? @@ -222,12 +238,14 @@ class Users::OmniauthCallbacksController < ApplicationController user = User.find_by_email(email) if user - if SiteSetting.must_approve_users? && !user.approved? - @data = {awaiting_approval: true} - else + + if Guardian.new(user).can_access_forum? log_on_user(user) @data = {authenticated: true} + else + @data = {awaiting_approval: true} end + else @data = { email: email, diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 41924d54674..1f1f0e9b396 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -246,13 +246,13 @@ class UsersController < ApplicationController @user.password = params[:password] if @user.save - if SiteSetting.must_approve_users? && !@user.approved? - @requires_approval = true - flash[:success] = I18n.t('password_reset.success_unapproved') - else + if Guardian.new(@user).can_access_forum? # Log in the user log_on_user(@user) flash[:success] = I18n.t('password_reset.success') + else + @requires_approval = true + flash[:success] = I18n.t('password_reset.success_unapproved') end end end @@ -293,11 +293,11 @@ class UsersController < ApplicationController if @user = EmailToken.confirm(params[:token]) # Log in the user unless they need to be approved - if SiteSetting.must_approve_users? - @needs_approval = true - else + if Guardian.new(@user).can_access_forum? @user.enqueue_welcome_message('welcome_user') if @user.send_welcome_message log_on_user(@user) + else + @needs_approval = true end else diff --git a/lib/guardian.rb b/lib/guardian.rb index b445324167d..0158c670daa 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -164,6 +164,17 @@ class Guardian true end + # Support sites that have to approve users + def can_access_forum? + return true unless SiteSetting.must_approve_users? + return false if user.blank? + + # Admins can't lock themselves out of a site + return true if user.admin? + + user.approved? + end + def can_see_pending_invites_from?(user) return false if user.blank? return false if @user.blank? diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 61bc3adf22c..0da7d11e599 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -815,6 +815,50 @@ describe Guardian do it 'is true when looking at your own invites' do Guardian.new(user).can_see_pending_invites_from?(user).should be_true end + end + + context "can_access_forum?" do + + let(:unapproved_user) { Fabricate.build(:user) } + + context "when must_approve_users is false" do + before do + SiteSetting.stubs(:must_approve_users?).returns(false) + end + + it "returns true for a nil user" do + Guardian.new(nil).can_access_forum?.should be_true + end + + it "returns true for an unapproved user" do + Guardian.new(unapproved_user).can_access_forum?.should be_true + end + end + + context 'when must_approve_users is true' do + before do + SiteSetting.stubs(:must_approve_users?).returns(true) + end + + it "returns false for a nil user" do + Guardian.new(nil).can_access_forum?.should be_false + end + + it "returns false for an unapproved user" do + Guardian.new(unapproved_user).can_access_forum?.should be_false + end + + it "returns true for an admin user" do + unapproved_user.admin = true + Guardian.new(unapproved_user).can_access_forum?.should be_true + end + + it "returns true for an approved user" do + unapproved_user.approved = true + Guardian.new(unapproved_user).can_access_forum?.should be_true + end + + end end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 0d5d8b6480c..f3676dc0a88 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -117,6 +117,7 @@ describe UsersController do context 'reponse' do before do + Guardian.any_instance.expects(:can_access_forum?).returns(true) EmailToken.expects(:confirm).with('asdfasdf').returns(user) get :activate_account, token: 'asdfasdf' end @@ -139,9 +140,9 @@ describe UsersController do end - context 'must_approve_users' do + context 'user is not approved' do before do - SiteSetting.expects(:must_approve_users?).returns(true) + Guardian.any_instance.expects(:can_access_forum?).returns(false) EmailToken.expects(:confirm).with('asdfasdf').returns(user) get :activate_account, token: 'asdfasdf' end