Merge pull request #22907 from code-dot-org/powerschool-untrusted-email

Powerschool: use Clever takeover flow instead of auto-takeover

dashboard/app/controllers/home_controller.rb

@@ -61,7 +61,7 @@ def index
         redirect_to '/home'
       end
     else
-      clear_clever_session_variables
+      clear_takeover_session_variables
       redirect_to '/courses'
     end
   end

dashboard/app/controllers/omniauth_callbacks_controller.rb

@@ -27,11 +27,11 @@ def all
       # Redirect to open roster dialog on home page if user just authorized access
       # to Google Classroom courses and rosters
       redirect_to '/home?open=rosterDialog'
-    elsif @user.provider == 'clever' && @user.persisted?
-      handle_clever_signin(@user)
+    elsif User::OAUTH_PROVIDERS_UNTRUSTED_EMAIL.include?(@user.provider) && @user.persisted?
+      handle_untrusted_email_signin(@user)
     elsif @user.persisted?
       # If email is already taken, persisted? will be false because of a validation failure
-      check_and_apply_clever_takeover(@user)
+      check_and_apply_oauth_takeover(@user)
       sign_in_user
     elsif allows_silent_takeover(@user)
       silent_takeover(@user)
@@ -84,17 +84,17 @@ def extract_powerschool_data(auth)
     auth
   end
 
-  # Clever signins have unique requirements, and must be handled a bit outside the normal flow
-  def handle_clever_signin(user)
-    force_takeover = user.teacher? && user.email.present? && user.email.end_with?('.cleveremailalreadytaken')
+  # Clever/Powerschool signins have unique requirements, and must be handled a bit outside the normal flow
+  def handle_untrusted_email_signin(user)
+    force_takeover = user.teacher? && user.email.present? && user.email.end_with?('.oauthemailalreadytaken')
 
     # If account exists (as looked up by Clever ID) and it's not the first login, just sign in
     if user.persisted? && user.sign_in_count > 0 && !force_takeover
       sign_in_user
     else
       # Otherwise, it's either the first login, or a user who must connect -
       # offer to connect the Clever account to an existing one, or insist if needed
-      session['clever_link_flag'] = true
+      session['clever_link_flag'] = user.provider
       session['clever_takeover_id'] = user.uid
       session['clever_takeover_token'] = user.oauth_token
       session['force_clever_takeover'] = force_takeover
@@ -140,7 +140,7 @@ def sign_in_user
 
   def allows_silent_takeover(oauth_user)
     allow_takeover = oauth_user.provider.present?
-    allow_takeover &= %w(facebook google_oauth2 windowslive powerschool).include?(oauth_user.provider)
+    allow_takeover &= %w(facebook google_oauth2 windowslive).include?(oauth_user.provider)
     # allow_takeover &= oauth_user.email_verified # TODO (eric) - set up and test for different providers
     lookup_user = User.find_by_email_or_hashed_email(oauth_user.email)
     allow_takeover && lookup_user

dashboard/app/controllers/sections_controller.rb

@@ -48,7 +48,7 @@ def log_in
       bypass_sign_in user
       user.update_tracked_fields!(request)
       session[:show_pairing_dialog] = true if params[:show_pairing_dialog]
-      check_and_apply_clever_takeover(user)
+      check_and_apply_oauth_takeover(user)
       redirect_to_section_script_or_course
     else
       flash[:alert] = I18n.t('signinsection.invalid_login')

dashboard/app/controllers/sessions_controller.rb

@@ -34,14 +34,14 @@ def clever_takeover
   end
 
   def clever_modal_dismissed
-    clear_clever_session_variables
+    clear_takeover_session_variables
     render status: 200, nothing: true
   end
 
   # POST /resource/sign_in
   def create
     super do |user|
-      check_and_apply_clever_takeover(user)
+      check_and_apply_oauth_takeover(user)
     end
   end
 

dashboard/app/helpers/users_helper.rb

@@ -7,29 +7,29 @@ module UsersHelper
 
   # If Clever takeover flags are present, the current account (user) is the one that the person just
   # logged into (to prove ownership), and all the Clever details are migrated over, including sections.
-  def check_and_apply_clever_takeover(user)
-    if session['clever_link_flag'].present? && session['clever_takeover_id'].present? && session['clever_takeover_token'].present?
+  def check_and_apply_oauth_takeover(user)
+    if session['clever_link_flag'].present? && session['clever_takeover_id'].present?
       uid = session['clever_takeover_id']
       # TODO: validate that we're not destroying an active account?
