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

clean up user "deletion" 

fixes CNVS-1552

any time the UI/API tries to "delete" a user, it should only be trying
to remove it from some root account (the @domain_root_account if not
otherwise specified). if that root account was the last root account the
user was associated with, then the remnants of the user are fully
deleted, but only then. leave User#destroy as a short-cut to delete the
user from all their accounts at once, but should not be invoked directly
from any UI/API actions.

test-plan:

 PERMISSIONS

   being able to remove a user from an account entails being able to:

   - DELETE http://accounts-domain/users/:user
   - DELETE /accounts/:account/users/:user

   both should fail or succeed together

   * given
     - Sally who's an admin with the :manage_user_logins
       permission on one account (Account1) and a student on another
       account (Account2)
     - Bob who's a student on both accounts
     - Alice who's an admin on Account1 with greater permissions than
       Sally

   * Sally should:
     - see "Delete My Account" on her Account1 profile
     - not see "Delete My Account" on her Account2 profile
     - not see "Delete My Account" on Bob's Account1 profile
     - not see "Delete My Account" on Alice's Account1 profile
     - see "Delete from Account1" at /users/:sally
     - see "Delete from Account1" at /users/:bob
     - not see "Delete from Account2" at /users/:sally
     - not see "Delete from Account2" at /users/:bob
     - not see "Delete from Account1" at /users/:alice
     - be able to remove herself from Account1
     - be able to remove Bob from Account1
     - not be able to remove herself from Account2
     - not be able to remove Bob from Account2
     - not be able to remove Alice from Account1

   * given Sally's Account1 pseudonym has a SIS ID but her Account2
     pseudonym doesn't, Sally should:
     - no longer see "Delete My Account" on her Account1 profile
     - no longer see "Delete from Account1" at /users/:sally
     - still see "Delete from Account1" at /users/:bob
     - no longer be able to remove herself from Account1
     - still be able to remove Bob from Account1

 EFFECTS

 * as Sally, remove Bob from Account1 via
   DELETE http://account1-domain/users/:bob
   - Bob's pseudonyms, enrollments, etc. in Account1 should be removed
   - Bob's pseudonyms, enrollments, etc. in Account2 should be untouched

 * repeat using DELETE /accounts/:account1/users/:bob, with the same
   expectations

Change-Id: Ib7612f95d1c7e4cca36d8486950565ec096b4ab1
Reviewed-on: https://gerrit.instructure.com/41591
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: August Thornton <august@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
Product-Review: Jacob Fugal <jacob@instructure.com>
This commit is contained in:
Jacob Fugal 2014-09-22 17:04:03 -06:00
parent 49ea94f33e
commit 54649a4d62
10 changed files with 207 additions and 213 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

56
app/controllers/accounts_controller.rb
View File

@ -558,36 +558,48 @@ class AccountsController < ApplicationController
end
def confirm_delete_user
@root_account = @account.root_account
if authorized_action(@root_account, @current_user, :manage_user_logins)
@context = @root_account
@user = @root_account.all_users.where(id: params[:user_id]).first if params[:user_id].present?
if !@user
flash[:error] = t(:no_user_message, "No user found with that id")
redirect_to account_url(@account)
end
raise ActiveRecord::RecordNotFound unless @account.root_account?
@user = api_find(User, params[:user_id])
unless @account.user_account_associations.where(user_id: @user).exists?
flash[:error] = t(:no_user_message, "No user found with that id")
redirect_to account_url(@account)
return
end
@context = @account
render_unauthorized_action unless @user.allows_user_to_remove_from_account?(@account, @current_user)
end
# @API Delete a user from the root account
#
# Delete a user record from a Canvas root account. If a user is associated
# with multiple root accounts (in a multi-tenant instance of Canvas), this
# action will NOT remove them from the other accounts.
#
# WARNING: This API will allow a user to remove themselves from the account.
# If they do this, they won't be able to make API calls or log into Canvas at
# that account.
#
# @example_request
# curl https://<canvas>/api/v1/accounts/3/users/5 \
# -H 'Authorization: Bearer <ACCESS_TOKEN>' \
# -X DELETE
#
# @returns User
def remove_user
@root_account = @account.root_account
if authorized_action(@root_account, @current_user, :manage_user_logins)
@user = UserAccountAssociation.where(account_id: @root_account.id, user_id: params[:user_id]).first.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
if @user
if !(@user.associated_root_accounts.map(&:id).compact.uniq - [@root_account.id]).empty?
@user.remove_from_root_account(@root_account)
else
@user.destroy
end
flash[:notice] = t(:user_deleted_message, "%{username} successfully deleted", :username => @user.name)
end
raise ActiveRecord::RecordNotFound unless @account.root_account?
@user = api_find(User, params[:user_id])
raise ActiveRecord::RecordNotFound unless @account.user_account_associations.where(user_id: @user).exists?
if @user.allows_user_to_remove_from_account?(@account, @current_user)
@user.remove_from_root_account(@account)
flash[:notice] = t(:user_deleted_message, "%{username} successfully deleted", :username => @user.name)
respond_to do |format|
format.html { redirect_to account_users_url(@account) }
format.json { render :json => @user || {} }
end
else
render_unauthorized_action
end
end

