Permalink
Browse files

Fixed up loop holes on being able to delete the last administrator

  • Loading branch information...
mikel committed May 18, 2009
1 parent 412e0b8 commit 557ee0768f11abfc70235aa1f25f826111719a32
@@ -11,7 +11,7 @@ class ApplicationController < ActionController::Base
private
def admin_only
- unless current_user.member_of?(:admin)
+ unless current_user.admin
flash[:notice] = "You must be an administrator to access this page"
redirect_to root_path
return false
@@ -38,6 +38,7 @@ def update
@user = User.find(current_user.id)
@user.attributes = params[:user]
end
+
if @user.save
if current_user.admin
flash[:notice] = "Update Successful"
View
@@ -5,28 +5,22 @@ class User < ActiveRecord::Base
has_many :roles, :through => :memberships
attr_accessible :given_name, :family_name, :email, :password, :password_confirmation
-
- # Makes this user a member of the administrator group
- def add_role!(role_name)
- role = Role.find_by_name!(role_name.to_s)
- self.roles << role unless self.roles.include?(role)
- end
-
- def remove_role!(role_name)
- role = Role.find_by_name!(role_name.to_s)
- self.roles.delete(role) if self.roles.include?(role)
- end
- # Returns true if this user is an administrator
- def member_of?(role_name)
- !!self.roles.find_by_name(role_name.to_s)
+ validate_on_update :check_last_admin?
+
+ def check_last_admin?
+ if @tried_to_remove_admin && last_admin?
+ errors.add_to_base "Can not remove the last admin from the system."
+ end
end
def admin=(bool)
if boolianize(bool)
+ @tried_to_remove_admin = false
add_role!(:admin)
else
- remove_role!(:admin)
+ @tried_to_remove_admin = true
+ remove_role!(:admin) unless last_admin?
end
end
@@ -35,11 +29,29 @@ def admin
end
def last_admin?
- !User.find(:first, :conditions => ["users.id != ? AND roles.name = 'admin'", self.id],
- :joins => [:roles, :memberships])
+ @last_admin ||= !User.find(:first,
+ :conditions => ["users.id != ? AND roles.name = 'admin'", self.id],
+ :joins => [:roles, :memberships])
end
private
+
+ # Adds this role from the users memberships
+ def add_role!(role_name)
+ role = Role.find_by_name!(role_name.to_s)
+ self.roles << role unless self.roles.include?(role)
+ end
+
+ # Removes this role from the users memberships
+ def remove_role!(role_name)
+ role = Role.find_by_name!(role_name.to_s)
+ self.roles.delete(role) if self.roles.include?(role)
+ end
+
+ # Returns true if this user is a member of this role
+ def member_of?(role_name)
+ !!self.roles.find_by_name(role_name.to_s)
+ end
def boolianize(bool)
case bool
@@ -24,14 +24,6 @@ def self.up
t.timestamps
end
- User.reset_column_information
- User.create!(:login => 'admin',
- :password => 'mailer',
- :password_confirmation => 'mailer',
- :given_name => 'Default',
- :family_name => 'Admin',
- :email => 'admin@nowaythisisadomainname.org.au')
-
end
def self.down
@@ -5,9 +5,6 @@ def self.up
t.timestamps
end
-
- Role.create!(:name => 'admin')
- User.find(:first).add_role!('admin')
end
def self.down
@@ -117,6 +117,7 @@ Feature: Managing users
Scenario: User updating their own profile
Given I am logged in
+ And there is an admin in the system
When I go to the edit user page for "bsmith"
And I fill in "email" with "sammy@you.com"
And I fill in "given name" with "Sammy"
@@ -15,6 +15,11 @@
end
Given /^"([^\"]*)" is a.? "([^\"]*)"$/ do |user, role|
- User.find_by_login(user).add_role!(role)
+ User.find_by_login(user).send("#{role}=", true)
end
+Given /^there is an admin in the system$/ do
+ user = Factory(:user, :login => 'mr_admin')
+ role = Factory(:admin_role)
+ user.admin = true
+end
@@ -12,7 +12,7 @@ def login_user(params = [])
def make_administrator(user)
role = Factory(:admin_role)
- user.add_role!(:admin)
+ user.admin = true
end
end
View
@@ -0,0 +1,14 @@
+
+desc 'Bootstraps the system'
+task :bootstrap => :environment do
+ user = User.new(:password => 'mailer',
+ :password_confirmation => 'mailer',
+ :given_name => 'Default',
+ :family_name => 'Admin',
+ :email => 'admin@nowaythisisadomainname.org.au')
+ user.login = 'admin'
+ Role.create!(:name => 'admin')
+ Membership.create!(:user_id => 1, :role_id => 1)
+ user.save!
+ user.add_role!('admin')
+end
View
@@ -2,41 +2,41 @@
describe User do
- describe "memberships API" do
+ describe "Internal Membership handling (private methods)" do
it "should add itself to the admin role" do
user = Factory(:user)
role = Factory(:admin_role)
- user.add_role!(:admin)
+ user.send(:add_role!, :admin)
user.roles.should include(role)
end
it "should say if it is an admin" do
user = Factory(:user)
role = Factory(:admin_role)
- user.add_role!(:admin)
- user.member_of?(:admin).should be_true
+ user.send(:add_role!, :admin)
+ user.send(:member_of?, :admin).should be_true
end
it "should only add any role once" do
user = Factory(:user)
role = Factory(:admin_role)
doing {
- user.add_role!(:admin)
- user.add_role!(:admin)
- user.add_role!(:admin)
+ user.send(:add_role!, :admin)
+ user.send(:add_role!, :admin)
+ user.send(:add_role!, :admin)
}.should change(Membership, :count).by(1)
end
it "should raise an error if the role was not found" do
user = Factory(:user)
- doing {user.add_role!(:user)}.should raise_error(ActiveRecord::RecordNotFound, "Couldn't find Role with name = user")
+ doing {user.send(:add_role!, :user)}.should raise_error(ActiveRecord::RecordNotFound, "Couldn't find Role with name = user")
end
it "should remove a role" do
user = Factory(:user)
role = Factory(:admin_role)
- user.add_role!(:admin)
- doing { user.remove_role!(:admin) }.should change(Membership, :count).by(-1)
+ user.send(:add_role!, :admin)
+ doing { user.send(:remove_role!, :admin) }.should change(Membership, :count).by(-1)
user.roles.should_not include(role)
end
end
@@ -46,7 +46,7 @@
user = Factory(:user)
role = Factory(:admin_role)
user.admin = true
- user.save
+ user.save!
user.admin.should be_true
end
@@ -58,17 +58,23 @@
user.admin.should be_true
end
- it "should allow you remove admin rights" do
+ it "should not allow you remove admin rights if it is the only admin" do
user = Factory(:user)
role = Factory(:admin_role)
user.admin = true
user.save
user.admin = false
user.save
- user.admin.should be_false
+ user.admin.should be_true
end
it "should handle integer strings passed into admin= for false" do
+ # Setup another admin so we can remove the admin rights
+ user2 = Factory(:user, :login => 'mr_admin')
+ role2 = Factory(:admin_role)
+ user2.admin = true
+ user2.save
+
user = Factory(:user)
role = Factory(:admin_role)
user.admin = true
@@ -97,6 +103,22 @@
user1.should_not be_last_admin
user2.should_not be_last_admin
end
+
+ it "should validate ok if it is the last admin in the system" do
+ user = Factory(:user)
+ role = Factory(:admin_role)
+ user.admin = true
+ user.should be_valid
+ end
+
+ it "should not validate ok if it is the last admin and trying to change to not an admin" do
+ user = Factory(:user)
+ role = Factory(:admin_role)
+ user.admin = true
+ user.save!
+ user.admin = false
+ user.should_not be_valid
+ end
end
describe "mass assignment protection" do
@@ -114,7 +136,6 @@
user = User.new({:given_name => 'mikel'})
user.given_name.should == 'mikel'
end
-
end
end

0 comments on commit 557ee07

Please sign in to comment.