ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

allow account admins to delete managed pseudonyms. fixes #6059 

testplan:
 * make sure user models destroy transactionally
   * create two users, one with a managed pseudonym and one without,
     destroy both, and make sure that the failure to remove the managed
     pseudonym makes the original user destroy bail
   * make sure you can force delete a user with managed pseudonyms
 * make sure that the account controller can destroy users with managed
   pseudonyms
 * make sure the users controller can destroy users with managed
   pseudonyms only when the current user has permission
   * also, make sure other user destruction functionality is unchanged
 * make sure the users controller doesn't confirm a deletion when
   the user doesn't have permission
 * make sure we don't add a user destruction link when we know the
   destruction will fail, but add it otherwise

Change-Id: Ie7c17de1134543fe55a3fdd03c60879925ecc50d
Reviewed-on: https://gerrit.instructure.com/6832
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
This commit is contained in:
JT Olds 2011-12-05 13:48:11 -07:00
parent 8c3a8d9f30
commit cf557d9089
10 changed files with 361 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
app/controllers/accounts_controller.rb
View File

@ -144,9 +144,10 @@ class AccountsController < ApplicationController
end
def confirm_delete_user
if authorized_action(@account, @current_user, :manage_user_logins)
@context = @account
@user = @account.all_users.find_by_id(params[:user_id]) if params[:user_id].present?
@root_account = @account.root_account || @account
if authorized_action(@root_account, @current_user, :manage_user_logins)
@context = @root_account
@user = @root_account.all_users.find_by_id(params[:user_id]) if params[:user_id].present?
if !@user
flash[:error] = t(:no_user_message, "No user found with that id")
redirect_to account_url(@account)
@ -155,26 +156,23 @@ class AccountsController < ApplicationController
end
def remove_user
if authorized_action(@account, @current_user, :manage_user_logins)
@user = UserAccountAssociation.find_by_account_id_and_user_id(@account.id, params[:user_id]).user rescue nil
@root_account = @account.root_account || @account
if authorized_action(@root_account, @current_user, :manage_user_logins)
@user = UserAccountAssociation.find_by_account_id_and_user_id(@root_account.id, params[:user_id]).user rescue nil
# if the user is in any account other then the
# current one, remove them from the current account
# instead of deleting them completely
account_ids = []
if @user
account_ids = @user.associated_root_accounts.map(&:id)
end
account_ids = account_ids.compact.uniq - [@account.id]
if @user && !account_ids.empty?
@root_account = @account.root_account || @account
@user.remove_from_root_account(@root_account)
else
@user && @user.destroy
if !(@user.associated_root_accounts.map(&:id).compact.uniq - [@root_account.id]).empty?
@user.remove_from_root_account(@root_account)
else
@user.destroy(true)
end
flash[:notice] = t(:user_deleted_message, "%{username} successfully deleted", :username => @user.name)
end
respond_to do |format|
flash[:notice] = t(:user_deleted_message, "%{username} successfully deleted", :username => @user.name) if @user
format.html { redirect_to account_url(@account) }
format.json { render :json => @user.to_json }
format.html { redirect_to account_users_url(@account) }
format.json { render :json => (@user || {}).to_json }
end
end
end

17
app/controllers/users_controller.rb
View File

@ -741,18 +741,20 @@ class UsersController < ApplicationController
def delete
@user = User.find(params[:user_id])
if authorized_action(@user, @current_user, :manage)
if @user.pseudonyms.any?{|p| p.managed_password? }
flash[:notice] = t('no_deleting_sis_user', "You cannot delete a system-generated user")
redirect_to profile_url
if authorized_action(@user, @current_user, [:manage, :manage_logins])
if @user.pseudonyms.any? {|p| p.managed_password? }
unless @user.grants_right?(@current_user, session, :manage_logins)
flash[:error] = t('no_deleting_sis_user', "You cannot delete a system-generated user")
redirect_to profile_url
end
end
end
end
def destroy
@user = User.find(params[:id])
if authorized_action(@user, @current_user, :manage)
@user.destroy
if authorized_action(@user, @current_user, [:manage, :manage_logins])
@user.destroy(@user.grants_right?(@current_user, session, :manage_logins))
if @user == @current_user
@pseudonym_session.destroy rescue true
reset_session
@ -925,6 +927,3 @@ class UsersController < ApplicationController
end
end