-      existing_clever_account = User.where(uid: uid, provider: 'clever').first
+      existing_account = User.where(uid: uid, provider: session['clever_link_flag']).first
 
       # Move over sections that students follow
-      if user.student? && existing_clever_account
-        Follower.where(student_user_id: existing_clever_account.id).each do |follower|
+      if user.student? && existing_account
+        Follower.where(student_user_id: existing_account.id).each do |follower|
           follower.update(student_user_id: user.id)
         end
       end
 
-      existing_clever_account.destroy! if existing_clever_account
-      user.provider = 'clever'
+      existing_account.destroy! if existing_account
+      user.provider = session['clever_link_flag']
       user.uid = uid
       user.oauth_token = session['clever_takeover_token']
       user.save
-      clear_clever_session_variables
+      clear_takeover_session_variables
     end
   end
 
-  def clear_clever_session_variables
+  def clear_takeover_session_variables
     return if session.empty?
     session.delete('clever_link_flag')
     session.delete('clever_takeover_id')

dashboard/app/models/user.rb

@@ -159,6 +159,11 @@ class User < ActiveRecord::Base
     powerschool
   ).freeze
 
+  OAUTH_PROVIDERS_UNTRUSTED_EMAIL = %w(
+    clever
+    powerschool
+  ).freeze
+
   SYSTEM_DELETED_USERNAME = 'sys_deleted'
 
   # constants for resetting user secret words/picture
@@ -715,10 +720,10 @@ def self.from_omniauth(auth, params)
       user.name = name_from_omniauth auth.info.name
       user.user_type = params['user_type'] || auth.info.user_type
       # Store emails, except when using Clever
-      user.email = auth.info.email unless user.user_type == 'student' && auth.provider == 'clever'
+      user.email = auth.info.email unless user.user_type == 'student' && OAUTH_PROVIDERS_UNTRUSTED_EMAIL.include?(auth.provider)
 
-      if auth.provider == 'clever' && User.find_by_email_or_hashed_email(user.email)
-        user.email = user.email + '.cleveremailalreadytaken'
+      if OAUTH_PROVIDERS_UNTRUSTED_EMAIL.include?(auth.provider) && User.find_by_email_or_hashed_email(user.email)
+        user.email = user.email + '.oauthemailalreadytaken'
       end
 
       if auth.provider == :the_school_project

dashboard/test/controllers/omniauth_callbacks_controller_test.rb

@@ -336,25 +336,27 @@ class OmniauthCallbacksControllerTest < ActionController::TestCase
     end
   end
 
-  test 'clever takeover transfers sections to taken over account' do
-    teacher = create :teacher
-    section = create :section, user: teacher, login_type: 'clever'
-    clever_student = create :student, provider: 'clever', uid: '12345'
-    student = create :student
-
-    clever_students = [clever_student]
-    section.set_exact_student_list(clever_students)
-
-    # Pull sections_as_student from the database and store them in an array to compare later
-    sections_as_student = clever_student.sections_as_student.to_ary
-
-    @request.cookies[:pm] = 'clever_takeover'
-    @request.session['clever_link_flag'] = true
-    @request.session['clever_takeover_id'] = clever_student.uid
-    @request.session['clever_takeover_token'] = '54321'
-    check_and_apply_clever_takeover(student)
-
-    assert_equal sections_as_student, student.sections_as_student
+  test 'oauth takeover transfers sections to taken over account' do
+    User::OAUTH_PROVIDERS_UNTRUSTED_EMAIL.each do |provider|
+      teacher = create :teacher
+      section = create :section, user: teacher, login_type: 'clever'
+      oauth_student = create :student, provider: provider, uid: '12345'
+      student = create :student
+
+      oauth_students = [oauth_student]
+      section.set_exact_student_list(oauth_students)
+
+      # Pull sections_as_student from the database and store them in an array to compare later
+      sections_as_student = oauth_student.sections_as_student.to_ary
+
+      @request.cookies[:pm] = 'clever_takeover'
+      @request.session['clever_link_flag'] = provider
+      @request.session['clever_takeover_id'] = oauth_student.uid
+      @request.session['clever_takeover_token'] = '54321'
+      check_and_apply_oauth_takeover(student)
+
+      assert_equal sections_as_student, student.sections_as_student
+    end
   end
 
   def generate_auth_user_hash(email, user_type)