26
app/controllers/users_controller.rb
View File

@ -1461,31 +1461,13 @@ class UsersController < ApplicationController
def delete
@user = User.find(params[:user_id])
if authorized_action(@user, @current_user, [:manage, :manage_logins])
unless @user.grants_right?(@current_user, :delete)
flash[:error] = t('no_deleting_sis_user', "You cannot delete a system-generated user")
redirect_to user_profile_url(@current_user)
end
end
render_unauthorized_action unless @user.allows_user_to_remove_from_account?(@domain_root_account, @current_user)
end
# @API Delete a user
#
# Delete a user record from Canvas.
#
# WARNING: This API will allow a user to delete themselves. If you do this,
# you won't be able to make API calls or log into Canvas.
#
# @example_request
# curl https://<canvas>/api/v1/users/5 \
# -H 'Authorization: Bearer <ACCESS_TOKEN>' \
# -X DELETE
#
# @returns User
def destroy
@user = api_find(User, params[:id])
if authorized_action(@user, @current_user, :delete)
@user.destroy
if @user.allows_user_to_remove_from_account?(@domain_root_account, @current_user)
@user.remove_from_root_account(@domain_root_account)
if @user == @current_user
logout_current_user
end
@ -1501,6 +1483,8 @@ class UsersController < ApplicationController
render :json => user_json(@user, @current_user, session)
end
end
else
render_unauthorized_action
end
end

71
app/models/user.rb
View File

