Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upMake selecting a Lesson or Project work in Chrome incognito #324
Conversation
This doesn't seem to have really had any impact in my testing, but logically we want to favor session based authentication over LtiLaunch.state based auth.
| @@ -297,7 +297,7 @@ | |||
| # call sign_in user which stores the authentication in session and bypasses the need | |||
| # to check authentication. | |||
| manager.strategies.add(:lti_auth, LtiAuthentication::WardenStrategy) | |||
| manager.default_strategies(scope: :user).unshift :lti_auth | |||
| manager.default_strategies(scope: :user) << :lti_auth | |||
This comment has been minimized.
This comment has been minimized.
sadleb
Aug 18, 2020
Author
Contributor
This doesn't seem to have really had any impact in my testing, but logically we want to favor session based authentication over LtiLaunch.state based auth so put the strategy at the end of the array not the beginning.
| span.add_field('lti_auth.status', status) | ||
| span.add_field('status', status) | ||
| span.add_field('lti_auth.message', message) | ||
| span.add_field('lti_auth.url', url) if url | ||
| span.add_field('lti_auth.canvas_id', canvas_id) if canvas_id | ||
| span.add_field('url', url) if url | ||
| span.add_field('canvas_id', canvas_id) if canvas_id | ||
| span.add_field('user_id', user.id) if user |
This comment has been minimized.
This comment has been minimized.
sadleb
Aug 18, 2020
•
Author
Contributor
Just cleaning these keys up based on what we discussed as a norm (aka prefixes for things specific to this span or use existing keys for things that would apply elsewhere). Also, add the user_id in there. That was an oversight. It's nice to have.
| before(:each) do | ||
| sign_in user | ||
| end |
This comment has been minimized.
This comment has been minimized.
sadleb
Aug 18, 2020
Author
Contributor
I contemplated creating two sets of tests, one for session based auth and one for LtiLaunch.state auth, but I don't think it's worth it in the controller so I opted to change this into the fallback which should always work which is LtiLaunch.state auth. Same for the lti_assignments_selection_controller.
sadleb
changed the title
Needed to include the LtiHelper module instead of using the `helpers` instance b/c we use it with `verify_authenticity_token` which simply sends the message to the current controller instance. Test Plan: - `bundle exec rspec spec/controllers/lesson_contents_controller_spec.rb` - `bundle exec rspec spec/controllers/lti_assignment_selection_controller_spec.rb`
It's weird to rely on `is_sessionless_lti_launch?` to set the `@lti_launch` instance.
| @@ -5,5 +5,5 @@ | |||
| <input type='submit' value='Yes' /> | |||
| </form> | |||
| <br/> | |||
| <iframe id="lesson-content-preview" src="<%= lesson_content_url(@lesson_content) %>"></iframe> | |||
| <iframe id="lesson-content-preview" src="<%= lesson_content_url(@lesson_content) %>?state=<%= params[:state] %>"></iframe> | |||
This comment has been minimized.
This comment has been minimized.
rshipp
Aug 18, 2020
Contributor
can we pass state as params in the lesson_content_url helper? i'm not sure if that exists but it seems like it should
This comment has been minimized.
This comment has been minimized.
sadleb commentedAug 18, 2020
•
edited
Task: https://app.asana.com/0/search/1189491188178881/1188539121871585
To make the
LtiAuthentication::WardenStrategywork for anything accessed through anLtiLaunch, thestateparameter must be passed. This PR adds it to two spots I missed in #316. Also, CSRF authenticity token validation only works when we have a session. The concept is to prevent an attacker from getting a session token and being able to piggyback off it so that only applies for session based auth. Only skip verification when we have to fall-back toLtiAuthentication::WardenStrategyauth.Screenshot of where this now works. If you choose a Project or Lesson and insert it with Chrome in this mode, this PR causes that to work:
