Skip to content

Commit 94eb14e

Browse files
lukfugllukfugl
committed
fix up adding logins through the UI
api actions use params[:user][:id] and an Account @context, non-api actions use params[:user_id] and and either params[:account_id] or the @domain_root_account. correctly handle both paths. while we're at it, when the API call was added, the json response changed (lost the pseudonym[] wrapper) and the javascript broke. fix that, and fix the html so the javascript can correctly populate the template. test-plan: - log in as a site admin - add a login to a user with a non-default account selected in the dropdown; the login should be created in the correct account - log in as a non-site admin account admin - add a login to a user (no account dropdown); the login should be created in the current domain root account - while adding accounts through the UI, observe proper addition of new login to the logins list - through the API, add a login to a user via a root account; should succeed - through the API, add a login to a user via a non-root account should fail with a 400 Bad Request - through the API, attempt to add a login without specifying any parameters; should return a 404, not a 500 - through the API, attempt to list logins without specifying any parameters; should return a 404, not a 500 Change-Id: Ic28e73a992916b81a18b40244bbb6d919cccf858 Reviewed-on: https://gerrit.instructure.com/8269 Tested-by: Hudson <hudson@instructure.com> Reviewed-by: Zach Pendleton <zachp@instructure.com>
1 parent 990e490 commit 94eb14e

6 files changed

Lines changed: 105 additions & 57 deletions

File tree

‎app/controllers/pseudonyms_controller.rb‎

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ class PseudonymsController < ApplicationController
3030
#
3131
# @argument user[id] The ID of the user to search on.
3232
def index
33-
@user = api_find(User, params[:user][:id])
34-
return unless context_is_root_account?(@context) && authorized_action(@user, @current_user, :read)
33+
return unless get_user && authorized_action(@user, @current_user, :read)
34+
return unless context_is_root_account?
3535
@pseudonyms = Api.paginate(
3636
@user.pseudonyms.scoped(:conditions => { :account_id => @context.id }),
3737
self, api_v1_pseudonyms_path)
@@ -135,24 +135,26 @@ def new
135135
# @argument login[sis_user_id] SIS ID for the login. To set this parameter, the caller must be able to manage SIS permissions on the account.
136136
def create
137137
return unless get_user
138+
return unless @user == @current_user || authorized_action(@user, @current_user, :manage_logins)
138139

139140
if api_request?
140-
params[:pseudonym] = params[:login]
141-
params[:pseudonym][:password_confirmation] = params[:pseudonym][:password]
141+
return unless context_is_root_account?
142+
params[:pseudonym] = params[:login].merge(
143+
:password_confirmation => params[:login][:password],
144+
:account => @context
145+
)
146+
else
147+
account_id = params[:pseudonym].delete(:account_id)
148+
if current_user_is_site_admin?(:manage_user_logins)
149+
params[:pseudonym][:account] = Account.root_accounts.find(account_id)
150+
else
151+
params[:pseudonym][:account] = @domain_root_account
152+
end
142153
end
154+
143155
if !params[:pseudonym][:password]
144156
@existing_pseudonym = @user.pseudonyms.active.select{|p| p.account == Account.default }.first
145157
end
146-
account_id = api_request? ? params.delete(:account_id) : params[:pseudonym].delete(:account_id)
147-
148-
if current_user_is_site_admin?(:manage_user_logins)
149-
params[:pseudonym][:account] = Account.root_accounts.find(account_id)
150-
elsif @context.is_a?(Account) && @context.root_account?
151-
params[:pseudonym][:account] = @context
152-
else
153-
render(:json => { 'message' => 'Must create login on a root account.' }.to_json, :status => :bad_request)
154-
return
155-
end
156158

157159
sis_user_id = params[:pseudonym].delete(:sis_user_id)
158160
@pseudonym = @user.pseudonyms.build(params[:pseudonym])
@@ -189,10 +191,7 @@ def edit
189191

190192
def get_user
191193
if params[:user_id] || api_request?
192-
@user = api_request? ? api_find(User, params[:user][:id]) : User.find(params[:user_id])
193-
if @user != @current_user && !authorized_action(@user, @current_user, :manage_logins)
194-
return false
195-
end
194+
@user = api_request? ? api_find(User, params[:user] && params[:user][:id]) : User.find(params[:user_id])
196195
else
197196
@user = @current_user
198197
end
@@ -202,6 +201,7 @@ def get_user
202201

203202
def update
204203
return unless get_user
204+
return unless @user == @current_user || authorized_action(@user, @current_user, :manage_logins)
205205
@pseudonym = @user.pseudonyms.find(params[:id])
206206
params[:pseudonym].delete :account_id
207207
unless @pseudonym.account.grants_right?(@current_user, session, :manage_user_logins)
@@ -239,6 +239,7 @@ def update
239239

