don't allow setting sis id on root accounts

testplan: try to set an sis id via the UI, a raw request, or the console

Change-Id: I9749031729bf7d94070f4fd67c0a2f595ce337ed
Reviewed-on: https://gerrit.instructure.com/7007
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>

Author: ccutrer

app/controllers/accounts_controller.rb

@@ -98,7 +98,7 @@ def update
           end
         end
         if sis_id = params[:account].delete(:sis_source_id)
-          if sis_id != @account.sis_source_id && (@account.root_account || @account).grants_right?(@current_user, session, :manage_sis)
+          if !@account.root_account? && sis_id != @account.sis_source_id && (@account.root_account || @account).grants_right?(@current_user, session, :manage_sis)
             if sis_id == ''
               @account.sis_source_id = nil
             else

app/models/account.rb

@@ -198,15 +198,16 @@ def ensure_defaults
 
   def verify_unique_sis_source_id
     return true unless self.sis_source_id
-    root = self.root_account || self
-    existing_account = Account.find_by_root_account_id_and_sis_source_id(root.id, self.sis_source_id)
-    
     if self.root_account?
-      return true if !existing_account
-    elsif root.sis_source_id != self.sis_source_id
-      return true if !existing_account || existing_account.id == self.id
+      self.errors.add(:sis_source_id, t('#account.root_account_cant_have_sis_id', "SIS IDs cannot be set on root accounts"))
+      return false
     end
+
+    root = self.root_account
+    existing_account = Account.find_by_root_account_id_and_sis_source_id(root.id, self.sis_source_id)
 
+    return true if !existing_account || existing_account.id == self.id
+
     self.errors.add(:sis_source_id, t('#account.sis_id_in_use', "SIS ID \"%{sis_id}\" is already in use", :sis_id => self.sis_source_id))
     false
   end

app/views/accounts/settings.html.erb

@@ -53,7 +53,7 @@
               <td><%= f.blabel :name, :en => "Account Name" %></td>
               <td><%= f.text_field :name %></td>
             </tr>
-            <% if (@context.sis_source_id || can_do(@context.root_account || @context, @current_user, :manage_sis)) && !@account.site_admin? %>
+            <% if !@account.root_account? && (@context.sis_source_id || can_do(@context.root_account || @context, @current_user, :manage_sis)) %>
               <tr>
                 <td><%= f.blabel :sis_source_id, :en => "SIS ID" %></td>
                 <td>

spec/apis/v1/accounts_api_spec.rb

@@ -23,11 +23,11 @@
     user_with_pseudonym(:active_all => true)
     @a1 = account_model(:name => 'root')
     @a1.add_user(@user)
-    @a2 = account_model(:name => 'subby', :parent_account => @a1, :sis_source_id => 'sis1')
+    @a2 = account_model(:name => 'subby', :parent_account => @a1, :root_account => @a1, :sis_source_id => 'sis1')
     @a2.add_user(@user)
     @a3 = account_model(:name => 'no-access')
     # even if we have access to it implicitly, it's not listed
-    @a4 = account_model(:name => 'implicit-access', :parent_account => @a1)
+    @a4 = account_model(:name => 'implicit-access', :parent_account => @a1, :root_account => @a1)
   end
 
   it "should return the account list" do
@@ -67,7 +67,7 @@
 
   it "should find accounts by sis in only this root account" do
     Account.default.add_user(@user)
-    other_sub = account_model(:name => 'other_sub', :parent_account => Account.default, :sis_source_id => 'sis1')
+    other_sub = account_model(:name => 'other_sub', :parent_account => Account.default, :root_account => Account.default, :sis_source_id => 'sis1')
     other_sub.add_user(@user)
 
     # this is scoped to Account.default

spec/controllers/accounts_controller_spec.rb

@@ -141,4 +141,21 @@ def cross_listed_course
 
     assigns[:associated_courses_count].should == 1
   end
+
+  describe "update" do
+    it "should allow admins to set the sis_source_id on sub accounts" do
+      account_with_admin_logged_in
+      @account = @account.sub_accounts.create!
+      post 'update', :id => @account.id, :account => { :sis_source_id => 'abc' }
+      @account.reload
+      @account.sis_source_id.should == 'abc'
+    end
+
+    it "should not allow setting the sis_source_id on root accounts" do
+      account_with_admin_logged_in
+      post 'update', :id => @account.id, :account => { :sis_source_id => 'abc' }
+      @account.reload
+      @account.sis_source_id.should be_nil
+    end
+  end
 end

spec/models/account_spec.rb

@@ -597,6 +597,12 @@ def account_with_admin_and_restricted_user(account)
     end
   end
 
+  it "should not allow setting an sis id for a root account" do
+    @account = Account.create!
+    @account.sis_source_id = 'abc'
+    @account.save.should be_false
+  end
+
   describe "open_registration_for?" do
     it "should be true for anyone if open registration is turned on" do
       account = Account.default

spec/views/accounts/settings.html.erb_spec.rb

@@ -22,7 +22,7 @@
 describe "accounts/settings.html.erb" do
   describe "sis_source_id edit box" do
     before do
-      @account = Account.default
+      @account = Account.default.sub_accounts.create!
       @account.sis_source_id = "so_special_sis_id"
       @account.save