14
app/models/user.rb
View File

@ -657,12 +657,14 @@ class User < ActiveRecord::Base
end
alias_method :destroy!, :destroy
def destroy
self.workflow_state = 'deleted'
self.save
self.pseudonyms.each{|p| p.destroy }
self.communication_channels.each{|cc| cc.destroy }
self.enrollments.each{|e| e.destroy }
def destroy(even_if_managed_passwords=false)
ActiveRecord::Base.transaction do
self.workflow_state = 'deleted'
self.save
self.pseudonyms.each{|p| p.destroy(even_if_managed_passwords) }
self.communication_channels.each{|cc| cc.destroy }
self.enrollments.each{|e| e.destroy }
end
end
def remove_from_root_account(account)

11
app/views/users/_name.html.erb
View File

@ -4,7 +4,6 @@
can_manage_admin_users = managed_accounts
account ||= managed_accounts.first
root_account = account.root_account || account if account
can_manage_user_logins = @user.associated_root_accounts.active.any?{|a| can_do(a, @current_user, :manage_user_logins) }
%>
<% jammit_js :user_name %>
<fieldset id="name_and_email">
@ -56,13 +55,13 @@
<a href="<%= dashboard_url(:become_user_id => @user.id) %>"> <%= t('become', 'Become') %></a>
<% end %>
<% end %>
<% if can_manage_user_logins %>
<% if @user.associated_root_accounts.active.any?{|a| can_do(a, @current_user, :manage_user_logins) } %>
|
<a href="<%= user_admin_merge_url(@user.id) %>"><%= t('merge_with_another_user', 'Merge with Another User') %></a>
<% if (@context.is_a?(User) || @context.is_a?(Account)) && root_account %>
|
<a href="<%= account_confirm_delete_user_url(root_account.id, @user.id) %>"><%= t('delete_from_account', 'Delete from %{account}', :account => root_account.name) %></a>
<% end %>
<% end %>
<% if ((@context.is_a?(User) || @context.is_a?(Account)) && root_account) && @user.grants_right?(@current_user, session, :manage_logins) %>
|
<a href="<%= account_confirm_delete_user_url(root_account.id, @user.id) %>"><%= t('delete_from_account', 'Delete from %{account}', :account => root_account.name) %></a>
<% end %>
</td>
<% end %>

89
spec/controllers/accounts_controller_spec.rb
View File

