diff --git a/Changelog.md b/Changelog.md index 950edcc9da..07b9ed24e0 100644 --- a/Changelog.md +++ b/Changelog.md @@ -8,6 +8,7 @@ ### 🚨 Breaking changes ### ✨ New features and improvements +- Added JupyterHub session tokens in `jupyter/create_session` route (#8135) - Added experimental support for JupyterHub integration for assignment submission (#7986) - Enforced restriction on grouping deletion when submissions exist (#8134) - Improved table selection column styling, fixed table overflow in containers, and hid unused scrollbars (#8133) diff --git a/app/controllers/jupyter/jupyter_submissions_controller.rb b/app/controllers/jupyter/jupyter_submissions_controller.rb index e9f732defa..979753b3e4 100644 --- a/app/controllers/jupyter/jupyter_submissions_controller.rb +++ b/app/controllers/jupyter/jupyter_submissions_controller.rb @@ -20,39 +20,57 @@ class SubmissionError < StandardError; end }.freeze RESPONSE_BODY_TRUNCATE_LENGTH = 500 + JUPYTER_SESSION_TTL = 15.minutes - # The Jupyter endpoint resolves the submitting user separately, so it - # should not require an existing MarkUs browser session. - skip_before_action :verify_authenticity_token, only: [:submit], raise: false - skip_before_action :authenticate, only: [:submit] - skip_before_action :check_record, only: [:submit] - skip_before_action :check_course_switch, only: [:submit] + # codeql[rb/csrf-protection-disabled] -- authenticated via header token, not cookies, so CSRF does not apply + skip_before_action :verify_authenticity_token, only: [:create_session, :submit], raise: false - skip_verify_authorized only: :submit + # The Jupyter endpoints resolve the submitting user separately, so they + # do not require an existing MarkUs browser session. + skip_before_action :authenticate, only: [:create_session, :submit] + skip_before_action :check_record, only: [:create_session, :submit] + skip_before_action :check_course_switch, only: [:create_session, :submit] + + skip_verify_authorized only: [:create_session, :submit] before_action :ensure_jupyter_enabled! + before_action :authenticate_jupyter_session!, only: [:submit] + + # Verifies the caller's JupyterHub token and mints a short-lived signed session token. + def create_session + jupyter_info = create_session_params[:jupyter] + origin, = parse_jupyter_base_url!(jupyter_info[:base_url]) + token = jupyter_info[:token] + + user_name = find_username_from_jupyter_token!(origin, token) + session_token, expires_at = encode_jupyter_session(user_name: user_name, origin: origin, token: token) + + render json: { + status: 'success', + session_token: session_token, + expires_at: expires_at.iso8601, + markus_user_name: user_name + } + rescue StandardError => e + render_error(e) + end def submit payload = submit_params - jupyter_info = payload[:jupyter] jupyter_path = payload[:notebook_path].to_s destination_path = File.basename(jupyter_path) if destination_path.blank? raise ArgumentError, I18n.t('jupyter.submit.missing_destination_filename') end - origin, base_path = parse_jupyter_base_url!(jupyter_info[:base_url]) - token = jupyter_info[:token] - - user = find_user_from_jupyter_token!(origin, token) course = find_course_from_payload!(payload) - student = course.students.find_by(user_id: user.id) + student = course.students.find_by(user_id: current_user.id) if student.nil? raise ForbiddenError, - I18n.t('jupyter.submit.not_a_student', user_name: user.user_name, course_name: course.name) + I18n.t('jupyter.submit.not_a_student', user_name: current_user.user_name, course_name: course.name) end assignment = find_assignment_from_payload!(payload, student) @@ -61,7 +79,7 @@ def submit raise ForbiddenError, I18n.t('submissions.api_submission_disabled') end - jupyter_file = fetch_jupyter_file!(origin, base_path, token, jupyter_path) + jupyter_file = fetch_jupyter_file!(@jupyter_origin, @jupyter_base_path, @jupyter_token, jupyter_path) submit_jupyter_file!( assignment: assignment, @@ -79,10 +97,16 @@ def submit course: course.name, assignment_id: assignment.id, assignment: assignment.short_identifier, - markus_user_name: user.user_name + markus_user_name: current_user.user_name } } rescue StandardError => e + render_error(e) + end + + private + + def render_error(e) status = ERROR_STATUSES.find { |error_class, _| e.is_a?(error_class) }&.last if status.nil? Rails.logger.error("Jupyter submission failed: #{e.class}: #{e.message}\n#{e.backtrace&.join("\n")}") @@ -100,8 +124,6 @@ def submit end end - private - def ensure_jupyter_enabled! return if Settings.jupyter.enabled @@ -111,11 +133,38 @@ def ensure_jupyter_enabled! }, status: :service_unavailable end - def submit_params - params.require([:notebook_path, :jupyter]) + # Resolves +current_user+ (via +@real_user+, mirroring Api::MainApiController#authenticate) + # from a previously-issued session token, without touching the MarkUs session cookie. + # Stashes the parsed Jupyter origin/base_path/token as ivars so +submit+ doesn't need to + # re-parse +jupyter.base_url+. + def authenticate_jupyter_session! params.require(:jupyter).require([:base_url, :token]) + jupyter_info = params.require(:jupyter).permit(:base_url, :token) + session_token = params[:session_token] + + if session_token.blank? + raise IdentityError, I18n.t('jupyter.submit.missing_session_token') + end - params.permit(:notebook_path, :course_id, :course, :assignment_id, :assignment, + origin, base_path = parse_jupyter_base_url!(jupyter_info[:base_url]) + @jupyter_origin = origin + @jupyter_base_path = base_path + @jupyter_token = jupyter_info[:token] + @real_user = decode_jupyter_session!(session_token, origin: origin, token: @jupyter_token) + rescue StandardError => e + render_error(e) + end + + def create_session_params + params.require(:jupyter).require([:base_url, :token]) + + params.permit(jupyter: [:base_url, :token]) + end + + def submit_params + params.require(:notebook_path) + + params.permit(:notebook_path, :session_token, :course_id, :course, :assignment_id, :assignment, jupyter: [:base_url, :token]) end @@ -155,7 +204,7 @@ def parse_jupyter_base_url!(base_url) raise BadRequestError, I18n.t('jupyter.submit.unparseable_base_url', error: e.message) end - def find_user_from_jupyter_token!(origin, token) + def find_username_from_jupyter_token!(origin, token) uri = URI.parse("#{origin}/hub/api/user") model = jupyter_api_get!(uri, token, error_class: IdentityError) name = model['name'] @@ -164,13 +213,51 @@ def find_user_from_jupyter_token!(origin, token) raise IdentityError, I18n.t('jupyter.submit.missing_username') end - user = User.find_by(user_name: name) + unless User.exists?(user_name: name) + raise ActiveRecord::RecordNotFound, I18n.t('jupyter.submit.unknown_user', user_name: name.inspect) + end + + name + end + + def encode_jupyter_session(user_name:, origin:, token:) + expires_at = JUPYTER_SESSION_TTL.from_now + payload = { + 'user_name' => user_name, + 'origin' => origin, + 'token_hash' => Digest::SHA256.hexdigest(token), + 'expires_at' => expires_at.to_i + } + + [Rails.application.message_verifier(:jupyter_session).generate(payload), expires_at] + end + + # Verifies a session token minted by +encode_jupyter_session+. Verifies request + # params origin and JupyterHub token against the session token. + def decode_jupyter_session!(session_token, origin:, token:) + payload = Rails.application.message_verifier(:jupyter_session).verify(session_token) + + if payload['expires_at'].to_i < Time.current.to_i + raise IdentityError, I18n.t('jupyter.submit.session_expired') + end + + unless payload['origin'] == origin + raise IdentityError, I18n.t('jupyter.submit.session_origin_mismatch') + end + + unless ActiveSupport::SecurityUtils.secure_compare(payload['token_hash'], Digest::SHA256.hexdigest(token)) + raise IdentityError, I18n.t('jupyter.submit.session_token_mismatch') + end + + user = User.find_by(user_name: payload['user_name']) if user.nil? - raise ActiveRecord::RecordNotFound, I18n.t('jupyter.submit.unknown_user', user_name: name.inspect) + raise ActiveRecord::RecordNotFound, I18n.t('jupyter.submit.unknown_user', user_name: payload['user_name']) end user + rescue ActiveSupport::MessageVerifier::InvalidSignature + raise IdentityError, I18n.t('jupyter.submit.invalid_session_token') end def find_course_from_payload!(payload) diff --git a/config/initializers/cors.rb b/config/initializers/cors.rb index 55fbe5f757..ca5589282e 100644 --- a/config/initializers/cors.rb +++ b/config/initializers/cors.rb @@ -15,7 +15,10 @@ headers: :any, methods: [:post] - # New JupyterLab extension submission endpoint. + # New JupyterLab submission extension endpoints. + resource %r{/jupyter/authenticate}, + headers: :any, + methods: [:post, :options] resource %r{/jupyter/submit}, headers: :any, methods: [:post, :options] diff --git a/config/locales/views/jupyter/en.yml b/config/locales/views/jupyter/en.yml index dc5fc7da87..cc78cd6207 100644 --- a/config/locales/views/jupyter/en.yml +++ b/config/locales/views/jupyter/en.yml @@ -11,12 +11,17 @@ en: invalid_base_url_path: Jupyter base_url "%{base_url}" must not contain ".." path segments. invalid_json_response: 'JupyterHub response from %{uri} was not valid JSON: %{error}' invalid_jupyter_url: 'Invalid Jupyter URL: %{error}' + invalid_session_token: Jupyter session_token is invalid. missing_assignment: Submission data must contain "assignment_id" or "assignment". missing_course: Submission data must contain "course_id" or "course" field. missing_destination_filename: Could not determine a destination filename for the submission. + missing_session_token: Submission data must contain a "session_token" obtained from jupyter/authenticate. missing_username: JupyterHub identity response did not include a username. not_a_student: MarkUs user "%{user_name}" is not a student in course "%{course_name}". origin_not_allowed: Jupyter origin "%{origin}" is not in the configured list of allowed hosts. request_failed: 'JupyterHub request to %{uri} returned HTTP %{code}: %{body}' + session_expired: Jupyter session_token has expired. Please authenticate again. + session_origin_mismatch: Jupyter session_token was issued for a different Jupyter origin. + session_token_mismatch: Jupyter session_token does not match the supplied Jupyter token. unknown_user: No MarkUs user exists with user_name=%{user_name}. unparseable_base_url: 'Invalid Jupyter base_url: %{error}' diff --git a/config/routes.rb b/config/routes.rb index 7067d2daa1..22d23dc810 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1103,6 +1103,7 @@ post 'main/logout', controller: 'main', action: 'logout' namespace :jupyter do + post 'authenticate', controller: 'jupyter_submissions', action: 'create_session' post 'submit', controller: 'jupyter_submissions', action: 'submit' end diff --git a/spec/controllers/jupyter/jupyter_submissions_controller_spec.rb b/spec/controllers/jupyter/jupyter_submissions_controller_spec.rb index a9a62fcf11..66f8050bc4 100644 --- a/spec/controllers/jupyter/jupyter_submissions_controller_spec.rb +++ b/spec/controllers/jupyter/jupyter_submissions_controller_spec.rb @@ -7,7 +7,7 @@ let(:notebook_content) { { 'cells' => [], 'metadata' => {}, 'nbformat' => 4, 'nbformat_minor' => 5 } } let(:jupyter_params) { { base_url: base_url, token: token } } - let(:valid_params) do + let(:base_submit_params) do { notebook_path: notebook_path, course_id: course.id, @@ -38,23 +38,70 @@ def stub_notebook_contents(status: 200, body: nil) .to_return(status: status, body: body) end + # Authenticates via jupyter/authenticate (stubbing the Hub identity lookup) and returns the + # resulting session_token, so submit specs don't need to re-derive one by hand. + def create_session_token!(user_name:) + stub_hub_identity(user_name: user_name) + post :create_session, params: { jupyter: jupyter_params } + response.parsed_body.fetch('session_token') + end + before do allow(Settings.jupyter).to receive(:enabled).and_return(true) allow(Settings.jupyter_server).to receive(:hosts).and_return([origin]) end + describe 'authenticate' do + let!(:student) { create(:student, course: course) } + + it 'returns a session_token and the markus_user_name for a valid Hub token' do + stub_hub_identity(user_name: student.user_name) + + post :create_session, params: { jupyter: jupyter_params } + + expect(response).to have_http_status :ok + body = response.parsed_body + expect(body['status']).to eq 'success' + expect(body['session_token']).to be_present + expect(body['markus_user_name']).to eq student.user_name + end + + it 'returns 401 when the JupyterHub identity lookup fails' do + stub_hub_identity(user_name: student.user_name, status: 403) + + post :create_session, params: { jupyter: jupyter_params } + + expect(response).to have_http_status :unauthorized + end + + it 'returns 404 when the JupyterHub user has no matching MarkUs account' do + stub_hub_identity(user_name: 'no-such-markus-user') + + post :create_session, params: { jupyter: jupyter_params } + + expect(response).to have_http_status :not_found + end + + it 'returns 503 when the jupyter feature flag is disabled' do + allow(Settings.jupyter).to receive(:enabled).and_return(false) + + post :create_session, params: { jupyter: jupyter_params } + + expect(response).to have_http_status :service_unavailable + end + end + describe 'successful submissions' do context 'when the assignment only allows students to work alone' do let(:assignment) { create(:assignment, course: course, assignment_properties_attributes: { api_submit: true }) } let!(:student) { create(:student, course: course) } - before do - stub_hub_identity(user_name: student.user_name) - stub_notebook_contents - end + before { stub_notebook_contents } it 'creates a solo group for the student and submits the notebook to the repo' do - post :submit, params: valid_params + session_token = create_session_token!(user_name: student.user_name) + + post :submit, params: base_submit_params.merge(session_token: session_token) expect(response).to have_http_status :ok body = response.parsed_body @@ -76,6 +123,23 @@ def stub_notebook_contents(status: 200, body: nil) expect(files['hw1.ipynb']).not_to be_nil end end + + it 'does not write to the MarkUs session cookie' do + session_token = create_session_token!(user_name: student.user_name) + + post :submit, params: base_submit_params.merge(session_token: session_token) + + expect(session[:user_name]).to be_nil + end + + it 'does not re-verify identity against JupyterHub when submitting' do + session_token = create_session_token!(user_name: student.user_name) + + post :submit, params: base_submit_params.merge(session_token: session_token) + + expect(response).to have_http_status :ok + expect(WebMock).to have_requested(:get, "#{origin}/hub/api/user").once + end end context 'when the assignment allows groups of students' do @@ -84,15 +148,13 @@ def stub_notebook_contents(status: 200, body: nil) end let!(:student) { create(:student, course: course) } - before do - stub_hub_identity(user_name: student.user_name) - stub_notebook_contents - end + before { stub_notebook_contents } it 'auto-creates a group for the student and submits the notebook' do expect(student.has_accepted_grouping_for?(assignment.id)).to be false - post :submit, params: valid_params + session_token = create_session_token!(user_name: student.user_name) + post :submit, params: base_submit_params.merge(session_token: session_token) expect(response).to have_http_status :ok expect(student.has_accepted_grouping_for?(assignment.id)).to be true @@ -107,82 +169,120 @@ def stub_notebook_contents(status: 200, body: nil) it 'returns 503 when the jupyter feature flag is disabled' do allow(Settings.jupyter).to receive(:enabled).and_return(false) - post :submit, params: valid_params + post :submit, params: base_submit_params expect(response).to have_http_status :service_unavailable end - it 'returns 400 when a top-level required param is missing' do - post :submit, params: valid_params.except(:notebook_path) + it 'returns 401 when session_token is missing' do + post :submit, params: base_submit_params - expect(response).to have_http_status :bad_request + expect(response).to have_http_status :unauthorized end it 'returns 400 when a required jupyter field is missing' do - post :submit, params: valid_params.merge(jupyter: jupyter_params.except(:token)) + post :submit, params: base_submit_params.merge(jupyter: jupyter_params.except(:token), session_token: 'x') expect(response).to have_http_status :bad_request end it 'returns 400 when base_url is not an absolute http(s) URL' do - post :submit, params: valid_params.merge(jupyter: jupyter_params.merge(base_url: 'not-a-url')) + post :submit, params: base_submit_params.merge(jupyter: jupyter_params.merge(base_url: 'not-a-url'), + session_token: 'x') expect(response).to have_http_status :bad_request end it 'returns 400 when the base_url origin is not in the configured allowlist' do + session_token = create_session_token!(user_name: student.user_name) allow(Settings.jupyter_server).to receive(:hosts).and_return(['http://a-different-host.test']) - post :submit, params: valid_params + post :submit, params: base_submit_params.merge(session_token: session_token) expect(response).to have_http_status :bad_request end - it 'returns 401 when the JupyterHub identity lookup fails' do - stub_hub_identity(user_name: student.user_name, status: 403) + it 'returns 401 when session_token is garbage/tampered' do + post :submit, params: base_submit_params.merge(session_token: 'not-a-real-session-token') - post :submit, params: valid_params + expect(response).to have_http_status :unauthorized + end + + it 'returns 401 when session_token has expired' do + payload = { + 'user_name' => student.user_name, + 'origin' => origin, + 'token_hash' => Digest::SHA256.hexdigest(token), + 'expires_at' => 1.minute.ago.to_i + } + expired_token = Rails.application.message_verifier(:jupyter_session).generate(payload) + + post :submit, params: base_submit_params.merge(session_token: expired_token) expect(response).to have_http_status :unauthorized end - it 'returns 404 when the JupyterHub user has no matching MarkUs account' do - stub_hub_identity(user_name: 'no-such-markus-user') + it 'returns 401 when session_token was issued for a different Jupyter token (spoofing attempt)' do + session_token = create_session_token!(user_name: student.user_name) - post :submit, params: valid_params + post :submit, params: base_submit_params.merge( + jupyter: jupyter_params.merge(token: 'a-different-token'), + session_token: session_token + ) - expect(response).to have_http_status :not_found + expect(response).to have_http_status :unauthorized + end + + it 'returns 401 when session_token was issued for a different Jupyter origin' do + second_origin = 'http://other-jupyter.example.test' + allow(Settings.jupyter_server).to receive(:hosts).and_return([origin, second_origin]) + session_token = create_session_token!(user_name: student.user_name) + + post :submit, params: base_submit_params.merge( + jupyter: jupyter_params.merge(base_url: "#{second_origin}/user/testuser/"), + session_token: session_token + ) + + expect(response).to have_http_status :unauthorized + end + + it 'returns 400 when a top-level required param is missing' do + session_token = create_session_token!(user_name: student.user_name) + + post :submit, params: base_submit_params.except(:notebook_path).merge(session_token: session_token) + + expect(response).to have_http_status :bad_request end it 'returns 404 when the course cannot be found' do - stub_hub_identity(user_name: student.user_name) + session_token = create_session_token!(user_name: student.user_name) - post :submit, params: valid_params.merge(course_id: -1) + post :submit, params: base_submit_params.merge(course_id: -1, session_token: session_token) expect(response).to have_http_status :not_found end it 'returns 403 when the user is not a student in the given course' do outsider = create(:student, course: create(:course)) - stub_hub_identity(user_name: outsider.user_name) + session_token = create_session_token!(user_name: outsider.user_name) - post :submit, params: valid_params + post :submit, params: base_submit_params.merge(session_token: session_token) expect(response).to have_http_status :forbidden end it 'returns 404 when the assignment cannot be found' do - stub_hub_identity(user_name: student.user_name) + session_token = create_session_token!(user_name: student.user_name) - post :submit, params: valid_params.merge(assignment_id: -1) + post :submit, params: base_submit_params.merge(assignment_id: -1, session_token: session_token) expect(response).to have_http_status :not_found end it 'returns 403 when API submission is disabled for the assignment' do - stub_hub_identity(user_name: student.user_name) + session_token = create_session_token!(user_name: student.user_name) - post :submit, params: valid_params + post :submit, params: base_submit_params.merge(session_token: session_token) expect(response).to have_http_status :forbidden expect(response.parsed_body['message']).to eq I18n.t('submissions.api_submission_disabled') @@ -190,10 +290,11 @@ def stub_notebook_contents(status: 200, body: nil) it 'returns 502 when fetching the notebook contents fails' do enabled_assignment = create(:assignment, course: course, assignment_properties_attributes: { api_submit: true }) - stub_hub_identity(user_name: student.user_name) + session_token = create_session_token!(user_name: student.user_name) stub_notebook_contents(status: 404, body: 'Not Found') - post :submit, params: valid_params.merge(assignment_id: enabled_assignment.id) + post :submit, params: base_submit_params.merge(assignment_id: enabled_assignment.id, + session_token: session_token) expect(response).to have_http_status :bad_gateway end @@ -203,10 +304,11 @@ def stub_notebook_contents(status: 200, body: nil) assignment_properties_attributes: { only_required_files: true, api_submit: true }) create(:assignment_file, assignment: required_assignment, filename: 'required.ipynb') - stub_hub_identity(user_name: student.user_name) + session_token = create_session_token!(user_name: student.user_name) stub_notebook_contents - post :submit, params: valid_params.merge(assignment_id: required_assignment.id) + post :submit, params: base_submit_params.merge(assignment_id: required_assignment.id, + session_token: session_token) expect(response).to have_http_status :unprocessable_content end