From 936c11e31ff64c063c17dce5d8e7c7710a1518e3 Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Tue, 20 Sep 2022 16:24:42 +0200 Subject: [PATCH] Refactor authentication token for new study-group-based authorization --- app/controllers/application_controller.rb | 10 +++++++++- app/controllers/subscriptions_controller.rb | 3 ++- app/mailers/user_mailer.rb | 6 +++--- app/models/authentication_token.rb | 5 +++-- spec/factories/authentication_token.rb | 7 +++++++ spec/features/authentication_spec.rb | 9 +++++---- spec/mailers/user_mailer_spec.rb | 2 +- 7 files changed, 30 insertions(+), 12 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1fb3c47a..941d7606 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -48,7 +48,15 @@ class ApplicationController < ActionController::Base if token.expire_at.future? token.update(expire_at: Time.zone.now) - auto_login(token.user) + session[:study_group_id] = token.study_group_id + + # Sorcery Login only works for InternalUsers + return auto_login(token.user) if token.user.is_a? InternalUser + + # All external users are logged in "manually" + session[:external_user_id] = token.user.id + session.delete(:lti_parameters_id) + token.user end end diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb index bff28ac4..f3caeb29 100644 --- a/app/controllers/subscriptions_controller.rb +++ b/app/controllers/subscriptions_controller.rb @@ -55,9 +55,10 @@ class SubscriptionsController < ApplicationController def subscription_params current_user_id = current_user.try(:id) current_user_class_name = current_user.try(:class).try(:name) + study_group_id = current_user.try(:current_study_group_id) if params[:subscription].present? params[:subscription].permit(:request_for_comment_id, :subscription_type).merge(user_id: current_user_id, - user_type: current_user_class_name, deleted: false) + user_type: current_user_class_name, study_group_id: study_group_id, deleted: false) end end private :subscription_params diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 1bc42c89..6f2b665a 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -20,7 +20,7 @@ class UserMailer < ApplicationMailer def got_new_comment(comment, request_for_comment, commenting_user) # TODO: check whether we can take the last known locale of the receiver? - token = AuthenticationToken.generate!(request_for_comment.user) + token = AuthenticationToken.generate!(request_for_comment.user, request_for_comment.submission.study_group) @receiver_displayname = request_for_comment.user.displayname @commenting_user_displayname = commenting_user.displayname @comment_text = ERB::Util.html_escape comment.text @@ -32,7 +32,7 @@ class UserMailer < ApplicationMailer end def got_new_comment_for_subscription(comment, subscription, from_user) - token = AuthenticationToken.generate!(subscription.user) + token = AuthenticationToken.generate!(subscription.user, subscription.study_group) @receiver_displayname = subscription.user.displayname @author_displayname = from_user.displayname @comment_text = ERB::Util.html_escape comment.text @@ -45,7 +45,7 @@ class UserMailer < ApplicationMailer end def send_thank_you_note(request_for_comment, receiver) - token = AuthenticationToken.generate!(receiver) + token = AuthenticationToken.generate!(receiver, request_for_comment.submission.study_group) @receiver_displayname = receiver.displayname @author = request_for_comment.user.displayname @thank_you_note = ERB::Util.html_escape request_for_comment.thank_you_note diff --git a/app/models/authentication_token.rb b/app/models/authentication_token.rb index 60a33137..e2bd732f 100644 --- a/app/models/authentication_token.rb +++ b/app/models/authentication_token.rb @@ -6,11 +6,12 @@ class AuthenticationToken < ApplicationRecord include Creation belongs_to :study_group, optional: true - def self.generate!(user) + def self.generate!(user, study_group) create!( shared_secret: SecureRandom.hex(32), user: user, - expire_at: 7.days.from_now + expire_at: 7.days.from_now, + study_group: study_group ) end end diff --git a/spec/factories/authentication_token.rb b/spec/factories/authentication_token.rb index f3102284..e3133400 100644 --- a/spec/factories/authentication_token.rb +++ b/spec/factories/authentication_token.rb @@ -6,6 +6,13 @@ FactoryBot.define do shared_secret { SecureRandom.hex(32) } expire_at { 7.days.from_now } + after(:create) do |auth_token| + # Do not change anything if a study group was provided explicitly or user has no study groups + next if auth_token.study_group_id.present? || auth_token.user.study_groups.blank? + + auth_token.update!(study_group_id: auth_token.user.study_groups.first.id) + end + trait :invalid do expire_at { 8.days.ago } end diff --git a/spec/features/authentication_spec.rb b/spec/features/authentication_spec.rb index e28fce26..5555b269 100644 --- a/spec/features/authentication_spec.rb +++ b/spec/features/authentication_spec.rb @@ -48,15 +48,16 @@ describe 'Authentication' do context 'with an authentication token' do let(:user) { create(:learner) } + let(:study_group) { request_for_comment.submission.study_group } let(:request_for_comment) { create(:rfc_with_comment, user: user) } let(:commenting_user) { InternalUser.create(attributes_for(:teacher)) } let(:mail) { UserMailer.got_new_comment(request_for_comment.comments.first, request_for_comment, commenting_user) } let(:rfc_link) { request_for_comment_url(request_for_comment, token: token.shared_secret) } - before { allow(AuthenticationToken).to receive(:generate!).with(user).and_return(token).once } + before { allow(AuthenticationToken).to receive(:generate!).with(user, study_group).and_return(token).once } context 'when the token is valid' do - let(:token) { create(:authentication_token, user: user) } + let(:token) { create(:authentication_token, user: user, study_group: study_group) } it 'allows access to the request for comment' do mail.deliver_now @@ -67,7 +68,7 @@ describe 'Authentication' do end context 'with an expired authentication token' do - let(:token) { create(:authentication_token, :invalid, user: user) } + let(:token) { create(:authentication_token, :invalid, user: user, study_group: study_group) } it 'denies access to the request for comment' do mail.deliver_now @@ -80,7 +81,7 @@ describe 'Authentication' do end context 'when the authentication token is used to login' do - let(:token) { create(:authentication_token, user: user) } + let(:token) { create(:authentication_token, user: user, study_group: study_group) } it 'invalidates the token on login' do mail.deliver_now diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb index 324c0945..551ed14e 100644 --- a/spec/mailers/user_mailer_spec.rb +++ b/spec/mailers/user_mailer_spec.rb @@ -118,7 +118,7 @@ describe UserMailer do let(:user) { create(:learner) } let(:token) { AuthenticationToken.find_by(user: user) } let(:request_for_comment) { create(:rfc_with_comment, user: user) } - let(:subscription) { Subscription.create(request_for_comment: request_for_comment, user: user) } + let(:subscription) { Subscription.create(request_for_comment: request_for_comment, user: user, study_group_id: user.current_study_group_id) } let(:from_user) { InternalUser.create(attributes_for(:teacher)) } let(:mail) { described_class.got_new_comment_for_subscription(request_for_comment.comments.first, subscription, from_user).deliver_now }