Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LTI account linking controller and service #58464

Merged
merged 11 commits into from
May 9, 2024
34 changes: 34 additions & 0 deletions dashboard/app/controllers/lti/v1/account_linking_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
module Lti
module V1
class AccountLinkingController < ApplicationController
before_action :lti_account_linking_enabled?

# GET /lti/v1/account_linking/landing
def landing
end

# GET /lti/v1/account_linking/existing_account
def existing_account
@user = User.new_with_session(ActionController::Parameters.new, session)
end

# POST /lti/v1/account_linking/link_email
def link_email
head :bad_request unless PartialRegistration.in_progress?(session)
params.require([:email, :password])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these params are missing, it will throw an exception. Do we care about handling that exception and letting the user know they have a missing param, or are we ok with just returning :unauthorized in the else block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we'll definitely want to notify the user, but I was thinking we can be added later when we finalize the UI page that will post to this route. The front-end should be able to handle this with the usual "X field is required" messaging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also need to gracefully handle unauthorized responses for incorrect passwords

existing_user = User.find_by_email_or_hashed_email(params[:email])
if existing_user&.valid_password?(params[:password])
Services::Lti::AccountLinker.call(user: existing_user, session: session)
sign_in existing_user
redirect_to home_path
else
head :unauthorized
end
end

private def lti_account_linking_enabled?
head :not_found unless DCDO.get('lti_account_linking_enabled', false)
end
end
end
end
3 changes: 3 additions & 0 deletions dashboard/app/controllers/lti_v1_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,9 @@ def authenticate
user = Services::Lti.initialize_lti_user(decoded_jwt)
PartialRegistration.persist_attributes(session, user)
session[:user_return_to] = destination_url
if DCDO.get('lti_account_linking_enabled', false)
redirect_to lti_v1_account_linking_landing_path and return
end
redirect_to new_user_registration_url
end
else
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
!!!
%html{lang: "en"}
%body
%h1 Connect Existing Account

= form_with(url: lti_v1_account_linking_link_email_path, html: {class: "signup"}) do |f|
/ Email
.row
.span3.signup-field-label
= f.label :email do
= succeed '&nbsp;*'.html_safe do
= User.human_attribute_name(:email)
- if @user.errors[:email].present?
%p.error= @user.errors[:email]&.first
.span4
= f.email_field :email, maxlength: 255
carl-codeorg marked this conversation as resolved.
Show resolved Hide resolved

/ Password
.row
.span3.signup-field-label
= f.label :password
- if @user.errors[:password].present?
%p.error= @user.errors[:password]&.first
.span4
= f.password_field :password, maxlength: 255
carl-codeorg marked this conversation as resolved.
Show resolved Hide resolved

/ Submit
.row
.span8
%button.submit{:id => 'connect_form_submit'}= 'Connect Existing Account'
7 changes: 7 additions & 0 deletions dashboard/app/views/lti/v1/account_linking/landing.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
!!!
%html{lang: "en"}
%body
%h1 Welcome to Code.org!
%p Looks like this is your first time logging in from Canvas. Seclect one of the options below to continue.
%a.btn.btn-primary{href: new_user_registration_url} I am new to Code.org
%a.btn.btn-primary{href: lti_v1_account_linking_existing_account_path} I have an existing Code.org account
5 changes: 5 additions & 0 deletions dashboard/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,11 @@
patch :bulk_update_owners
end
end
namespace :account_linking do
get :landing
get :existing_account
post :link_email
end
end
end

Expand Down
25 changes: 25 additions & 0 deletions dashboard/lib/services/lti/account_linker.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
module Services
module Lti
class AccountLinker < Services::Base
attr_reader :user, :session

def initialize(user:, session:)
@user = user
@session = session
end

def call
ActiveRecord::Base.transaction do
user.authentication_options << rehydrated_user.authentication_options.first
Services::Lti.create_lti_user_identity(user)
user.lti_roster_sync_enabled = true if user.teacher?
PartialRegistration.delete(session)
end
end

private def rehydrated_user
@rehydrated_user ||= ::User.new_with_session(ActionController::Parameters.new, session)
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
require 'test_helper'

class Lti::V1::AccountLinkingControllerTest < ActionController::TestCase
setup do
@user = create(:teacher, email: 'test@lti.com')
@lti_integration = create :lti_integration
end

setup do
Copy link
Contributor

@nicklathe nicklathe May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the two setup blocks a Rails best practices thing? One set's variables and one sets up stubs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, no I think I accidentally added a second block when I was debugging some DCDO-related errors. Will update this

DCDO.stubs(:get)
DCDO.stubs(:get).with('lti_account_linking_enabled', false).returns(true)
end

test 'links an LTI login to an existing account' do
partial_lti_teacher = create :teacher
fake_id_token = {iss: @lti_integration.issuer, aud: @lti_integration.client_id, sub: 'foo'}
auth_id = Services::Lti::AuthIdGenerator.new(fake_id_token).call
ao = AuthenticationOption.new(
authentication_id: auth_id,
credential_type: AuthenticationOption::LTI_V1,
email: @user.email,
)
partial_lti_teacher.authentication_options = [ao]
PartialRegistration.persist_attributes session, partial_lti_teacher
User.any_instance.stubs(:valid_password?).returns(true)

post :link_email, params: {email: @user.email, password: 'password'}
assert_redirected_to home_path
assert Policies::Lti.lti?(@user)
end

test 'fails if the password is wrong' do
PartialRegistration.stubs(:in_progress?).returns(true)

post :link_email, params: {email: @user.email, password: 'password'}
assert_response :unauthorized
end

test 'blocks access if lti_account_linking_enabled is false' do
sign_in @user
DCDO.expects(:get).with('lti_account_linking_enabled', false).times(3).returns(false)
get :existing_account
assert_response :not_found
get :landing
assert_response :not_found
post :link_email, params: {email: @user.email, password: 'password'}
assert_response :not_found
end

test 'allows access if lti_account_linking_enabled is true' do
get :existing_account
assert_response :ok
end
end
25 changes: 25 additions & 0 deletions dashboard/test/lib/services/lti/account_linker_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
require 'test_helper'

class Services::Lti::AccountLinkerTest < ActiveSupport::TestCase
setup do
@user = create :teacher
@session = {}
@lti_integration = create :lti_integration
end

test 'reassigns an auth option from a partial registration to an existing user' do
partial_lti_teacher = create :teacher
ao = create :lti_authentication_option
fake_id_token = {iss: @lti_integration.issuer, aud: @lti_integration.client_id, sub: 'foo'}
auth_id = Services::Lti::AuthIdGenerator.new(fake_id_token).call
ao.update(authentication_id: auth_id)
partial_lti_teacher.authentication_options = [ao]
PartialRegistration.persist_attributes @session, partial_lti_teacher
assert_equal 1, @user.authentication_options.count
refute Policies::Lti.lti?(@user)

Services::Lti::AccountLinker.call(user: @user, session: @session)
assert_equal 2, @user.reload.authentication_options.count
assert Policies::Lti.lti?(@user)
end
end