@ -39,6 +39,95 @@ describe AccountsController do
@course2.course_sections.first.crosslist_to_course(@course1)
end
context "confirm_delete_user" do
it "should confirm deletion of canvas-authenticated users" do
account_with_admin_logged_in
user_with_pseudonym :account => @account
get 'confirm_delete_user', :account_id => @account.id, :user_id => @user.id
response.should be_success
end
it "should not confirm deletion of non-existent users" do
account_with_admin_logged_in
get 'confirm_delete_user', :account_id => @account.id, :user_id => (User.all.map(&:id).max + 1)
response.should redirect_to(account_url(@account))
flash[:error].should =~ /No user found with that id/
end
it "should confirm deletion of managed password users" do
account_with_admin_logged_in
user_with_managed_pseudonym :account => @account
get 'confirm_delete_user', :account_id => @account.id, :user_id => @user.id
response.should be_success
end
end
context "remove_user" do
it "should delete canvas-authenticated users" do
account_with_admin_logged_in
user_with_pseudonym :account => @account
@user.workflow_state.should == "pre_registered"
post 'remove_user', :account_id => @account.id, :user_id => @user.id
flash[:notice].should =~ /successfully deleted/
response.should redirect_to(account_users_url(@account))
@user.reload
@user.workflow_state.should == "deleted"
end
it "should do nothing for non-existent users as html" do
account_with_admin_logged_in
post 'remove_user', :account_id => @account.id, :user_id => (User.all.map(&:id).max + 1)
flash[:notice].should be_nil
response.should redirect_to(account_users_url(@account))
end
it "should do nothing for non-existent users as json" do
account_with_admin_logged_in
post 'remove_user', :account_id => @account.id, :user_id => (User.all.map(&:id).max + 1), :format => "json"
flash[:notice].should be_nil
json_parse(response.body).should == {}
end
it "should only remove users from the current account if the user exists in multiple accounts" do
@other_account = account_model
account_with_admin_logged_in
user_with_pseudonym :account => @account, :username => "nobody@example.com"
pseudonym @user, :account => @other_account, :username => "nobody2@example.com"
@user.workflow_state.should == "pre_registered"
@user.associated_accounts.map(&:id).include?(@account.id).should be_true
@user.associated_accounts.map(&:id).include?(@other_account.id).should be_true
post 'remove_user', :account_id => @account.id, :user_id => @user.id
flash[:notice].should =~ /successfully deleted/
response.should redirect_to(account_users_url(@account))
@user.reload
@user.workflow_state.should == "pre_registered"
@user.associated_accounts.map(&:id).include?(@account.id).should be_false
@user.associated_accounts.map(&:id).include?(@other_account.id).should be_true
end
it "should delete users who have managed passwords with html" do
account_with_admin_logged_in
user_with_managed_pseudonym :account => @account
@user.workflow_state.should == "pre_registered"
post 'remove_user', :account_id => @account.id, :user_id => @user.id
flash[:notice].should =~ /successfully deleted/
response.should redirect_to(account_users_url(@account))
@user.reload
@user.workflow_state.should == "deleted"
end
it "should delete users who have managed passwords with json" do
account_with_admin_logged_in
user_with_managed_pseudonym :account => @account
@user.workflow_state.should == "pre_registered"
post 'remove_user', :account_id => @account.id, :user_id => @user.id, :format => "json"
flash[:notice].should =~ /successfully deleted/
@user = json_parse(@user.reload.to_json)
json_parse(response.body).should == @user
@user["user"]["workflow_state"].should == "deleted"
end
end
describe "SIS imports" do
it "should set batch mode and term if given" do
account_with_admin_logged_in

102
spec/controllers/users_controller_spec.rb
View File