240240
def destroy
241241
return unless get_user
242+
return unless @user == @current_user || authorized_action(@user, @current_user, :manage_logins)
242243
@pseudonym = @user.pseudonyms.find(params[:id])
243244
if @user.pseudonyms.active.length < 2
244245
@pseudonym.errors.add_to_base(t('errors.login_required', "Users must have at least one login"))
@@ -253,8 +254,8 @@ def destroy
253254
end
254255

255256
protected
256-
def context_is_root_account?(context)
257-
if context.root_account?
257+
def context_is_root_account?
258+
if @context.root_account?
258259
true
259260
else
260261
render(:json => { 'message' => 'Action must be called on a root account.' }.to_json, :status => :bad_request)

‎app/views/users/_logins.html.erb‎

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -23,32 +23,30 @@
2323
<%= link_to(pseudonym ? pseudonym.account.name : nbsp, account_url(pseudonym ? pseudonym.account_id : "{{ account_id }}"), :class => 'account_name') %>
2424
</td>
2525
<td class='details'>
26-
<% if pseudonym %>
27-
<table>
28-
<tr>
29-
<th><%= before_label('last_request', 'Last request') %></th>
30-
<td><%= datetime_string(pseudonym.last_request_at) || t('never', "never") %></td>
31-
</tr>
32-
<tr class='login_details' style="<%= hidden if pseudonyms.length > 1 %>">
33-
<th><%= before_label('current_login', 'Current login') %></th>
34-
<td><%= datetime_string(pseudonym.current_login_at) || t('never', "never") %></td>
35-
</tr>
36-
<tr class='login_details' style="<%= hidden if pseudonyms.length > 1 %>">
37-
<th><%= before_label('current_ip', 'Current login IP') %></th>
38-
<td><%= pseudonym.current_login_ip || t('none', "none") %></td>
39-
</tr>
40-
<tr class='login_details' style="<%= hidden if pseudonyms.length > 1 %>">
41-
<th><%= before_label('last_login', 'Last login') %></th>
42-
<td><%= datetime_string(pseudonym.last_login_at) || t('never', "never") %></td>
43-
</tr>
44-
<tr class='login_details' style="<%= hidden if pseudonyms.length > 1 %>">
45-
<th><%= before_label('last_ip', 'Last login IP') %></th>
46-
<td><%= pseudonym.last_login_ip || t('none', "none") %></td>
47-
</tr>
48-
</table>
49-
<% if pseudonyms.length > 1 %>
50-
<a href="#" class="login_details_link"><%= t('more', 'more...') %></a>
51-
<% end %>
26+
<table>
27+
<tr>
28+
<th><%= before_label('last_request', 'Last request') %></th>
29+
<td><%= datetime_string(pseudonym.try(:last_request_at)) || t('never', "never") %></td>
30+
</tr>
31+
<tr class='login_details' style="<%= hidden if pseudonyms.length > 1 || !pseudonym %>">
32+
<th><%= before_label('current_login', 'Current login') %></th>
33+
<td><%= datetime_string(pseudonym.try(:current_login_at)) || t('never', "never") %></td>
34+
</tr>
35+
<tr class='login_details' style="<%= hidden if pseudonyms.length > 1 || !pseudonym %>">
36+
<th><%= before_label('current_ip', 'Current login IP') %></th>
37+
<td><%= pseudonym.try(:current_login_ip) || t('none', "none") %></td>
38+
</tr>
39+
<tr class='login_details' style="<%= hidden if pseudonyms.length > 1 || !pseudonym %>">
40+
<th><%= before_label('last_login', 'Last login') %></th>
41+
<td><%= datetime_string(pseudonym.try(:last_login_at)) || t('never', "never") %></td>
42+
</tr>
43+
<tr class='login_details' style="<%= hidden if pseudonyms.length > 1 || !pseudonym %>">
44+
<th><%= before_label('last_ip', 'Last login IP') %></th>
45+
<td><%= pseudonym.try(:last_login_ip) || t('none', "none") %></td>
46+
</tr>
47+
</table>
48+
<% if pseudonyms.length > 1 || !pseudonym %>
49+
<a href="#" class="login_details_link"><%= t('more', 'more...') %></a>
5250
<% end %>
5351
</td>
5452
<% if !pseudonym || can_do(pseudonym.account, @current_user, :manage_user_logins) %>
@@ -90,6 +88,10 @@
9088
<span id="passwordable_account_ids" style="display: none;"><%= Account.root_accounts.select{|a| a.settings && a.settings[:admins_can_change_passwords]}.map(&:id).join(',') %></span>
9189
</td>
9290
</tr>
91+
<% else %>
92+
<tr style="display: none;">
93+
<td class="default_account_name"><%= @domain_root_account.name %></td>
94+
</tr>
9395
<% end %>
9496
<tr class="password">
9597
<td><%= f.blabel :password, :en => "Password" %></td>

