From 3330197931ca02074b8fa8916bb03925fbe47b70 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 13:12:37 +0100 Subject: [PATCH 01/12] Add oauth2 gem as dependency --- gemfiles/rails_6.1.gemfile.lock | 17 +++++++++++++++++ gemfiles/rails_7.0.gemfile.lock | 17 +++++++++++++++++ gemfiles/rails_7.1.gemfile.lock | 16 ++++++++++++++++ gemfiles/rails_7.2.gemfile.lock | 16 ++++++++++++++++ rpi_auth.gemspec | 1 + 5 files changed, 67 insertions(+) diff --git a/gemfiles/rails_6.1.gemfile.lock b/gemfiles/rails_6.1.gemfile.lock index 154bffa..40f47c5 100644 --- a/gemfiles/rails_6.1.gemfile.lock +++ b/gemfiles/rails_6.1.gemfile.lock @@ -2,6 +2,7 @@ PATH remote: .. specs: rpi_auth (4.0.0) + oauth2 omniauth-rails_csrf_protection (~> 1.0.0) omniauth_openid_connect (~> 0.7.1) rails (>= 6.1.4) @@ -74,6 +75,7 @@ GEM ast (2.4.3) attr_required (1.0.2) base64 (0.2.0) + bigdecimal (3.1.9) bindata (2.5.1) builder (3.3.0) byebug (12.0.0) @@ -117,6 +119,8 @@ GEM bindata faraday (~> 2.0) faraday-follow_redirects + jwt (2.10.1) + base64 language_server-protocol (3.17.0.4) lint_roller (1.1.0) listen (3.9.0) @@ -137,6 +141,8 @@ GEM mini_mime (1.1.5) mini_portile2 (2.8.8) minitest (5.25.5) + multi_xml (0.7.1) + bigdecimal (~> 3.1) net-http (0.6.0) uri net-imap (0.5.6) @@ -152,6 +158,13 @@ GEM nokogiri (1.18.7) mini_portile2 (~> 2.8.2) racc (~> 1.4) + oauth2 (2.0.9) + faraday (>= 0.17.3, < 3.0) + jwt (>= 1.0, < 3.0) + multi_xml (~> 0.5) + rack (>= 1.2, < 4) + snaky_hash (~> 2.0) + version_gem (~> 1.1) omniauth (2.1.3) hashie (>= 3.4.6) rack (>= 2.2.3) @@ -290,6 +303,9 @@ GEM simplecov_json_formatter (~> 0.1) simplecov-html (0.13.1) simplecov_json_formatter (0.1.4) + snaky_hash (2.0.1) + hashie + version_gem (~> 1.1, >= 1.1.1) sprockets (4.2.1) concurrent-ruby (~> 1.0) rack (>= 2.2.4, < 4) @@ -313,6 +329,7 @@ GEM validate_url (1.0.15) activemodel (>= 3.0.0) public_suffix + version_gem (1.1.7) webfinger (2.1.3) activesupport faraday (~> 2.0) diff --git a/gemfiles/rails_7.0.gemfile.lock b/gemfiles/rails_7.0.gemfile.lock index 265ae65..67f9a86 100644 --- a/gemfiles/rails_7.0.gemfile.lock +++ b/gemfiles/rails_7.0.gemfile.lock @@ -2,6 +2,7 @@ PATH remote: .. specs: rpi_auth (4.0.0) + oauth2 omniauth-rails_csrf_protection (~> 1.0.0) omniauth_openid_connect (~> 0.7.1) rails (>= 6.1.4) @@ -80,6 +81,7 @@ GEM ast (2.4.3) attr_required (1.0.2) base64 (0.2.0) + bigdecimal (3.1.9) bindata (2.5.1) builder (3.3.0) byebug (12.0.0) @@ -123,6 +125,8 @@ GEM bindata faraday (~> 2.0) faraday-follow_redirects + jwt (2.10.1) + base64 language_server-protocol (3.17.0.4) lint_roller (1.1.0) listen (3.9.0) @@ -143,6 +147,8 @@ GEM mini_mime (1.1.5) mini_portile2 (2.8.8) minitest (5.25.5) + multi_xml (0.7.1) + bigdecimal (~> 3.1) net-http (0.6.0) uri net-imap (0.5.6) @@ -158,6 +164,13 @@ GEM nokogiri (1.18.7) mini_portile2 (~> 2.8.2) racc (~> 1.4) + oauth2 (2.0.9) + faraday (>= 0.17.3, < 3.0) + jwt (>= 1.0, < 3.0) + multi_xml (~> 0.5) + rack (>= 1.2, < 4) + snaky_hash (~> 2.0) + version_gem (~> 1.1) omniauth (2.1.3) hashie (>= 3.4.6) rack (>= 2.2.3) @@ -296,6 +309,9 @@ GEM simplecov_json_formatter (~> 0.1) simplecov-html (0.13.1) simplecov_json_formatter (0.1.4) + snaky_hash (2.0.1) + hashie + version_gem (~> 1.1, >= 1.1.1) swd (2.0.3) activesupport (>= 3) attr_required (>= 0.0.5) @@ -312,6 +328,7 @@ GEM validate_url (1.0.15) activemodel (>= 3.0.0) public_suffix + version_gem (1.1.7) webfinger (2.1.3) activesupport faraday (~> 2.0) diff --git a/gemfiles/rails_7.1.gemfile.lock b/gemfiles/rails_7.1.gemfile.lock index 63604d4..ce9da46 100644 --- a/gemfiles/rails_7.1.gemfile.lock +++ b/gemfiles/rails_7.1.gemfile.lock @@ -2,6 +2,7 @@ PATH remote: .. specs: rpi_auth (4.0.0) + oauth2 omniauth-rails_csrf_protection (~> 1.0.0) omniauth_openid_connect (~> 0.7.1) rails (>= 6.1.4) @@ -144,6 +145,8 @@ GEM bindata faraday (~> 2.0) faraday-follow_redirects + jwt (2.10.1) + base64 language_server-protocol (3.17.0.4) lint_roller (1.1.0) listen (3.9.0) @@ -164,6 +167,8 @@ GEM mini_mime (1.1.5) mini_portile2 (2.8.8) minitest (5.25.5) + multi_xml (0.7.1) + bigdecimal (~> 3.1) mutex_m (0.3.0) net-http (0.6.0) uri @@ -180,6 +185,13 @@ GEM nokogiri (1.18.7) mini_portile2 (~> 2.8.2) racc (~> 1.4) + oauth2 (2.0.9) + faraday (>= 0.17.3, < 3.0) + jwt (>= 1.0, < 3.0) + multi_xml (~> 0.5) + rack (>= 1.2, < 4) + snaky_hash (~> 2.0) + version_gem (~> 1.1) omniauth (2.1.3) hashie (>= 3.4.6) rack (>= 2.2.3) @@ -336,6 +348,9 @@ GEM simplecov_json_formatter (~> 0.1) simplecov-html (0.13.1) simplecov_json_formatter (0.1.4) + snaky_hash (2.0.1) + hashie + version_gem (~> 1.1, >= 1.1.1) stringio (3.1.6) swd (2.0.3) activesupport (>= 3) @@ -353,6 +368,7 @@ GEM validate_url (1.0.15) activemodel (>= 3.0.0) public_suffix + version_gem (1.1.7) webfinger (2.1.3) activesupport faraday (~> 2.0) diff --git a/gemfiles/rails_7.2.gemfile.lock b/gemfiles/rails_7.2.gemfile.lock index 9622982..3a79670 100644 --- a/gemfiles/rails_7.2.gemfile.lock +++ b/gemfiles/rails_7.2.gemfile.lock @@ -2,6 +2,7 @@ PATH remote: .. specs: rpi_auth (4.0.0) + oauth2 omniauth-rails_csrf_protection (~> 1.0.0) omniauth_openid_connect (~> 0.7.1) rails (>= 6.1.4) @@ -145,6 +146,8 @@ GEM bindata faraday (~> 2.0) faraday-follow_redirects + jwt (2.10.1) + base64 language_server-protocol (3.17.0.4) lint_roller (1.1.0) listen (3.9.0) @@ -164,6 +167,8 @@ GEM method_source (1.1.0) mini_mime (1.1.5) minitest (5.25.5) + multi_xml (0.7.1) + bigdecimal (~> 3.1) net-http (0.6.0) uri net-imap (0.5.6) @@ -192,6 +197,13 @@ GEM racc (~> 1.4) nokogiri (1.18.7-x86_64-linux-musl) racc (~> 1.4) + oauth2 (2.0.9) + faraday (>= 0.17.3, < 3.0) + jwt (>= 1.0, < 3.0) + multi_xml (~> 0.5) + rack (>= 1.2, < 4) + snaky_hash (~> 2.0) + version_gem (~> 1.1) omniauth (2.1.3) hashie (>= 3.4.6) rack (>= 2.2.3) @@ -348,6 +360,9 @@ GEM simplecov_json_formatter (~> 0.1) simplecov-html (0.13.1) simplecov_json_formatter (0.1.4) + snaky_hash (2.0.1) + hashie + version_gem (~> 1.1, >= 1.1.1) stringio (3.1.6) swd (2.0.3) activesupport (>= 3) @@ -366,6 +381,7 @@ GEM validate_url (1.0.15) activemodel (>= 3.0.0) public_suffix + version_gem (1.1.7) webfinger (2.1.3) activesupport faraday (~> 2.0) diff --git a/rpi_auth.gemspec b/rpi_auth.gemspec index f0f60ad..cae6669 100644 --- a/rpi_auth.gemspec +++ b/rpi_auth.gemspec @@ -21,6 +21,7 @@ Gem::Specification.new do |spec| spec.required_ruby_version = '>= 3.1.0' + spec.add_dependency 'oauth2' spec.add_dependency 'omniauth_openid_connect', '~> 0.7.1' spec.add_dependency 'omniauth-rails_csrf_protection', '~> 1.0.0' spec.add_dependency 'rails', '>= 6.1.4' From 3b44a7cc83920cf278d2e1455ba2cbea3494b5a1 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 13:14:31 +0100 Subject: [PATCH 02/12] Add webmock as development dependency --- gemfiles/rails_6.1.gemfile.lock | 10 ++++++++++ gemfiles/rails_7.0.gemfile.lock | 10 ++++++++++ gemfiles/rails_7.1.gemfile.lock | 10 ++++++++++ gemfiles/rails_7.2.gemfile.lock | 10 ++++++++++ rpi_auth.gemspec | 1 + 5 files changed, 41 insertions(+) diff --git a/gemfiles/rails_6.1.gemfile.lock b/gemfiles/rails_6.1.gemfile.lock index 40f47c5..9489b46 100644 --- a/gemfiles/rails_6.1.gemfile.lock +++ b/gemfiles/rails_6.1.gemfile.lock @@ -90,6 +90,9 @@ GEM xpath (~> 3.2) coderay (1.1.3) concurrent-ruby (1.3.5) + crack (1.0.0) + bigdecimal + rexml crass (1.0.6) date (3.4.1) diff-lcs (1.6.1) @@ -108,6 +111,7 @@ GEM ffi (1.17.1) globalid (1.2.1) activesupport (>= 6.1) + hashdiff (1.1.2) hashie (5.0.0) i18n (1.14.7) concurrent-ruby (~> 1.0) @@ -250,6 +254,7 @@ GEM rb-inotify (0.11.1) ffi (~> 1.0) regexp_parser (2.10.0) + rexml (3.4.1) rspec-core (3.13.3) rspec-support (~> 3.13.0) rspec-expectations (3.13.3) @@ -334,6 +339,10 @@ GEM activesupport faraday (~> 2.0) faraday-follow_redirects + webmock (3.25.1) + addressable (>= 2.8.0) + crack (>= 0.3.2) + hashdiff (>= 0.4.0, < 2.0.0) websocket-driver (0.7.7) base64 websocket-extensions (>= 0.1.0) @@ -359,6 +368,7 @@ DEPENDENCIES rubocop-rails rubocop-rspec simplecov + webmock BUNDLED WITH 2.3.27 diff --git a/gemfiles/rails_7.0.gemfile.lock b/gemfiles/rails_7.0.gemfile.lock index 67f9a86..a9a6668 100644 --- a/gemfiles/rails_7.0.gemfile.lock +++ b/gemfiles/rails_7.0.gemfile.lock @@ -96,6 +96,9 @@ GEM xpath (~> 3.2) coderay (1.1.3) concurrent-ruby (1.3.5) + crack (1.0.0) + bigdecimal + rexml crass (1.0.6) date (3.4.1) diff-lcs (1.6.1) @@ -114,6 +117,7 @@ GEM ffi (1.17.1) globalid (1.2.1) activesupport (>= 6.1) + hashdiff (1.1.2) hashie (5.0.0) i18n (1.14.7) concurrent-ruby (~> 1.0) @@ -256,6 +260,7 @@ GEM rb-inotify (0.11.1) ffi (~> 1.0) regexp_parser (2.10.0) + rexml (3.4.1) rspec-core (3.13.3) rspec-support (~> 3.13.0) rspec-expectations (3.13.3) @@ -333,6 +338,10 @@ GEM activesupport faraday (~> 2.0) faraday-follow_redirects + webmock (3.25.1) + addressable (>= 2.8.0) + crack (>= 0.3.2) + hashdiff (>= 0.4.0, < 2.0.0) websocket-driver (0.7.7) base64 websocket-extensions (>= 0.1.0) @@ -358,6 +367,7 @@ DEPENDENCIES rubocop-rails rubocop-rspec simplecov + webmock BUNDLED WITH 2.3.27 diff --git a/gemfiles/rails_7.1.gemfile.lock b/gemfiles/rails_7.1.gemfile.lock index ce9da46..e1659e7 100644 --- a/gemfiles/rails_7.1.gemfile.lock +++ b/gemfiles/rails_7.1.gemfile.lock @@ -110,6 +110,9 @@ GEM coderay (1.1.3) concurrent-ruby (1.3.5) connection_pool (2.5.0) + crack (1.0.0) + bigdecimal + rexml crass (1.0.6) date (3.4.1) diff-lcs (1.6.1) @@ -129,6 +132,7 @@ GEM ffi (1.17.1) globalid (1.2.1) activesupport (>= 6.1) + hashdiff (1.1.2) hashie (5.0.0) i18n (1.14.7) concurrent-ruby (~> 1.0) @@ -294,6 +298,7 @@ GEM regexp_parser (2.10.0) reline (0.6.1) io-console (~> 0.5) + rexml (3.4.1) rspec-core (3.13.3) rspec-support (~> 3.13.0) rspec-expectations (3.13.3) @@ -373,6 +378,10 @@ GEM activesupport faraday (~> 2.0) faraday-follow_redirects + webmock (3.25.1) + addressable (>= 2.8.0) + crack (>= 0.3.2) + hashdiff (>= 0.4.0, < 2.0.0) websocket-driver (0.7.7) base64 websocket-extensions (>= 0.1.0) @@ -398,6 +407,7 @@ DEPENDENCIES rubocop-rails rubocop-rspec simplecov + webmock BUNDLED WITH 2.3.27 diff --git a/gemfiles/rails_7.2.gemfile.lock b/gemfiles/rails_7.2.gemfile.lock index 3a79670..58affb8 100644 --- a/gemfiles/rails_7.2.gemfile.lock +++ b/gemfiles/rails_7.2.gemfile.lock @@ -104,6 +104,9 @@ GEM coderay (1.1.3) concurrent-ruby (1.3.5) connection_pool (2.5.0) + crack (1.0.0) + bigdecimal + rexml crass (1.0.6) date (3.4.1) diff-lcs (1.6.1) @@ -130,6 +133,7 @@ GEM ffi (1.17.1-x86_64-linux-musl) globalid (1.2.1) activesupport (>= 6.1) + hashdiff (1.1.2) hashie (5.0.0) i18n (1.14.7) concurrent-ruby (~> 1.0) @@ -306,6 +310,7 @@ GEM regexp_parser (2.10.0) reline (0.6.1) io-console (~> 0.5) + rexml (3.4.1) rspec-core (3.13.3) rspec-support (~> 3.13.0) rspec-expectations (3.13.3) @@ -386,6 +391,10 @@ GEM activesupport faraday (~> 2.0) faraday-follow_redirects + webmock (3.25.1) + addressable (>= 2.8.0) + crack (>= 0.3.2) + hashdiff (>= 0.4.0, < 2.0.0) websocket-driver (0.7.7) base64 websocket-extensions (>= 0.1.0) @@ -418,6 +427,7 @@ DEPENDENCIES rubocop-rails rubocop-rspec simplecov + webmock BUNDLED WITH 2.3.27 diff --git a/rpi_auth.gemspec b/rpi_auth.gemspec index cae6669..c57562e 100644 --- a/rpi_auth.gemspec +++ b/rpi_auth.gemspec @@ -37,4 +37,5 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'rubocop-rails' spec.add_development_dependency 'rubocop-rspec' spec.add_development_dependency 'simplecov' + spec.add_development_dependency 'webmock' end From 38b3ec4cf9791315b1282d4b1a606aead87e4bf4 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 13:16:02 +0100 Subject: [PATCH 03/12] Make webmock method available in specs --- spec/spec_helper.rb | 1 + spec/support/webmock.rb | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 spec/support/webmock.rb diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index df435e4..cea613d 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -10,6 +10,7 @@ require_relative 'support/omniauth' require_relative 'support/request_helpers' +require_relative 'support/webmock' require 'rpi_auth/spec_helpers' ENV['RAILS_ENV'] = 'test' diff --git a/spec/support/webmock.rb b/spec/support/webmock.rb new file mode 100644 index 0000000..ebb3576 --- /dev/null +++ b/spec/support/webmock.rb @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +require 'webmock/rspec' From 8bb1808e56bc6dfb53fedc0b15109cdc5df56847 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 13:18:02 +0100 Subject: [PATCH 04/12] Move Authenticatable#attributes out of included block This is more idiomatic use of an `ActiveSupport::Concern`. Instance methods defined on the concern module will be available in the class that includes the concern and the method will also be available in the ancestor chain for overriding and the use of `super`. The `included` block is intended for calling class-level "macros" like `attr_accessor`; not for defining instance methods. --- lib/rpi_auth/models/authenticatable.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/rpi_auth/models/authenticatable.rb b/lib/rpi_auth/models/authenticatable.rb index 75fbf6e..6557696 100644 --- a/lib/rpi_auth/models/authenticatable.rb +++ b/lib/rpi_auth/models/authenticatable.rb @@ -23,11 +23,11 @@ module Authenticatable include ActiveModel::Serialization attr_accessor :user_id, *PROFILE_KEYS + end - # Allow serialization - def attributes - (['user_id'] + PROFILE_KEYS).index_with { |_k| nil } - end + # Allow serialization + def attributes + (['user_id'] + PROFILE_KEYS).index_with { |_k| nil } end class_methods do From 44b40bf0b1fc66b8c1c5ba78073c9a03a131ba61 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 13:22:06 +0100 Subject: [PATCH 05/12] Extract Authenticatable#attribute_keys method So I can override it in modules or classes later in the ancestor chain. --- lib/rpi_auth/models/authenticatable.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/rpi_auth/models/authenticatable.rb b/lib/rpi_auth/models/authenticatable.rb index 6557696..d45c964 100644 --- a/lib/rpi_auth/models/authenticatable.rb +++ b/lib/rpi_auth/models/authenticatable.rb @@ -25,9 +25,13 @@ module Authenticatable attr_accessor :user_id, *PROFILE_KEYS end + def attribute_keys + %w[user_id] + PROFILE_KEYS + end + # Allow serialization def attributes - (['user_id'] + PROFILE_KEYS).index_with { |_k| nil } + attribute_keys.map(&:to_s).index_with { |_k| nil } end class_methods do From d024c3e13ad807d055b974ba18716f0844d6ea26 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 13:26:21 +0100 Subject: [PATCH 06/12] Add explicit require for Controllers::CurrentUser Previously `RpiAuth::Controllers::CurrentUser` (which is in the `lib` directory) was only being required somewhat by accident via `RpiAuth::AuthController` which is required automatically, because it's in the `app/controllers` directory. I think a better solution would be to move the `CurrentUser` concern into the more idiomatic `app/controllers` directory (or perhaps `app/controllera/concerns`) where it would be automatically loaded and there would be no need for an expicit `require`. However, that feels like a bigger piece of work. --- spec/dummy/app/controllers/application_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/dummy/app/controllers/application_controller.rb b/spec/dummy/app/controllers/application_controller.rb index c967eb1..7402251 100644 --- a/spec/dummy/app/controllers/application_controller.rb +++ b/spec/dummy/app/controllers/application_controller.rb @@ -1,3 +1,4 @@ +require 'rpi_auth/controllers/current_user' class ApplicationController < ActionController::Base include RpiAuth::Controllers::CurrentUser end From 42248336d571d85403479dc73e38b8ea6d834050 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 22 Apr 2025 17:32:39 +0100 Subject: [PATCH 07/12] Only use helper_method in CurrentUser if available In `code-club-frontend`, `RpiAuth::Controllers::CurrentUser` is being included into `AuthenticationConcern` which is in turn used in `ApplicationComponent` which is *not* a Rails controller and so doesn't support `AbstractController::Helpers::ClassMethods#helper_method`. --- lib/rpi_auth/controllers/current_user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rpi_auth/controllers/current_user.rb b/lib/rpi_auth/controllers/current_user.rb index fc75779..21129c9 100644 --- a/lib/rpi_auth/controllers/current_user.rb +++ b/lib/rpi_auth/controllers/current_user.rb @@ -6,7 +6,7 @@ module CurrentUser extend ActiveSupport::Concern included do - helper_method :current_user + helper_method :current_user if respond_to?(:helper_method) end def current_user From 2078dcd46d6a329ddaceb08a38ae23a291d138cf Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 13:32:23 +0100 Subject: [PATCH 08/12] Add OauthClient class This has been copied as closely as possible from `code-club-frontend`: - `lib/oauth_client.rb` [1] - `spec/lib/oauth_client_spec.rb` [2] Some things that I've changed: - Moved into the `RpiAuth` namespace. - Use `RpiAuth.configuration.bypass_auth` instead of reading `BYPASS_AUTH` env var directly as per this PR [3] - Use `RpiAuth.configure` in the `before` block instead of using `ClimateControl.modify` in an `around` block. The former is possible, because the `RpiAuth` configuration is reset before every example [4]. [1]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/lib/oauth_client.rb [2]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/main/spec/lib/oauth_client_spec.rb [3]: https://github.com/RaspberryPiFoundation/experience-cs/pull/315 [4]: https://github.com/RaspberryPiFoundation/rpi-auth/blob/e54cc7a30c61cf43874a38c79f1e2f3e2b6d2103/spec/spec_helper.rb#L132-L136 --- lib/rpi_auth/oauth_client.rb | 34 ++++++++++++++++++++ spec/rpi_auth/oauth_client_spec.rb | 51 ++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 lib/rpi_auth/oauth_client.rb create mode 100644 spec/rpi_auth/oauth_client_spec.rb diff --git a/lib/rpi_auth/oauth_client.rb b/lib/rpi_auth/oauth_client.rb new file mode 100644 index 0000000..135b830 --- /dev/null +++ b/lib/rpi_auth/oauth_client.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'oauth2/client' +require 'oauth2/access_token' + +module RpiAuth + class OauthClient + attr_reader :client + + def initialize + @client = OAuth2::Client.new( + RpiAuth.configuration.auth_client_id, + RpiAuth.configuration.auth_client_secret, + site: RpiAuth.configuration.auth_token_url, + token_url: RpiAuth.configuration.token_endpoint.path, + authorize_url: RpiAuth.configuration.authorization_endpoint.path + ) + end + + def refresh_credentials(**credentials) + new_credentials = if RpiAuth.configuration.bypass_auth + credentials.merge(expires_at: 1.hour.from_now) + else + OAuth2::AccessToken.from_hash(client, credentials).refresh.to_hash + end + + { + access_token: new_credentials[:access_token], + refresh_token: new_credentials[:refresh_token], + expires_at: new_credentials[:expires_at].to_i + } + end + end +end diff --git a/spec/rpi_auth/oauth_client_spec.rb b/spec/rpi_auth/oauth_client_spec.rb new file mode 100644 index 0000000..d5c42c7 --- /dev/null +++ b/spec/rpi_auth/oauth_client_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'oauth2' + +RSpec.describe RpiAuth::OauthClient do + include ActiveSupport::Testing::TimeHelpers + + describe '#refresh_credentials' do + subject(:refresh_credentials) { oauth_client.refresh_credentials(**credentials) } + + let(:oauth_client) { described_class.new } + let(:credentials) { { access_token: 'foo', refresh_token: 'bar' } } + let(:refresh_body) { { token: 'baz', refresh_token: 'quux', expires_in: rand(300..600) } } + + before do + RpiAuth.configure do |config| + config.bypass_auth = bypass_auth + config.auth_url = 'https://auth.com:123/' + end + + freeze_time + end + + after do + unfreeze_time + end + + context 'when RpiAuth.configuration.bypass is not set' do + let(:bypass_auth) { false } + + before do + stub_request(:post, RpiAuth.configuration.token_endpoint) + .with(body: { grant_type: 'refresh_token', refresh_token: credentials[:refresh_token] }) + .to_return(status: 200, body: refresh_body.to_json, headers: { content_type: 'application/json' }) + end + + it do + expect(refresh_credentials).to eq({ access_token: refresh_body[:token], + refresh_token: refresh_body[:refresh_token], + expires_at: Time.now.to_i + refresh_body[:expires_in] }) + end + end + + context 'when RpiAuth.configuration.bypass is set' do + let(:bypass_auth) { true } + + it { expect(refresh_credentials).to eq credentials.merge(expires_at: 1.hour.from_now.to_i) } + end + end +end From e517b2f75a11228b62b5a094b5a5a386047ccbd7 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 13:40:28 +0100 Subject: [PATCH 09/12] Add Models::WithTokens concern This can optionally be included into your user model in order to obtain an access token, a refresh token, and an expiry time when logging in. These attributes are set by the same mechanism in `AuthController#callback` that populates `user_id` on the user via `Authenticatable.from_omniauth`, but instead calls `WithTokens.from_omniauth` which in turn calls `Authenticatable.from_omniauth` via the ancestor chain. This also relies on: - `RpiAuth.configuration.scope` including the "offline" scope in the Rails app which is using the `rpi_auth` gem. - In the `profile` app `hydra_client` config for the Rails app, `grant_types` must include "refresh_token" and `scope` must include "offline". Again this has been substantially copied from `code-club-frontend`: - `app/models/user.rb` [1] - `spec/models/user_spec.rb` [2] Some things that I've changed: - Moved methods on `User` into an `RpiAuth` namespaced concern which can be included into the user model in the Rails app. - Override the recently added `Authenticatable#attribute_keys` method, but call `super` to avoid duplication. - I haven't included the aliases for `#id` & `#id=` methods, because it wasn't immediately obvious that these are relevant for projects other than `code-club-frontend`. - Use `RpiAuth.configuration.bypass_auth` instead of reading `BYPASS_AUTH` env var directly as per this PR [3] - Use `RpiAuth.configure` in the `before` block instead of using `ClimateControl.modify` in an `around` block. The former is possible, because the `RpiAuth` configuration is reset before every example [4]. [1]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/app/models/user.rb [2]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/spec/models/user_spec.rb [3]: https://github.com/RaspberryPiFoundation/experience-cs/pull/315 [4]: https://github.com/RaspberryPiFoundation/rpi-auth/blob/e54cc7a30c61cf43874a38c79f1e2f3e2b6d2103/spec/spec_helper.rb#L132-L136 --- .rubocop_todo.yml | 5 +- lib/rpi_auth.rb | 1 + lib/rpi_auth/models/with_tokens.rb | 43 +++++++++++ spec/dummy/app/models/user.rb | 1 + spec/rpi_auth/models/with_tokens_spec.rb | 97 ++++++++++++++++++++++++ 5 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 lib/rpi_auth/models/with_tokens.rb create mode 100644 spec/rpi_auth/models/with_tokens_spec.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 701fff0..2a61f75 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -12,7 +12,7 @@ # Include: **/*.gemspec, **/Gemfile, **/gems.rb Gemspec/DevelopmentDependencies: Exclude: - - 'rpi_auth.gemspec' + - "rpi_auth.gemspec" # Offense count: 1 # Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes. @@ -43,4 +43,5 @@ RSpec/NestedGroups: # Include: **/*_spec.rb RSpec/SpecFilePathFormat: Exclude: - - 'spec/rpi_auth/models/authenticatable_spec.rb' + - "spec/rpi_auth/models/authenticatable_spec.rb" + - "spec/rpi_auth/models/with_tokens_spec.rb" diff --git a/lib/rpi_auth.rb b/lib/rpi_auth.rb index a9a5707..f5a55cd 100644 --- a/lib/rpi_auth.rb +++ b/lib/rpi_auth.rb @@ -4,6 +4,7 @@ require 'rpi_auth/engine' require 'rpi_auth/configuration' require 'rpi_auth/models/authenticatable' +require 'rpi_auth/models/with_tokens' require 'omniauth/rails_csrf_protection' module RpiAuth diff --git a/lib/rpi_auth/models/with_tokens.rb b/lib/rpi_auth/models/with_tokens.rb new file mode 100644 index 0000000..9a86cf0 --- /dev/null +++ b/lib/rpi_auth/models/with_tokens.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'rpi_auth/oauth_client' + +module RpiAuth + module Models + module WithTokens + extend ActiveSupport::Concern + + include Authenticatable + + included do + attr_accessor :access_token, :expires_at, :refresh_token + end + + def attribute_keys + super + %w[access_token expires_at refresh_token user_id] + end + + def refresh_credentials! + oauth_client = OauthClient.new + credentials = oauth_client.refresh_credentials(access_token:, refresh_token:) + + assign_attributes(**credentials) + end + + class_methods do + def from_omniauth(auth) + user = super + return unless user + + if auth.credentials + user.access_token = auth.credentials.token + user.refresh_token = auth.credentials.refresh_token + user.expires_at = Time.now.to_i + auth.credentials.expires_in + end + + user + end + end + end + end +end diff --git a/spec/dummy/app/models/user.rb b/spec/dummy/app/models/user.rb index 5f45bef..e10adfb 100644 --- a/spec/dummy/app/models/user.rb +++ b/spec/dummy/app/models/user.rb @@ -1,3 +1,4 @@ class User include RpiAuth::Models::Authenticatable + include RpiAuth::Models::WithTokens end diff --git a/spec/rpi_auth/models/with_tokens_spec.rb b/spec/rpi_auth/models/with_tokens_spec.rb new file mode 100644 index 0000000..b5cf04b --- /dev/null +++ b/spec/rpi_auth/models/with_tokens_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'spec_helper' + +class DummyUser + include RpiAuth::Models::Authenticatable + include RpiAuth::Models::WithTokens +end + +RSpec.describe DummyUser, type: :model do + include ActiveSupport::Testing::TimeHelpers + + subject(:user) { described_class.new } + + it { is_expected.to respond_to(:access_token) } + it { is_expected.to respond_to(:refresh_token) } + it { is_expected.to respond_to(:expires_at) } + + describe '#refresh_credentials!' do + subject(:refresh_credentials) { user.refresh_credentials! } + + let(:stub_oauth_client) { instance_double(RpiAuth::OauthClient) } + let(:new_tokens) { { access_token: 'foo', refresh_token: 'bar', expires_at: 1.hour.from_now.utc } } + + before do + allow(RpiAuth::OauthClient).to receive(:new).and_return(stub_oauth_client) + allow(stub_oauth_client).to receive(:refresh_credentials).with(access_token: user.access_token, refresh_token: user.refresh_token).and_return(new_tokens) + end + + it { expect { refresh_credentials }.to change(user, :access_token).from(user.access_token).to(new_tokens[:access_token]) } + it { expect { refresh_credentials }.to change(user, :refresh_token).from(user.refresh_token).to(new_tokens[:refresh_token]) } + it { expect { refresh_credentials }.to change(user, :expires_at).from(user.expires_at).to(new_tokens[:expires_at]) } + end + + describe '#from_omniauth' do + subject(:user) { described_class.from_omniauth(auth) } + + let(:omniauth_user) { described_class.new } + let(:info) { omniauth_user.serializable_hash } + let(:credentials) { { token: SecureRandom.base64(12), refresh_token: SecureRandom.base64(12), expires_in: rand(60..240) } } + + let(:auth) do + OmniAuth::AuthHash.new( + { + provider: 'rpi', + uid: omniauth_user.user_id, + credentials:, + extra: { + raw_info: info + } + } + ) + end + + it { is_expected.to be_a described_class } + + it 'sets the access_token' do + expect(user.access_token).to eq credentials[:token] + end + + it 'sets the refresh_token' do + expect(user.refresh_token).to eq credentials[:refresh_token] + end + + context 'when no credentials are returned' do + let(:credentials) { nil } + + it 'sets the access_token to be nil' do + expect(user.access_token).to be_nil + end + end + + it 'sets the expires_at time correctly' do + freeze_time do + expect(user.expires_at).to eq credentials[:expires_in].seconds.from_now.to_i + end + end + + context 'with unusual keys in info' do + let(:info) { { foo: :bar, flibble: :woo } } + + it { is_expected.to be_a described_class } + end + + context 'with no info' do + let(:info) { nil } + + it { is_expected.to be_a described_class } + end + + context 'with no auth set' do + let(:auth) { nil } + + it { is_expected.to be_nil } + end + end +end From fca29abfc50f2aaec9461eea3a74dcf98a83662b Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 14:03:56 +0100 Subject: [PATCH 10/12] Add Controllers::AutoRefreshingToken concern This can optionally be included into your controller to automatically use the user's refresh token to obtain a new access token when the old one expires. It adds a before action to the target controller which automatically calls `OauthClient#refresh_credentials!` if the user is signed in and their access token has expired. Again this has been substantially copied from `code-club-frontend`: - `app/controllers/application_controller.rb` [1] - `spec/requests/refresh_credentials_spec.rb` [2] Some things that I've changed: - Moved functionality from `ApplicationController` into an `RpiAuth` namespaced concern which can be included into the controller in the Rails app. - Make `AutoRefreshingToken#refresh_credentials_if_needed` private since it's not a controller action. - Use the home page in the dummy app in the request spec instead of the home page in `code-club-frontend`. - Construct a user directly in the request spec; there's no user factory in the `rpi_auth` gem. - Set `RpiAuth.configuration.user_model = 'User'` in the `before` block as per the other request specs. - Only call `before_action` if available. In `code-club-frontend`, `RpiAuth::Controllers::CurrentUser` is being included into `AuthenticationConcern` which is in turn used in `ApplicationComponent` which is *not* a Rails controller and so doesn't support `AbstractController::Callbacks::ClassMethods#before_action`. I'm slightly concerned about a few details of the implementation of `AutoRefreshingToken#refresh_credentials_if_needed`: - Rescuing all `ArgumentError` exceptions seems risky, because it's quite a common exception. It would be better to be more specific. - Even rescuing all `OAuth2::Error` exceptions seems a bit broad, although I haven't investigated what this encompasses. - While resetting the session in the event of an exception seems OK security-wise, it probably doesn't result in a great user experience. It would be better to redirect and/or display an error message to the user to explain what's happened. [1]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/app/controllers/application_controller.rb#L8 [2]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/spec/requests/refresh_credentials_spec.rb --- .../controllers/auto_refreshing_token.rb | 30 ++++++ .../app/controllers/application_controller.rb | 3 + .../spec/requests/refresh_credentials_spec.rb | 95 +++++++++++++++++++ 3 files changed, 128 insertions(+) create mode 100644 lib/rpi_auth/controllers/auto_refreshing_token.rb create mode 100644 spec/dummy/spec/requests/refresh_credentials_spec.rb diff --git a/lib/rpi_auth/controllers/auto_refreshing_token.rb b/lib/rpi_auth/controllers/auto_refreshing_token.rb new file mode 100644 index 0000000..aee9a3c --- /dev/null +++ b/lib/rpi_auth/controllers/auto_refreshing_token.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require 'oauth2/error' + +module RpiAuth + module Controllers + module AutoRefreshingToken + extend ActiveSupport::Concern + + include CurrentUser + + included do + before_action :refresh_credentials_if_needed if respond_to?(:before_action) + end + + private + + def refresh_credentials_if_needed + return unless current_user + + return if Time.now.to_i < current_user.expires_at + + current_user.refresh_credentials! + self.current_user = current_user + rescue OAuth2::Error, ArgumentError + reset_session + end + end + end +end diff --git a/spec/dummy/app/controllers/application_controller.rb b/spec/dummy/app/controllers/application_controller.rb index 7402251..9740b68 100644 --- a/spec/dummy/app/controllers/application_controller.rb +++ b/spec/dummy/app/controllers/application_controller.rb @@ -1,4 +1,7 @@ require 'rpi_auth/controllers/current_user' +require 'rpi_auth/controllers/auto_refreshing_token' + class ApplicationController < ActionController::Base include RpiAuth::Controllers::CurrentUser + include RpiAuth::Controllers::AutoRefreshingToken end diff --git a/spec/dummy/spec/requests/refresh_credentials_spec.rb b/spec/dummy/spec/requests/refresh_credentials_spec.rb new file mode 100644 index 0000000..f01a4d5 --- /dev/null +++ b/spec/dummy/spec/requests/refresh_credentials_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require 'oauth2/error' + +RSpec.describe 'Refreshing the auth token', type: :request do + include ActiveSupport::Testing::TimeHelpers + + subject(:request) { get root_path } + + let(:logged_in_text) { 'Log out' } + let(:stub_oauth_client) { instance_double(RpiAuth::OauthClient) } + + before do + freeze_time + allow(RpiAuth::OauthClient).to receive(:new).and_return(stub_oauth_client) + allow(stub_oauth_client).to receive(:refresh_credentials).with(any_args) + RpiAuth.configuration.user_model = 'User' + end + + after do + unfreeze_time + end + + shared_examples 'there is no attempt to renew the token' do + it 'calls refresh_credentials on the oauth client' do + request + expect(stub_oauth_client).not_to have_received(:refresh_credentials) + end + end + + shared_examples 'there is an attempt to renew the token' do + it 'does not call refresh_credentials on the oauth client' do + request + expect(stub_oauth_client).to have_received(:refresh_credentials) + end + end + + shared_examples 'the user is logged in' do + it do + request + expect(response.body).to include(logged_in_text) + end + end + + shared_examples 'the user is logged out' do + it do + request + expect(response.body).not_to include(logged_in_text) + end + end + + context 'when not logged in' do + it_behaves_like 'the user is logged out' + it_behaves_like 'there is no attempt to renew the token' + end + + context 'when logged in' do + let(:user) { User.new(expires_at:) } + + before do + log_in(user:) + end + + context 'when the access token has not expired' do + let(:expires_at) { 10.seconds.from_now } + + it_behaves_like 'the user is logged in' + it_behaves_like 'there is no attempt to renew the token' + end + + context 'when the access token has expired' do + let(:expires_at) { 10.seconds.ago } + + before do + allow(stub_oauth_client).to receive(:refresh_credentials).with(any_args).and_return({ access_token: 'foo', + refresh_token: 'bar', + expires_at: 10.minutes.from_now }) + end + + it_behaves_like 'the user is logged in' + it_behaves_like 'there is an attempt to renew the token' + + context 'when an OAuth error is raised' do + before do + allow(stub_oauth_client).to receive(:refresh_credentials).with(any_args).and_raise(OAuth2::Error.new('blargh')) + end + + it_behaves_like 'the user is logged out' + it_behaves_like 'there is an attempt to renew the token' + end + end + end +end From 3d898261034d691953fc85b929b5e54475f78e0b Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 14:26:33 +0100 Subject: [PATCH 11/12] Add README section for access token behaviour --- README.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/README.md b/README.md index f94804b..c75a833 100644 --- a/README.md +++ b/README.md @@ -187,6 +187,29 @@ class in `config/application.rb`. config.railties_order = [RpiAuth::Engine, :main_app, :all] ``` +### Obtaining an access token for user + +This optional behaviour is useful if your Rails app (which is using this gem) +needs to use a RPF API which required authentication via an OAuth2 access +token. + +Include the `RpiAuth::Models::WithTokens` concern (which depends on the +`RpiAuth::Models::Authenticatable` concern) into your user model in order to +add `access_token`, `refresh_token` & `expires_at` attributes. These methods +are automatically populated by `RpiAuth::AuthController#callback` via the +`RpiAuth::Models::WithTokens.from_omniauth` method. + +This also relies on the following: +- `RpiAuth.configuration.scope` including the "offline" scope in the Rails app + which is using the `rpi_auth` gem. +- In the `profile` app `hydra_client` config for the Rails app, `grant_types` + must include "refresh_token" and `scope` must include "offline". + +Include the `RpiAuth::Controllers::AutoRefreshingToken` concern (which depends +on the `RpiAuth::Controllers::CurrentUser` concern) into your controller so +that when the user's access token expires, a new one is obtained using the +user's refresh token. + ## Test helpers and routes There are some standardised test helpers in `RpiAuth::SpecHelpers` that can be used when testing. From 2a54a8dfad405a5ae062aa6aad4129bd7217dbc6 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 17 Apr 2025 14:29:18 +0100 Subject: [PATCH 12/12] Add CHANGELOG entry for new behaviour --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e2e9fd..495acf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- Add access token-related functionality including auto-refresh (#83) ### Fixed - Fix use of `User#expires_at` in `SpecHelpers#stub_auth_for` (#82)