@ -810,33 +810,54 @@ class User < ActiveRecord::Base
alias_method :destroy!, :destroy
def destroy
ActiveRecord::Base.transaction do
self.workflow_state = 'deleted'
self.deleted_at = Time.now.utc
self.save
self.pseudonyms.each{|p| p.destroy }
self.communication_channels.each{|cc| cc.destroy }
self.delete_enrollments
end
self.remove_from_root_account(:all)
self.workflow_state = 'deleted'
self.deleted_at = Time.now.utc
self.save
end
# avoid extraneous callbacks when enrolled in multiple sections
def delete_enrollments
courses_to_update = self.enrollments.active.select(:course_id).uniq.map(&:course_id)
def delete_enrollments(enrollment_scope=self.enrollments)
courses_to_update = enrollment_scope.active.uniq.pluck(:course_id)
Enrollment.suspend_callbacks(:update_cached_due_dates) do
self.enrollments.each { |e| e.destroy }
enrollment_scope.each{ |e| e.destroy }
end
courses_to_update.each do |course|
DueDateCacher.recompute_course(course)
end
end
def remove_from_root_account(account)
self.enrollments.where(root_account_id: account).each(&:destroy)
self.pseudonyms.active.where(account_id: account).each(&:destroy)
self.account_users.where(account_id: account).each(&:destroy)
self.save
self.update_account_associations
def remove_from_root_account(root_account)
ActiveRecord::Base.transaction do
if root_account == :all
# make sure to hit all shards
enrollment_scope = self.enrollments.shard(self)
pseudonym_scope = self.pseudonyms.active.shard(self)
account_user_scope = self.account_users.shard(self)
has_other_root_accounts = false
else
# make sure to do things on the root account's shard. but note,
# root_account.enrollments won't include the student view user's
# enrollments, so we need to fetch them off the user instead; the
# student view user won't be cross shard, so that will still be the
# right shard
enrollment_scope = fake_student? ? self.enrollments : root_account.enrollments.where(user_id: self)
pseudonym_scope = root_account.pseudonyms.active.where(user_id: self)
account_user_scope = root_account.account_users.where(user_id: self)
has_other_root_accounts = self.associated_accounts.shard(self).where('accounts.id <> ?', root_account).exists?
end
self.delete_enrollments(enrollment_scope)
pseudonym_scope.each(&:destroy)
account_user_scope.each(&:destroy)
# only delete the user's communication channels when the last account is
# removed (they don't belong to any particular account). they will always
# be on the user's shard
self.communication_channels.each(&:destroy) unless has_other_root_accounts
self.update_account_associations
end
end
def associate_with_shard(shard, strength = :strong)
@ -1005,15 +1026,7 @@ class User < ActiveRecord::Base
self.all_accounts.select(&:root_account?).all? {|a| has_subset_of_account_permissions?(user, a) } )
)
end
can :manage_user_details and can :manage_logins and can :rename
given do |user|
user && (
self.grants_right?(user, :manage_logins) ||
(self.grants_right?(user, :manage) && self.pseudonyms.none? {|p| p.system_created?})
)
end
can :delete
can :manage_user_details and can :rename
given{ |user| self.pseudonyms.with_each_shard.any?{ |p| p.grants_right?(user, :update) } }
can :merge
@ -1048,6 +1061,12 @@ class User < ActiveRecord::Base
end
end
def allows_user_to_remove_from_account?(account, other_user)
Pseudonym.new(account: account, user: self).grants_right?(other_user, :delete) &&
(Pseudonym.new(account: account, user: self).grants_right?(other_user, :manage_sis) ||
!account.pseudonyms.active.where(user_id: self).where('sis_user_id IS NOT NULL').exists?)
end
def self.infer_id(obj)
case obj
when User

2
app/views/profile/profile.html.erb
View File

@ -141,7 +141,7 @@
<tr class="edit_data_row" style="display: none;">
<td colspan="2">
<% if @user.grants_right?(@current_user, :delete) %>
<% if @user == @current_user && @user.allows_user_to_remove_from_account?(@domain_root_account, @current_user) %>
<p><a style="font-size: 0.9em" href="<%= user_delete_url(@user.id) %>" class="admin"><%= t('links.delete_account', "Delete My Account") %> </a></p>
<% end %>
<div class="form-actions">

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

@ -60,7 +60,7 @@
|
<a class="merge_user_link" href="<%= user_admin_merge_url(@user.id) %>"><%= t('merge_with_another_user', 'Merge with Another User')%></a>
<% end %>
<% if ((@context.is_a?(User) || @context.is_a?(Account)) && root_account) && @user.grants_right?(@current_user, session, :manage_logins) %>
<% if ((@context.is_a?(User) || @context.is_a?(Account)) && root_account) && @user.allows_user_to_remove_from_account?(root_account, @current_user) %>
|
<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 %>

5
config/routes.rb
View File