‎config/routes.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ def add_conferences(context)
494494
association.invite_assessor "invite", :controller => "rubric_assessments", :action => "invite"
495495
association.resources :rubric_assessments, :as => 'assessments'
496496
end
497-
user.resources :pseudonyms
497+
user.resources :pseudonyms, :except => %w(index)
498498
user.resources :question_banks, :only => [:index]
499499
user.assignments_needing_grading 'assignments_needing_grading', :controller => 'users', :action => 'assignments_needing_grading'
500500
user.assignments_needing_submitting 'assignments_needing_submitting', :controller => 'users', :action => 'assignments_needing_submitting'

‎public/javascripts/user_logins.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,13 @@ $(document).ready(function() {
4848
} else {
4949
var $login = $("#login_information .login.blank").clone(true);
5050
$("#login_information .add_holder").before($login.show());
51-
data.pseudonym.account_name = $(this).data('account_name') || $(this).find(".default_account_name").text();
51+
data.account_name = $(this).data('account_name') || $(this).find(".default_account_name").text();
5252
}
5353
$login.fillTemplateData({
54-
data: data.pseudonym,
54+
data: data,
5555
hrefValues: ['id', 'account_id']
5656
});
57-
if($.inArray(data.pseudonym.account_id.toString(), passwordable_account_ids) != -1) {
57+
if($.inArray(data.account_id.toString(), passwordable_account_ids) != -1) {
5858
$login.find(".links").addClass('passwordable');
5959
}
6060
$("#login_information .login .delete_pseudonym_link").show();

‎spec/apis/v1/pseudonyms_api_spec.rb‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@
8787
@path = "/api/v1/accounts/#{@account.id}/logins"
8888
@path_options = { :controller => 'pseudonyms', :action => 'create', :format => 'json', :account_id => @account.id.to_param }
8989
end
90+
9091
context "an authorized user" do
9192
it "should create a new pseudonym" do
9293
json = api_call(:post, @path, @path_options, {
@@ -105,6 +106,22 @@
105106
'user_id' => @student.id
106107
}
107108
end
109+
110+
it "should return 400 if account_id is not a root account" do
111+
@subaccount = Account.create!(:parent_account => @account)
112+
@path = "/api/v1/accounts/#{@subaccount.id}/logins"
113+
@path_options = { :controller => 'pseudonyms', :action => 'create', :format => 'json', :account_id => @subaccount.id.to_param }
114+
raw_api_call(:post, @path, @path_options, {
115+
:user => { :id => @student.id },
116+
:login => {
117+
:password => 'abc123',
118+
:sis_user_id => '12345',
119+
:unique_id => 'duplicate@example.com'
120+
}
121+
})
122+
response.code.should eql '400'
123+
end
124+
108125
it "should return 400 on duplicate pseudonyms" do
109126
@student.pseudonyms.create(:unique_id => 'duplicate@example.com')
110127
raw_api_call(:post, @path, @path_options, {

‎spec/controllers/pseudonyms_controller_spec.rb‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -179,15 +179,43 @@
179179
end
180180

181181
describe "create" do
182-
before :each do
183-
user_with_pseudonym(:active_all => true)
184-
Account.site_admin.add_user(@user)
185-
user_session(@user, @pseudonym)
182+
# these specs only test the non-api version of the calls
183+
context "with site admin permissions" do
184+
before :each do
185+
user_with_pseudonym(:active_all => true)
186+
Account.site_admin.add_user(@user)
187+
user_session(@user, @pseudonym)
188+
end
189+
190+
it "should use the account id from params" do
191+
post 'create', :format => 'json', :user_id => @user.id, :pseudonym => { :account_id => Account.site_admin.id, :unique_id => 'unique1' }
192+
response.should be_success
193+
end
186194
end
187195

188-
it "should work with a user as context and account in params" do
189-
post 'create', :format => 'json', :user_id => @user.id, :pseudonym => { :account_id => Account.site_admin.id, :unique_id => 'unique1' }
190-
response.should be_success
196+
context "without site admin permissions" do
197+
before :each do
198+
@account = Account.create!
199+
user_with_pseudonym(:active_all => true, :account => @account)
200+
LoadAccount.stubs(:default_domain_root_account).returns(@account)
201+
@account.add_user(@user)
202+
user_session(@user, @pseudonym)
203+
end
204+
205+
it "should ignore use the domain_root_account" do
206+
post 'create', :format => 'json', :user_id => @user.id, :pseudonym => { :unique_id => 'unique1' }
207+
response.should be_success
208+
@user.pseudonyms.size.should == 2
209+
(@user.pseudonyms - [@pseudonym]).last.account.should == @account
210+
end
211+
212+
it "should ignore account id in params and use the domain_root_account" do
213+
@account2 = Account.create!
214+
post 'create', :format => 'json', :user_id => @user.id, :pseudonym => { :account_id => @account2.id, :unique_id => 'unique1' }
215+
response.should be_success
216+
@user.pseudonyms.size.should == 2
217+
(@user.pseudonyms - [@pseudonym]).last.account.should == @account
218+
end
191219
end
192220
end
193221

0 commit comments

Comments
 (0)