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

Conversation

carl-codeorg
Copy link
Contributor

@carl-codeorg carl-codeorg commented May 8, 2024

The first PR to start implementing account linking for LTI.

  • Adds a controller to handle the new account linking pages and routes. This includes a route for linking a new LTI login to an existing email/password account.
  • Adds a service for account linking. The main LTI service is getting large, and there will be more code once we start implementing linking LTI logins to Oauth accounts, so I figured this deserves its own service.
  • Adds unstyled placeholders for the new LTI registration landing page and account linking page.
  • All the new routes/pages are gated behind a DOTD flag to keep them disabled until we're ready to ship.

Links

JIRA
LTI tech spec

Testing story

To test this locally, set enable the DCDO flag:

DCDO.set('lti_account_linking_enabled', true)

Then make sure you have an existing user with an email login to test with. Launch from a new user in the LMS (or just delete any existing LTI auth_options for that user). Click "I already have a Code.org account" and then enter the creds for your existing email account. Your LTI auth option should now be attached to the email account, and you will be logged in and redirected to the homepage.

Unit Tests:

bundle exec spring testunit test/controllers/lti/v1/users_controller_test.rb
  4/4: [====================================================] 100% Time: 00:00:01, Time: 00:00:01

Finished in 1.12101s
4 tests, 9 assertions, 0 failures, 0 errors, 0 skips
bundle exec spring testunit test/lib/services/lti/account_linker_test.rb
  1/1: [====================================================] 100% Time: 00:00:00, Time: 00:00:00

Finished in 0.25297s
1 tests, 4 assertions, 0 failures, 0 errors, 0 skips

Deployment strategy

Follow-up work

This PR just lays the groundwork for the rest of the LTI account linking feature. Not intended to be prod ready yet, so it's behind a DCDO flag.

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@carl-codeorg carl-codeorg marked this pull request as ready for review May 8, 2024 16:08
@carl-codeorg carl-codeorg requested a review from a team May 8, 2024 16:11
dashboard/config/routes.rb Outdated Show resolved Hide resolved
dashboard/app/controllers/lti_v1_controller.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@artem-vavilov artem-vavilov left a comment

Choose a reason for hiding this comment

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

Good job! 👍🏻

Co-authored-by: Artem Vavilov <artem.vavilov.7@gmail.com>
carl-codeorg and others added 2 commits May 9, 2024 09:01
…ml.haml

Co-authored-by: Artem Vavilov <artem.vavilov.7@gmail.com>
…ml.haml

Co-authored-by: Artem Vavilov <artem.vavilov.7@gmail.com>
# 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

@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

Copy link
Contributor

@nicklathe nicklathe left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@carl-codeorg carl-codeorg merged commit c1d78db into staging May 9, 2024
2 checks passed
@carl-codeorg carl-codeorg deleted the p20-908/lti-linking-controller branch May 9, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants