diff --git a/app/controllers/device_authorizations_controller.rb b/app/controllers/device_authorizations_controller.rb index 3d84a61..92b6fde 100644 --- a/app/controllers/device_authorizations_controller.rb +++ b/app/controllers/device_authorizations_controller.rb @@ -87,14 +87,14 @@ class DeviceAuthorizationsController < ApplicationController end def record_consent(device_code, user) - consent = OidcUserConsent.find_or_initialize_by(user: user, application: device_code.application) - # Merge into any existing consent instead of overwriting it. The consent - # record is shared with the browser flow (unique on user+application) and - # scopes_granted is treated as a granted superset, so a narrower device - # request must not shrink previously granted scopes or wipe stored claims. - consent.scopes = consent.scopes_granted.to_s.split | granted_scopes(device_code) - consent.claims_requests = {} if consent.new_record? - consent.granted_at = Time.current - consent.save! + # merge: true — the consent record is shared with the browser flow, so a + # narrower device request must not shrink previously granted scopes or wipe + # stored claims. + OidcUserConsent.record!( + user: user, + application: device_code.application, + scopes: granted_scopes(device_code), + merge: true + ) end end diff --git a/app/models/oidc_user_consent.rb b/app/models/oidc_user_consent.rb index f6031e3..868ac27 100644 --- a/app/models/oidc_user_consent.rb +++ b/app/models/oidc_user_consent.rb @@ -8,9 +8,29 @@ class OidcUserConsent < ApplicationRecord before_validation :set_granted_at, on: :create before_validation :set_sid, on: :create - # Parse scopes_granted into an array + # Upsert a user's consent for an application. The record is unique on + # user+application and shared across the browser and device flows. + # + # merge: false (the browser consent screen) records exactly the scopes the user + # just approved. merge: true (device approval) unions the scopes into any + # existing grant and leaves stored claims untouched, so a narrower device + # request can never shrink a prior grant or wipe its claims. claims_requests is + # written only when supplied (nil = keep whatever is stored, defaulting to {} + # for a brand-new record). + def self.record!(user:, application:, scopes:, claims_requests: nil, merge: false) + consent = find_or_initialize_by(user: user, application: application) + incoming = Array(scopes) + consent.scopes = merge ? (consent.scopes | incoming) : incoming + consent.claims_requests = claims_requests unless claims_requests.nil? + consent.claims_requests ||= {} + consent.granted_at = Time.current + consent.save! + consent + end + + # Parse scopes_granted into an array (nil-safe for not-yet-saved records). def scopes - scopes_granted.split(" ") + scopes_granted.to_s.split(" ") end # Set scopes from an array diff --git a/test/models/oidc_user_consent_test.rb b/test/models/oidc_user_consent_test.rb index b7a4e4f..00e121e 100644 --- a/test/models/oidc_user_consent_test.rb +++ b/test/models/oidc_user_consent_test.rb @@ -206,6 +206,45 @@ class OidcUserConsentTest < ActiveSupport::TestCase assert consent.covers_scopes?(["openid", "email", "profile"]) end + # --- .record! upsert semantics -------------------------------------------- + + test "record! creates a new consent with the given scopes and claims" do + user = users(:alice) + app = applications(:another_app) + OidcUserConsent.where(user: user, application: app).delete_all + + consent = OidcUserConsent.record!(user: user, application: app, + scopes: %w[openid email], claims_requests: {"userinfo" => {"email" => nil}}) + + assert_equal %w[openid email], consent.scopes + assert_equal({"userinfo" => {"email" => nil}}, consent.parsed_claims_requests) + end + + test "record! without merge overwrites scopes (browser flow)" do + user = users(:alice) + app = applications(:another_app) + OidcUserConsent.create!(user: user, application: app, + scopes_granted: "openid email profile", granted_at: 1.day.ago) + + consent = OidcUserConsent.record!(user: user, application: app, scopes: %w[openid], + claims_requests: {}) + + assert_equal %w[openid], consent.scopes, "browser flow records exactly what was consented" + end + + test "record! with merge unions scopes and preserves stored claims (device flow)" do + user = users(:alice) + app = applications(:another_app) + OidcUserConsent.create!(user: user, application: app, + scopes_granted: "openid email profile", claims_requests: {"userinfo" => {"email" => nil}}, + granted_at: 1.day.ago) + + consent = OidcUserConsent.record!(user: user, application: app, scopes: %w[openid], merge: true) + + assert_equal %w[openid email profile].sort, consent.scopes.sort, "merge must not shrink a prior grant" + assert_equal({"userinfo" => {"email" => nil}}, consent.parsed_claims_requests, "merge must not wipe claims") + end + test "should validate scope coverage logic with real OIDC scenarios" do # Typical OIDC consent scenario @consent.scopes_granted = "openid profile email"