fix: prevent account takeover by splitting user lookup paths

Address security vulnerability C4 by replacing the single user lookup
with two distinct paths. The canonical production path now uses
canvas_uid as the source of truth and refuses to link accounts when
an email conflict is detected, preventing unauthorized takeovers.

The legacy email-matching behavior is preserved in a separate developer
path, which is strictly gated to local environments for safe testing
and masquerading.

- Implement canvas_lookup_or_create with strict UID matching
- Implement developer_lookup_or_create for local impersonation
- Add double-gated environment checks for developer logins
- Add comprehensive test coverage for takeover refusal scenarios

Co-authored-by: Claude Code <noreply@anthropic.com>

Author: 2 people

app/controllers/session_controller.rb

@@ -65,14 +65,39 @@ def omniauth_callback
       expires_at: expires_at
     )
 
-    # Persist / update the user just like `create`
-    user = find_or_create_user(user_data, access_token)
+    # Choose the account-resolution path based on the provider.
+    #
+    # The :developer provider is a deliberate masquerade hole that lets a
+    # developer log in *as* an existing user without going through Canvas. It
+    # must never be reachable in production, so we gate it twice: the provider
+    # is only registered in dev/test (config/initializers/omniauth.rb) AND we
+    # re-check developer_login_allowed? here as defense in depth.
+    developer = auth.provider == 'developer'
+    if developer && !developer_login_allowed?
+      Rails.logger.error('Developer login attempted in a non-permitted environment')
+      redirect_to root_path, alert: 'Authentication failed. Please try again.'
+      return
+    end
 
-    # Auto-enroll developer login users in test courses
-    if auth.provider == 'developer'
-      ensure_developer_test_enrollments(user)
+    user =
+      if developer
+        developer_lookup_or_create(user_data, access_token)
+      else
+        canvas_lookup_or_create(user_data, access_token)
+      end
+
+    # canvas_lookup_or_create returns nil when it refuses to silently re-key an
+    # existing account to a new canvas_uid (potential account takeover).
+    if user.nil?
+      redirect_to root_path,
+                  alert: 'We could not link your Canvas account to an existing ' \
+                         'account with the same email. Please contact an administrator.'
+      return
     end
 
+    # Auto-enroll developer login users in test courses
+    ensure_developer_test_enrollments(user) if developer
+
     redirect_to courses_path, notice: "Logged in! Welcome, #{user_data['name']}!"
   rescue StandardError => e
     Rails.logger.error("OmniAuth callback error: #{e.message}")
@@ -101,10 +126,64 @@ def ensure_developer_test_enrollments(user)
     end
   end
 
