From be56c138632eb20b06835a81e75553065e6402f1 Mon Sep 17 00:00:00 2001
From: Jeremy Stanley <jeremy@instructure.com>
Date: Mon, 10 Dec 2012 14:23:29 -0700
Subject: [PATCH] disallow sis enrollment roles as custom role names

to avoid ambiguity, don't allow custom roles named 'student',
'teacher', 'ta', 'observer', or 'designer'

also fix the error message from the api not to assume that
the base role type is invalid

test plan:
 - using the api, try to create a custom role with one of the
   above names
   -> it should tell you the name is reserved
 - also, try to create a custom role with a valid name
   but an invalid base role type
   -> it should tell you the base role type is invalid

Change-Id: Ic7f91a56e79d2929a64780115553ed38aec5f592
Reviewed-on: https://gerrit.instructure.com/15978
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Cody Cutrer <cody@instructure.com>
QA-Review: Adam Phillipps <adam@instructure.com>
---
 app/controllers/role_overrides_controller.rb |  2 +-
 app/models/role.rb                           |  4 ++--
 spec/apis/v1/roles_api_spec.rb               | 10 ++++++++-
 spec/models/role_spec.rb                     | 23 ++++++++++++++++++++
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/app/controllers/role_overrides_controller.rb b/app/controllers/role_overrides_controller.rb
index 9f659821dda..3cf36ee02a8 100644
--- a/app/controllers/role_overrides_controller.rb
+++ b/app/controllers/role_overrides_controller.rb
@@ -242,7 +242,7 @@ class RoleOverridesController < ApplicationController
     role.workflow_state = 'active'
     role.deleted_at = nil
     if !role.save
-      render :json => {:message => "invalid base role type"}, :status => :bad_request
+      render :json => { :message => role.errors.full_messages.to_sentence }, :status => :bad_request
       return
     end
     # remove old role overrides that were associated with this role name
diff --git a/app/models/role.rb b/app/models/role.rb
index 1a205ce08e2..111ff4a6737 100644
--- a/app/models/role.rb
+++ b/app/models/role.rb
@@ -22,8 +22,8 @@ class Role < ActiveRecord::Base
   attr_accessible :name
   before_validation :infer_root_account_id
   validates_presence_of :name
-  validates_inclusion_of :base_role_type, :in => RoleOverride::BASE_ROLE_TYPES
-  validates_exclusion_of :name, :in => RoleOverride::KNOWN_ROLE_TYPES
+  validates_inclusion_of :base_role_type, :in => RoleOverride::BASE_ROLE_TYPES, :message => 'is invalid'
+  validates_exclusion_of :name, :in => RoleOverride::KNOWN_ROLE_TYPES + Enrollment::SIS_TYPES.values
   validates_uniqueness_of :name, :scope => :account_id
   validate :ensure_no_name_conflict_with_different_base_role_type
 
diff --git a/spec/apis/v1/roles_api_spec.rb b/spec/apis/v1/roles_api_spec.rb
index 2fb960dadab..8d5df921fa4 100644
--- a/spec/apis/v1/roles_api_spec.rb
+++ b/spec/apis/v1/roles_api_spec.rb
@@ -240,7 +240,15 @@ describe "Roles API", :type => :integration do
         { :controller => 'role_overrides', :action => 'add_role', :format => 'json', :account_id => @admin.account.id.to_s },
         { :role => @role, :base_role_type => "notagoodbaserole" })
       response.status.should == '400 Bad Request'
-      JSON.parse(response.body).should == {"message" => "invalid base role type"}
+      JSON.parse(response.body).should == {"message" => "Base role type is invalid"}
+    end
+
+    it "should fail for a course role with a reserved name" do
+      raw_api_call(:post, "/api/v1/accounts/#{@admin.account.id}/roles",
+                   { :controller => 'role_overrides', :action => 'add_role', :format => 'json', :account_id => @admin.account.id.to_s },
+                   { :role => 'student', :base_role_type => "StudentEnrollment" })
+      response.status.should == '400 Bad Request'
+      JSON.parse(response.body).should == {"message" => "Name is reserved"}
     end
 
     it "should not create an override for course role for account-only permissions" do
diff --git a/spec/models/role_spec.rb b/spec/models/role_spec.rb
index c43c3eec29e..ea2d18be8ff 100644
--- a/spec/models/role_spec.rb
+++ b/spec/models/role_spec.rb
@@ -98,6 +98,29 @@ describe Role do
       role.should be_valid
     end
 
+    it "should disallow names that match base sis enrollment role names" do
+      role = @account.roles.create
+      role.base_role_type = 'StudentEnrollment'
+
+      role.name = 'student'
+      role.should_not be_valid
+
+      role.name = 'teacher'
+      role.should_not be_valid
+
+      role.name = 'ta'
+      role.should_not be_valid
+
+      role.name = 'designer'
+      role.should_not be_valid
+
+      role.name = 'observer'
+      role.should_not be_valid
+
+      role.name = 'cheater'
+      role.should be_valid
+    end
+
     it "should infer the root account id" do
       role = create_role('StudentEnrollment', "1337 Student")
       role.root_account_id.should == @account.id