@ -63,6 +63,108 @@ describe UsersController do
courses.map { |c| c['id'] }.should == [course2.id]
end
context "GET 'delete'" do
it "should fail when the user doesn't exist" do
account_admin_user
user_session(@admin)
lambda { get 'delete', :user_id => (User.all.map(&:id).max + 1)}.should raise_error(ActiveRecord::RecordNotFound)
end
it "should fail when the current user doesn't have user manage permissions" do
course_with_teacher_logged_in
student_in_course :course => @course
get 'delete', :user_id => @student.id
response.status.should =~ /401 Unauthorized/
end
it "should succeed when the current user has the :manage permission and is not deleting any system-generated pseudonyms" do
course_with_student_logged_in
get 'delete', :user_id => @student.id
response.should be_success
end
it "should fail when the current user won't be able to delete managed pseudonyms" do
course_with_student_logged_in
managed_pseudonym @student
get 'delete', :user_id => @student.id
flash[:error].should =~ /cannot delete a system-generated user/
response.redirected_to.should == profile_url
end
it "should succeed when the current user has enough permissions to delete any system-generated pseudonyms" do
account_admin_user
user_session(@admin)
course_with_student
managed_pseudonym @student
get 'delete', :user_id => @student.id
flash[:error].should_not =~ /cannot delete a system-generated user/
response.should be_success
end
end
context "POST 'destroy'" do
it "should fail when the user doesn't exist" do
account_admin_user
user_session(@admin)
PseudonymSession.find(1).stubs(:destroy).returns(nil)
lambda { post 'destroy', :id => (User.all.map(&:id).max + 1)}.should raise_error(ActiveRecord::RecordNotFound)
end
it "should fail when the current user doesn't have user manage permissions" do
course_with_teacher_logged_in
student_in_course :course => @course
PseudonymSession.find(1).stubs(:destroy).returns(nil)
post 'destroy', :id => @student.id
response.status.should =~ /401 Unauthorized/
@student.reload.workflow_state.should_not == 'deleted'
end
it "should succeed when the current user has the :manage permission and is not deleting any system-generated pseudonyms" do
course_with_student_logged_in
PseudonymSession.find(1).stubs(:destroy).returns(nil)
post 'destroy', :id => @student.id
response.redirected_to.should == root_url
@student.reload.workflow_state.should == 'deleted'
end
it "should fail when the current user won't be able to delete managed pseudonyms" do
course_with_student_logged_in
managed_pseudonym @student
PseudonymSession.find(1).stubs(:destroy).returns(nil)
lambda { post 'destroy', :id => @student.id }.should raise_error
@student.reload.workflow_state.should_not == 'deleted'
end
it "should succeed when the current user has enough permissions to delete any system-generated pseudonyms" do
account_admin_user
user_session(@admin)
course_with_student
managed_pseudonym @student
PseudonymSession.find(1).stubs(:destroy).returns(nil)
post 'destroy', :id => @student.id
response.redirected_to.should == users_url
@student.reload.workflow_state.should == 'deleted'
end
it "should clear the session and log the user out when the current user deletes himself, with managed pseudonyms and :manage_login permissions" do
account_admin_user
user_session(@admin)
managed_pseudonym @admin
PseudonymSession.find(1).expects(:destroy).returns(nil)
post 'destroy', :id => @admin.id
response.redirected_to.should == root_url
@admin.reload.workflow_state.should == 'deleted'
end
it "should clear the session and log the user out when the current user deletes himself, without managed pseudonyms and :manage_login permissions" do
course_with_student_logged_in
PseudonymSession.find(1).expects(:destroy).returns(nil)
post 'destroy', :id => @student.id
response.redirected_to.should == root_url
@student.reload.workflow_state.should == 'deleted'
end
end
context "POST 'create'" do
it "should not allow creating when open_registration is disabled and you're not an admin'" do
post 'create', :pseudonym => { :unique_id => 'jacob@instructure.com' }, :user => { :name => 'Jacob Fugal' }

23
spec/models/user_spec.rb
View File

@ -320,6 +320,29 @@ describe User do
]
end
it "should delete the user transactionally in case the pseudonym removal fails" do
user_with_managed_pseudonym
@pseudonym.should be_managed_password
@user.workflow_state.should == "pre_registered"
lambda { @user.destroy }.should raise_error("Cannot delete system-generated pseudonyms")
@user.workflow_state.should == "deleted"
@user.reload
@user.workflow_state.should == "pre_registered"
@account.account_authorization_config.destroy
@pseudonym.should_not be_managed_password
@user.destroy
@user.workflow_state.should == "deleted"
@user.reload
@user.workflow_state.should == "deleted"
user_with_managed_pseudonym
@pseudonym.should be_managed_password
@user.workflow_state.should == "pre_registered"
@user.destroy(true)
@user.workflow_state.should == "deleted"
@user.reload
@user.workflow_state.should == "deleted"
end
context "move_to_user" do
it "should delete the old user" do
@user1 = user_model

52
spec/spec_helper.rb
View File