-  # TODO: Refactor.
-  def find_or_create_user(user_data, auth_token)
-    auth_token.token
-    user = nil
+  # Whether the developer (masquerade) login path may be used.
+  #
+  # This mirrors the env guard that registers the :developer OmniAuth provider
+  # in config/initializers/omniauth.rb -- both must be true for developer login
+  # to be reachable. Keeping the check here (and not only in the initializer)
+  # ensures the impersonation path can never run in production even if a
+  # :developer callback somehow reaches the controller.
+  def developer_login_allowed?
+    # Rails.env.local? is true only in development and test, matching the env
+    # guard around the :developer provider in config/initializers/omniauth.rb.
+    Rails.env.local?
+  end
+
+  # Canonical production login path.
+  #
+  # Canvas is the source of truth for identity, so a user is keyed by their
+  # canvas_uid:
+  #   * If we recognize the canvas_uid, we refresh the email from Canvas.
+  #   * If we DON'T recognize the canvas_uid but the incoming email already
+  #     belongs to another account, we REFUSE to re-key that account to the new
+  #     canvas_uid and return nil. Silently re-keying would let anyone able to
+  #     create a Canvas account with a victim's email take over the victim's
+  #     account here.
+  #   * Otherwise we create a brand new account.
+  def canvas_lookup_or_create(user_data, auth_token)
+    user = User.find_by(canvas_uid: user_data['id'])
+
+    if user
+      user.email = user_data['email']
+    elsif User.exists?(email: user_data['primary_email'])
+      # A different canvas_uid is presenting an email we already know. Refuse
+      # the silent account re-key (potential takeover) and let the caller
+      # surface an error to the user.
+      Rails.logger.warn(
+        "Refusing to link canvas_uid=#{user_data['id']} to existing account " \
+        "with email=#{user_data['primary_email']}"
+      )
+      return nil
+    else
+      user = User.new(canvas_uid: user_data['id'])
+      user.assign_attributes(
+        email: user_data['email'],
+        name: user_data['name']
+      )
+    end
+
+    persist_login!(user, auth_token)
+  end
+
+  # Developer / test masquerade path.
+  #
+  # This INTENTIONALLY preserves the legacy email-first matching so a developer
+  # can log in *as* an existing user without going through Canvas. It is a
+  # deliberate impersonation hole and is only ever reached when
+  # developer_login_allowed? is true (see omniauth_callback and omniauth.rb).
+  # Do NOT use this logic for the production Canvas path -- see
+  # canvas_lookup_or_create for why email-first matching is unsafe there.
+  def developer_lookup_or_create(user_data, auth_token)
     if User.exists?(email: user_data['primary_email'])
       user = User.find_by(email: user_data['primary_email'])
       user.canvas_uid = user_data['id']
@@ -118,6 +197,13 @@ def find_or_create_user(user_data, auth_token)
         name: user_data['name']
       )
     end
+
+    persist_login!(user, auth_token)
+  end
+
+  # Shared finalization for both login paths: persist the user, refresh their
+  # LMS credentials, and establish the session.
+  def persist_login!(user, auth_token)
     user.save!
     update_user_credential(user, auth_token)
 

config/initializers/omniauth.rb

@@ -18,6 +18,11 @@ def build_access_token
   # URL-encode the scopes defined in CanvasFacade
   encoded_scopes = CGI.escape(CanvasFacade::CANVAS_API_SCOPES)
 
+  # SECURITY: The :developer provider enables the masquerade login path in
+  # SessionController (developer_lookup_or_create), which lets a developer log
+  # in as any existing user by email without Canvas. It must NEVER be available
+  # in production. This env guard is one of two gates; the controller also
+  # re-checks developer_login_allowed? before taking that path.
   provider :developer, fields: [:email] if Rails.env.development? || Rails.env.test?
 
   provider :canvas,

spec/controllers/session_controller_spec.rb

@@ -1,6 +1,22 @@
 require 'rails_helper'
 
 RSpec.describe SessionController, type: :controller do