@ -522,7 +522,8 @@ CanvasRails::Application.routes.draw do
get 'users/:user_id/delete' => 'accounts#confirm_delete_user', as: :confirm_delete_user
delete 'users/:user_id' => 'accounts#remove_user', as: :delete_user
resources :users
# create/delete are handled by specific routes just above
resources :users, only: [:index, :new, :edit, :show, :update]
resources :account_notifications, only: [:create, :destroy]
concerns :announcements
resources :assignments
@ -1035,7 +1036,6 @@ CanvasRails::Application.routes.draw do
delete 'users/self/todo/:asset_string/:purpose', action: :ignore_item, as: 'users_todo_ignore'
post 'accounts/:account_id/users', action: :create
get 'accounts/:account_id/users', action: :index, as: 'account_users'
delete 'accounts/:account_id/users/:id', action: :destroy
put 'users/:id', action: :update
post 'users/:user_id/files', action: :create_file
@ -1084,6 +1084,7 @@ CanvasRails::Application.routes.draw do
get 'accounts/:account_id/courses', action: :courses_api, as: 'account_courses'
get 'accounts/:account_id/sub_accounts', action: :sub_accounts, as: 'sub_accounts'
get 'accounts/:account_id/courses/:id', controller: :courses, action: :show, as: 'account_course_show'
delete 'accounts/:account_id/users/:user_id', action: :remove_user
end
scope(controller: :sub_accounts) do

48
spec/apis/v1/users_api_spec.rb
View File

