From 46ab58512e01901685f6f66e0f1e10b1560ad916 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 14 Aug 2025 12:12:17 +0100 Subject: [PATCH] Add optional on_login_success callback This adds a new `on_login_success` configuration option. If this option is set to a `Proc` and the login was successful, the `Proc` will be called in the context of the `RpiAuth::AuthController#callback` action, i.e. `current_user` will be available. This is intended to allow apps to record successful logins. I'm not convinced that using `instance_exec` is the most sensible approach, but I decided it was better to be consistent with the way the `success_redirect` option is implemented. If the `Proc` raises an exception, this will be rescued and the exception will be logged as a warning in the Rails log. This is so that the login will still complete successfully, even if there was a problem recording it. The immediate motivation for adding this is so we can record successful *student* logins in `experience-cs` in order to be able to calculate the "active users" metric described in this issue [1]. Unfortunately while this information is available in the `profile` app database for regular users; it's not available for student users. [1]: https://github.com/RaspberryPiFoundation/experience-cs/issues/727 --- CHANGELOG.md | 2 + README.md | 4 ++ app/controllers/rpi_auth/auth_controller.rb | 12 ++++++ lib/rpi_auth/configuration.rb | 3 +- spec/dummy/spec/requests/auth_request_spec.rb | 41 +++++++++++++++++++ spec/rpi_auth/configuration_spec.rb | 4 ++ 6 files changed, 65 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19128c0..0bc8f59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Add optional `on_login_success` callback (#90) + ### Fixed ### Removed diff --git a/README.md b/README.md index c75a833..374fa3f 100644 --- a/README.md +++ b/README.md @@ -170,6 +170,10 @@ link_to 'Log out', rpi_auth_logout_path, params: { returnTo: '/thanks-dude' } This has to be a relative URL, i.e. it has to start with a slash. This is to ensure there's no open redirect. +### Callback on successful login + +If the RpiAuth configuration option `on_login_success` is set to a `Proc`, this will be called in the context of the `RpiAuth::AuthController#callback` action, i.e. `current_user` will be available. This is intended to allow apps to record successful logins. + ### Globbed/catch-all routes If your app has a catch-all route at the end of the routing table, you must diff --git a/app/controllers/rpi_auth/auth_controller.rb b/app/controllers/rpi_auth/auth_controller.rb index 25d1cee..aae37e7 100644 --- a/app/controllers/rpi_auth/auth_controller.rb +++ b/app/controllers/rpi_auth/auth_controller.rb @@ -27,6 +27,8 @@ def callback auth = request.env['omniauth.auth'] self.current_user = RpiAuth.user_model.from_omniauth(auth) + run_login_success_callback + redirect_to ensure_relative_url(login_redirect_path) end @@ -53,6 +55,16 @@ def failure private + def run_login_success_callback + return unless RpiAuth.configuration.on_login_success.is_a?(Proc) + + begin + instance_exec(&RpiAuth.configuration.on_login_success) + rescue StandardError => e + Rails.logger.warn("Caught #{e} while processing on_login_success proc.") + end + end + def login_redirect_path unless RpiAuth.configuration.success_redirect.is_a?(Proc) return RpiAuth.configuration.success_redirect || request.env['omniauth.origin'] diff --git a/lib/rpi_auth/configuration.rb b/lib/rpi_auth/configuration.rb index 95aa78f..6082f2a 100644 --- a/lib/rpi_auth/configuration.rb +++ b/lib/rpi_auth/configuration.rb @@ -19,7 +19,8 @@ class Configuration :session_keys_to_persist, :success_redirect, :user_model, - :setup + :setup, + :on_login_success def initialize @bypass_auth = false diff --git a/spec/dummy/spec/requests/auth_request_spec.rb b/spec/dummy/spec/requests/auth_request_spec.rb index 20e6941..9dc642c 100644 --- a/spec/dummy/spec/requests/auth_request_spec.rb +++ b/spec/dummy/spec/requests/auth_request_spec.rb @@ -2,6 +2,11 @@ require 'spec_helper' +class Foo + cattr_accessor :successful_logins + self.successful_logins = [] +end + RSpec.describe 'Authentication' do let(:user) do User.new( @@ -311,6 +316,42 @@ end end end + + context 'when on_login_success is set as a proc in config' do + before do + allow(Rails.logger).to receive(:warn).and_call_original + + RpiAuth.configuration.on_login_success = lambda do + Rails.logger.warn "Successful login: #{current_user.user_id}" + end + end + + it 'calls the proc making the current user available' do + post '/auth/rpi' + follow_redirect! + + expect(Rails.logger).to have_received(:warn).with( + "Successful login: #{user.user_id}" + ) + end + + context 'when the proc raises an exception' do + before do + RpiAuth.configuration.on_login_success = lambda do + raise 'boom!' + end + end + + it 'logs a warning error' do + post '/auth/rpi' + follow_redirect! + + expect(Rails.logger).to have_received(:warn).with( + 'Caught boom! while processing on_login_success proc.' + ) + end + end + end end describe 'and toggling the scope at runtime' do diff --git a/spec/rpi_auth/configuration_spec.rb b/spec/rpi_auth/configuration_spec.rb index eb92ce3..60aaef7 100644 --- a/spec/rpi_auth/configuration_spec.rb +++ b/spec/rpi_auth/configuration_spec.rb @@ -110,4 +110,8 @@ it_behaves_like 'sets up the token url defaults' end end + + it 'sets on_login_success to nil by default' do + expect(configuration.on_login_success).to be_nil + end end