@ -175,18 +175,13 @@ Spec::Runner.configure do |config|
end
def user_with_pseudonym(opts={})
user = user_with_communication_channel(opts)
username = opts[:username] || "nobody@example.com"
password = opts[:password] || "asdfasdf"
password = nil if password == :autogenerate
@pseudonym = user.pseudonyms.create!(:account => opts[:account] || Account.default, :unique_id => username, :password => password, :password_confirmation => password)
@pseudonym.communication_channel = @cc
user(opts) unless opts[:user]
user = opts[:user] || @user
@pseudonym = pseudonym(user, opts)
user
end
def user_with_communication_channel(opts={})
user(opts) unless opts[:user]
user = opts[:user] || @user
def communication_channel(user, opts={})
username = opts[:username] || "nobody@example.com"
@cc = user.communication_channels.create!(:path_type => 'email', :path => username) do |cc|
cc.workflow_state = 'active' if opts[:active_cc] || opts[:active_all]
@ -194,6 +189,45 @@ Spec::Runner.configure do |config|
end
@cc.should_not be_nil
@cc.should_not be_new_record
@cc
end
def user_with_communication_channel(opts={})
user(opts) unless opts[:user]
user = opts[:user] || @user
@cc = communication_channel(user, opts)
user
end
def pseudonym(user, opts={})
username = opts[:username] || "nobody@example.com"
password = opts[:password] || "asdfasdf"
password = nil if password == :autogenerate
@pseudonym = user.pseudonyms.create!(:account => opts[:account] || Account.default, :unique_id => username, :password => password, :password_confirmation => password)
@pseudonym.communication_channel = communication_channel(user, opts)
@pseudonym
end
def managed_pseudonym(user, opts={})
other_account = opts[:account] || account_with_saml
if other_account.password_authentication?
config = AccountAuthorizationConfig.new
config.auth_type = "saml"
config.log_in_url = opts[:saml_log_in_url] if opts[:saml_log_in_url]
other_account.account_authorization_configs << config
end
opts[:account] = other_account
pseudonym(user, opts)
@pseudonym.sis_user_id = opts[:sis_user_id] || "U001"
@pseudonym.save!
@pseudonym.should be_managed_password
@pseudonym
end
def user_with_managed_pseudonym(opts={})
user(opts) unless opts[:user]
user = opts[:user] || @user
managed_pseudonym(user, opts)
user
end

64
spec/views/users/_name.html.erb_spec.rb Normal file
View File

@ -0,0 +1,64 @@
#
# Copyright (C) 2011 Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
#
require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper')
require File.expand_path(File.dirname(__FILE__) + '/../views_helper')
describe "/users/name" do
it "should allow deletes for unmanagaged pseudonyms with correct privileges" do
account_admin_user :account => Account.default
course_with_student :account => Account.default
view_context(Account.default, @admin)
assigns[:user] = @student
assigns[:enrollments] = []
render :partial => "users/name"
response.body.should =~ /Delete from #{Account.default.name}/
end
it "should allow deletes for managaged pseudonyms with correct privileges" do
account_admin_user :account => Account.default
course_with_student :account => Account.default
managed_pseudonym(@student, :account => account_model)
view_context(Account.default, @admin)
assigns[:user] = @student
assigns[:enrollments] = []
render :partial => "users/name"
response.body.should =~ /Delete from #{Account.default.name}/
end
it "should not allow deletes for managed pseudonyms without correct privileges" do
@admin = user :account => Account.default
course_with_student :account => Account.default
managed_pseudonym(@student, :account => account_model)
view_context(Account.default, @admin)
assigns[:user] = @student
assigns[:enrollments] = []
render :partial => "users/name"
response.body.should_not =~ /Delete from #{Account.default.name}/
end
it "should not allow deletes for unmanaged pseudonyms without correct privileges" do
@admin = user :account => Account.default
course_with_student :account => Account.default
view_context(Account.default, @admin)
assigns[:user] = @student
assigns[:enrollments] = []
render :partial => "users/name"
response.body.should_not =~ /Delete from #{Account.default.name}/
end
end

7
spec/views/users/show.html.erb_spec.rb
View File

@ -28,9 +28,10 @@ describe "/users/show" do
assigns[:topics] = []
assigns[:recent_events] = []
assigns[:upcoming_events] = []
assigns[:enrollments] = []
assigns[:group_memberships] = []
# render "users/show"
# response.should_not be_nil
render "users/show"
response.should_not be_nil
end
end