@ -914,28 +914,23 @@ describe "Users API", type: :request do
end
end
describe "user deletion" do
describe "removing user from account" do
before :once do
@admin = account_admin_user
course_with_student(:user => user_with_pseudonym(:name => 'Student', :username => 'student@example.com'))
@student = @user
@user = @admin
@path = "/api/v1/accounts/#{Account.default.id}/users/#{@student.id}"
@path_options = { :controller => 'users', :action => 'destroy',
:format => 'json', :id => @student.to_param,
@path_options = { :controller => 'accounts', :action => 'remove_user',
:format => 'json', :user_id => @student.to_param,
:account_id => Account.default.to_param }
end
context "a user with permissions" do
it "should be able to delete a user" do
json = api_call(:delete, @path, @path_options)
expect(@student.reload).to be_deleted
expect(json).to eq({
'id' => @student.id,
'name' => 'Student',
'short_name' => 'Student',
'sortable_name' => 'Student'
})
expect(@student.associated_accounts).not_to include(Account.default)
expect(json.to_json).to eq @student.reload.to_json
end
it "should be able to delete a user by SIS ID" do
@ -943,22 +938,17 @@ describe "Users API", type: :request do
id_param = "sis_user_id:#{@student.pseudonyms.first.sis_user_id}"
path = "/api/v1/accounts/#{Account.default.id}/users/#{id_param}"
path_options = @path_options.merge(:id => id_param)
path_options = @path_options.merge(:user_id => id_param)
api_call(:delete, path, path_options)
expect(response.code).to eql '200'
expect(@student.reload).to be_deleted
expect(@student.associated_accounts).not_to include(Account.default)
end
it 'should be able to delete itself' do
path = "/api/v1/accounts/#{Account.default.to_param}/users/#{@user.id}"
json = api_call(:delete, path, @path_options.merge(:id => @user.to_param))
expect(@user.reload).to be_deleted
expect(json).to eq({
'id' => @user.id,
'name' => @user.name,
'short_name' => @user.short_name,
'sortable_name' => @user.sortable_name
})
json = api_call(:delete, path, @path_options.merge(:user_id => @user.to_param))
expect(@user.associated_accounts).not_to include(Account.default)
expect(json.to_json).to eq @user.reload.to_json
end
end
@ -970,24 +960,10 @@ describe "Users API", type: :request do
end
end
context 'a student with no SIS ID' do
it 'should be able to delete itself' do
path = "/api/v1/accounts/#{Account.default.to_param}/users/#{@student.id}"
json = api_call_as_user(@student, :delete, path, @path_options.merge(:id => @student.to_param))
expect(@student.reload).to be_deleted
end
end
context 'a student with SIS ID' do
before(:once) do
@student.pseudonym.update_attribute(:sis_user_id, '12345')
end
context 'a non-admin user' do
it 'should not be able to delete itself' do
path = "/api/v1/accounts/#{Account.default.to_param}/users/#{@student.id}"
json = api_call_as_user(@student, :delete, path, @path_options.merge(:id => @student.to_param),
{}, {}, {expected_status: 401})
expect(@student.reload).not_to be_deleted
api_call_as_user(@student, :delete, path, @path_options.merge(:user_id => @student.to_param), {}, {}, expected_status: 401)
end
end
end

66
spec/controllers/accounts_controller_spec.rb
View File

@ -55,8 +55,7 @@ describe AccountsController do
it "should not confirm deletion of non-existent users" do
get 'confirm_delete_user', :account_id => @account.id, :user_id => (User.all.map(&:id).max + 1)
expect(response).to redirect_to(account_url(@account))
expect(flash[:error]).to match /No user found with that id/
expect(response).to be_not_found
end
it "should confirm deletion of managed password users" do
@ -70,63 +69,74 @@ describe AccountsController do
before(:once) { account_with_admin }
before(:each) { user_session(@admin) }
it "should delete canvas-authenticated users" do
it "should remove user from the account" do
user_with_pseudonym :account => @account
expect(@user.workflow_state).to eq "pre_registered"
post 'remove_user', :account_id => @account.id, :user_id => @user.id
expect(flash[:notice]).to match /successfully deleted/
expect(response).to redirect_to(account_users_url(@account))
@user.reload
expect(@user.workflow_state).to eq "deleted"
expect(@user.associated_accounts.map(&:id)).not_to include(@account.id)
end
it "should do nothing for non-existent users as html" do
it "should 404 for non-existent users as html" do
post 'remove_user', :account_id => @account.id, :user_id => (User.all.map(&:id).max + 1)
expect(flash[:notice]).to be_nil
expect(response).to redirect_to(account_users_url(@account))
expect(response).to be_not_found
end
it "should do nothing for non-existent users as json" do
it "should 404 for non-existent users as json" do
post 'remove_user', :account_id => @account.id, :user_id => (User.all.map(&:id).max + 1), :format => "json"
expect(flash[:notice]).to be_nil
expect(json_parse(response.body)).to eq({})
expect(response).to be_not_found
end
it "should only remove users from the current account if the user exists in multiple accounts" do
it "should only remove user from the account, but not delete them" do
user_with_pseudonym :account => @account
workflow_state_was = @user.workflow_state
post 'remove_user', :account_id => @account.id, :user_id => @user.id
expect(@user.reload.workflow_state).to eql workflow_state_was
end
it "should only remove users from the specified account" 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"
expect(@user.workflow_state).to eq "pre_registered"
expect(@user.associated_accounts.map(&:id).include?(@account.id)).to be_truthy
expect(@user.associated_accounts.map(&:id).include?(@other_account.id)).to be_truthy
post 'remove_user', :account_id => @account.id, :user_id => @user.id
expect(flash[:notice]).to match /successfully deleted/
expect(response).to redirect_to(account_users_url(@account))
@user.reload
expect(@user.workflow_state).to eq "pre_registered"
expect(@user.associated_accounts.map(&:id).include?(@account.id)).to be_falsey
expect(@user.associated_accounts.map(&:id).include?(@other_account.id)).to be_truthy
expect(@user.associated_accounts.map(&:id)).not_to include(@account.id)
expect(@user.associated_accounts.map(&:id)).to include(@other_account.id)
end
it "should delete users who have managed passwords with html" do
it "should delete the user's CCs when removed from their last account" do
user_with_pseudonym :account => @account
post 'remove_user', :account_id => @account.id, :user_id => @user.id
expect(@user.communication_channels.unretired).to be_empty
end
it "should not delete the user's CCs when other accounts remain" 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"
post 'remove_user', :account_id => @account.id, :user_id => @user.id
expect(@user.communication_channels.unretired).not_to be_empty
end
it "should remove users with managed passwords with html" do
user_with_managed_pseudonym :account => @account
expect(@user.workflow_state).to eq "pre_registered"
post 'remove_user', :account_id => @account.id, :user_id => @user.id
expect(flash[:notice]).to match /successfully deleted/
expect(response).to redirect_to(account_users_url(@account))
@user.reload
expect(@user.workflow_state).to eq "deleted"
expect(@user.associated_accounts.map(&:id)).not_to include(@account.id)
end
it "should delete users who have managed passwords with json" do
user_with_managed_pseudonym :account => @account
expect(@user.workflow_state).to eq "pre_registered"
it "should remove users with managed passwords with json" do
user_with_managed_pseudonym :account => @account, :name => "John Doe"
post 'remove_user', :account_id => @account.id, :user_id => @user.id, :format => "json"
expect(flash[:notice]).to match /successfully deleted/
@user = json_parse(@user.reload.to_json)
expect(json_parse(response.body)).to eq @user
expect(@user["user"]["workflow_state"]).to eq "deleted"
expect(json_parse(response.body)).to eq json_parse(@user.reload.to_json)
expect(@user.associated_accounts.map(&:id)).to_not include(@account.id)
end
end

84
spec/controllers/users_controller_spec.rb
View File

@ -139,24 +139,37 @@ describe UsersController do
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
account_admin_user_with_role_changes(role_changes: {manage_sis: false})
user_session(@user)
course_with_student
get 'delete', :user_id => @student.id
expect(response).to 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
account_admin_user_with_role_changes(role_changes: {manage_sis: false})
user_session(@admin)
course_with_student
managed_pseudonym @student, account: @course.account
get 'delete', :user_id => @student.id
expect(flash[:error]).to match /cannot delete a system-generated user/
expect(response).to redirect_to(user_profile_url(@student))
assert_unauthorized
end
it "should not fail when the only managed pseudonyms are in another account" do
account_admin_user_with_role_changes(role_changes: {manage_sis: false})
user_session(@user)
course_with_student
pseudonym @student, account: @course.account
managed_pseudonym @student, account: account_model
get 'delete', :user_id => @student.id
expect(response).to be_success
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
managed_pseudonym @student, account: @course.account
get 'delete', :user_id => @student.id
expect(flash[:error]).to be_nil
expect(response).to be_success
@ -179,24 +192,28 @@ describe UsersController do
PseudonymSession.find(1).stubs(:destroy).returns(nil)
post 'destroy', :id => @student.id
assert_status(401)
expect(@student.reload.workflow_state).not_to eq 'deleted'
expect(@student.associated_accounts.map(&:id)).to include(@course.account_id)
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
account_admin_user_with_role_changes(role_changes: {manage_sis: false})
user_session(@admin)
course_with_student
PseudonymSession.find(1).stubs(:destroy).returns(nil)
post 'destroy', :id => @student.id
expect(response).to redirect_to(root_url)
expect(@student.reload.workflow_state).to eq 'deleted'
expect(response).to redirect_to(users_url)
expect(@student.associated_accounts.map(&:id)).to_not include(@course.account_id)
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
account_admin_user_with_role_changes(role_changes: {manage_sis: false})
user_session(@admin)
course_with_student
managed_pseudonym @student, account: @course.account
PseudonymSession.find(1).stubs(:destroy).returns(nil)
post 'destroy', :id => @student.id
assert_status(401)
expect(@student.reload.workflow_state).not_to eq 'deleted'
assert_unauthorized
expect(@student.associated_accounts.map(&:id)).to include(@course.account_id)
end
it "should fail when the user has a SIS ID and uses canvas authentication" do
@ -205,18 +222,29 @@ describe UsersController do
@student.pseudonym.update_attribute :sis_user_id, 'kzarn'
post 'destroy', id: @student.id
assert_status(401)
expect(@student.reload.workflow_state).not_to eq 'deleted'
expect(@student.associated_accounts.map(&:id)).to include(@course.account_id)
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
managed_pseudonym @student, account: @course.account
PseudonymSession.find(1).stubs(:destroy).returns(nil)
post 'destroy', :id => @student.id
expect(response).to redirect_to(users_url)
expect(@student.reload.workflow_state).to eq 'deleted'
expect(@student.associated_accounts.map(&:id)).to_not include(@course.account_id)
end
it "should succeed when the system-generated pseudonyms are on another account" do
account_admin_user_with_role_changes(role_changes: {managed_sis: false})
user_session(@admin)
course_with_student
managed_pseudonym @student, account: account_model
PseudonymSession.find(1).stubs(:destroy).returns(nil)
post 'destroy', :id => @student.id
expect(response).to redirect_to(users_url)
expect(@student.associated_accounts.map(&:id)).to_not include(@course.account_id)
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
@ -226,15 +254,29 @@ describe UsersController do
PseudonymSession.find(1).expects(:destroy).returns(nil)
post 'destroy', :id => @admin.id
expect(response).to redirect_to(root_url)
expect(@admin.reload.workflow_state).to eq 'deleted'
expect(@admin.associated_accounts.map(&:id)).to_not include(Account.default.id)
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
account_admin_user(user: @user)
pseudonym(@admin, account: Account.default)
user_session(@admin, @pseudonym)
PseudonymSession.find(1).expects(:destroy).returns(nil)
post 'destroy', :id => @student.id
post 'destroy', :id => @admin.id
expect(response).to redirect_to(root_url)
expect(@student.reload.workflow_state).to eq 'deleted'
expect(@admin.associated_accounts(true).map(&:id)).to_not include(Account.default.id)
end
it "should not remove the user from their other root accounts, if any" do
other_account = account_model
account_admin_user
user_session(@admin)
course_with_student account: Account.default
course_with_student account: other_account, user: @student
post 'destroy', :id => @student.id
expect(response).to redirect_to(users_url)
expect(@student.associated_accounts.map(&:id)).to_not include(Account.default.id)
expect(@student.associated_accounts.map(&:id)).to include(other_account.id)
end
end

60
spec/models/user_spec.rb
View File

@ -692,14 +692,6 @@ describe User do
end
end
context "permissions" do
it "should not allow account admin to modify admin privileges of other account admins" do
expect(RoleOverride.readonly_for(Account.default, :manage_role_overrides, admin_role)).to be_truthy
expect(RoleOverride.readonly_for(Account.default, :manage_account_memberships, admin_role)).to be_truthy
expect(RoleOverride.readonly_for(Account.default, :manage_account_settings, admin_role)).to be_truthy
end
end
context "check_courses_right?" do
before :once do
course_with_teacher(:active_all => true)
@ -2510,53 +2502,11 @@ describe User do
end
end
describe '#grants_right?' do
let_once(:subaccount) do
account = Account.create!
account.root_account_id = Account.default.id
account.save!
account
end
let_once(:site_admin) do
user = User.create!
Account.site_admin.account_users.create!(user: user)
Account.default.account_users.create!(user: user)
user
end
let_once(:local_admin) do
user = User.create!
Account.default.account_users.create!(user: user)
subaccount.account_users.create!(user: user)
user
end
let_once(:sub_admin) do
user = User.create!
subaccount.account_users.create!(user: user)
user
end
it 'allows site admins to manage their own logins' do
expect(site_admin.grants_right?(site_admin, :manage_logins)).to be_truthy
end
it 'allows local admins to manage their own logins' do
expect(local_admin.grants_right?(local_admin, :manage_logins)).to be_truthy
end
it 'allows site admins to manage local admins logins' do
expect(local_admin.grants_right?(site_admin, :manage_logins)).to be_truthy
end
it 'forbids local admins from managing site admins logins' do
expect(site_admin.grants_right?(local_admin, :manage_logins)).to be_falsey
end
it 'only considers root accounts when checking subset permissions' do
expect(sub_admin.grants_right?(local_admin, :manage_logins)).to be_truthy
describe 'permissions' do
it "should not allow account admin to modify admin privileges of other account admins" do
expect(RoleOverride.readonly_for(Account.default, :manage_role_overrides, admin_role)).to be_truthy
expect(RoleOverride.readonly_for(Account.default, :manage_account_memberships, admin_role)).to be_truthy
expect(RoleOverride.readonly_for(Account.default, :manage_account_settings, admin_role)).to be_truthy
end
describe ":reset_mfa" do