+  let(:user_data) do
+    {
+      'id' => '12345',
+      'name' => 'Test User',
+      'primary_email' => 'test@example.com',
+      'email' => 'test@example.com'
+    }
+  end
+
+  let(:mock_token) do
+    instance_double(OAuth2::AccessToken,
+                    token: 'new-token',
+                    refresh_token: 'new-refresh',
+                    expires_at: 2.hours.from_now.to_i)
+  end
+
   describe 'GET #omniauth_callback' do
     let(:auth_hash) do
       OmniAuth::AuthHash.new(
@@ -34,6 +50,18 @@
       end
     end
 
+    context 'when a different canvas_uid presents an existing account email' do
+      let!(:existing_user) { User.create!(email: 'test@example.com', canvas_uid: 'old_uid') }
+
+      it 'refuses the login and redirects to root with an error flash' do
+        get :omniauth_callback, params: { provider: 'canvas' }
+
+        expect(existing_user.reload.canvas_uid).to eq('old_uid')
+        expect(response).to redirect_to(root_path)
+        expect(flash[:alert]).to include('could not link your Canvas account')
+      end
+    end
+
     context 'when no auth hash is present' do
       before { request.env.delete('omniauth.auth') }
 
@@ -58,29 +86,54 @@
     end
   end
 
-  describe '#find_or_create_user and #update_user_credential' do
-    let(:user_data) do
-      {
-        'id' => '12345',
-        'name' => 'Test User',
-        'primary_email' => 'test@example.com',
-        'email' => 'test@example.com'
-      }
+  describe '#canvas_lookup_or_create and #update_user_credential' do
+    context 'when user exists by canvas_uid' do
+      let!(:existing_user) { User.create!(email: 'old@example.com', canvas_uid: '12345') }
+
+      it 'updates email and LMS credentials' do
+        expect do
+          controller.send(:canvas_lookup_or_create, user_data, mock_token)
+        end.to change { existing_user.reload.email }.from('old@example.com').to('test@example.com')
+
+        creds = existing_user.reload.lms_credentials.first
+        expect(creds.token).to eq('new-token')
+      end
     end
 
-    let(:mock_token) do
-      instance_double(OAuth2::AccessToken,
-                      token: 'new-token',
-                      refresh_token: 'new-refresh',
-                      expires_at: 2.hours.from_now.to_i)
+    context 'when a different canvas_uid presents an already-known email' do
+      let!(:existing_user) { User.create!(email: 'test@example.com', canvas_uid: 'old_uid') }
+
+      it 'refuses to re-key the account, returns nil, and leaves it untouched' do
+        result = nil
+        expect do
+          result = controller.send(:canvas_lookup_or_create, user_data, mock_token)
+        end.not_to(change { existing_user.reload.canvas_uid })
+
+        expect(result).to be_nil
+        expect(existing_user.reload.canvas_uid).to eq('old_uid')
+      end
     end
 
+    context 'when user is new' do
+      it 'creates the user and LMS credentials' do
+        expect do
+          controller.send(:canvas_lookup_or_create, user_data, mock_token)
+        end.to change(User, :count).by(1)
+
+        user = User.find_by(canvas_uid: '12345')
+        expect(user.email).to eq('test@example.com')
+        expect(user.lms_credentials.first.token).to eq('new-token')
+      end
+    end
+  end
+
+  describe '#developer_lookup_or_create (masquerade path)' do
     context 'when user exists by email' do
       let!(:existing_user) { User.create!(email: 'test@example.com', canvas_uid: 'old_uid') }
 
-      it 'updates canvas_uid and LMS credentials' do
+      it 'masquerades by re-keying canvas_uid and updates LMS credentials' do
         expect do
-          controller.send(:find_or_create_user, user_data, mock_token)
+          controller.send(:developer_lookup_or_create, user_data, mock_token)
         end.to change { existing_user.reload.canvas_uid }.from('old_uid').to('12345')
 
         creds = existing_user.reload.lms_credentials.first
@@ -93,15 +146,15 @@
 
       it 'updates email and LMS credentials' do
         expect do
-          controller.send(:find_or_create_user, user_data, mock_token)
+          controller.send(:developer_lookup_or_create, user_data, mock_token)
         end.to change { existing_user.reload.email }.from('old@example.com').to('test@example.com')
       end
     end
 
     context 'when user is new' do
       it 'creates the user and LMS credentials' do
         expect do
-          controller.send(:find_or_create_user, user_data, mock_token)
+          controller.send(:developer_lookup_or_create, user_data, mock_token)
         end.to change(User, :count).by(1)
 
         user = User.find_by(canvas_uid: '12345')
@@ -111,6 +164,17 @@
     end
   end
 
+  describe '#developer_login_allowed?' do
+    it 'is permitted in the test environment' do
+      expect(controller.send(:developer_login_allowed?)).to be(true)
+    end
+
+    it 'is not permitted in production' do
+      allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new('production'))
+      expect(controller.send(:developer_login_allowed?)).to be(false)
+    end
+  end
+
   describe 'GET #omniauth_callback (developer provider)' do
     let(:dev_auth_hash) do
       OmniAuth::AuthHash.new(