diff --git a/app/controllers/concerns/saml_idp_logout_concern.rb b/app/controllers/concerns/saml_idp_logout_concern.rb index 0d2aede6236..9e696a6e6b3 100644 --- a/app/controllers/concerns/saml_idp_logout_concern.rb +++ b/app/controllers/concerns/saml_idp_logout_concern.rb @@ -23,6 +23,8 @@ def handle_valid_sp_logout_request def handle_valid_sp_remote_logout_request(user_id) # Remotely delete the user's current session + # mattw: Leaving this unchanged. If a user somehow tries to log out of a provider they aren't consented for + # (revoked?), we should still let them log out. session_id = ServiceProviderIdentity. find_by(user_id: user_id, service_provider: saml_request.issuer). rails_session_id diff --git a/app/controllers/concerns/verify_sp_attributes_concern.rb b/app/controllers/concerns/verify_sp_attributes_concern.rb index 65db253fbd0..559d41ae349 100644 --- a/app/controllers/concerns/verify_sp_attributes_concern.rb +++ b/app/controllers/concerns/verify_sp_attributes_concern.rb @@ -27,6 +27,7 @@ def update_verified_attributes ) end + # maw: come back and look at this some more. def consent_has_expired?(sp_session_identity) return false unless sp_session_identity return false if sp_session_identity.deleted_at.present? diff --git a/app/forms/openid_connect_token_form.rb b/app/forms/openid_connect_token_form.rb index af10144c77f..2a3bef869f7 100644 --- a/app/forms/openid_connect_token_form.rb +++ b/app/forms/openid_connect_token_form.rb @@ -70,6 +70,7 @@ def url_options def find_identity_with_code return if code.blank? || code.include?("\x00") + # mattw: I suspect we want to change this one @identity = ServiceProviderIdentity.where(session_uuid: code). order(updated_at: :desc).first end diff --git a/app/forms/security_event_form.rb b/app/forms/security_event_form.rb index 0b0e983d310..8d41341ea1f 100644 --- a/app/forms/security_event_form.rb +++ b/app/forms/security_event_form.rb @@ -279,6 +279,7 @@ def identity_from_agency_identity ) end + # Not changing; this is inbound RISC calls def identity_from_identity ServiceProviderIdentity.find_by( uuid: event.dig('subject', 'sub'), diff --git a/app/jobs/reports/sp_user_counts_report.rb b/app/jobs/reports/sp_user_counts_report.rb index 9e65818b727..ae4edfaabce 100644 --- a/app/jobs/reports/sp_user_counts_report.rb +++ b/app/jobs/reports/sp_user_counts_report.rb @@ -57,7 +57,7 @@ def track_users_linked_to_sps(ial:, event:) transaction_with_timeout do track_report_data_event( event, - count: ServiceProviderIdentity.where(ial: ial).select(:user_id).distinct.count, + count: ServiceProviderIdentity.consented.where(ial: ial).select(:user_id).distinct.count, ) end end diff --git a/app/models/service_provider.rb b/app/models/service_provider.rb index c63d6681366..537a280128b 100644 --- a/app/models/service_provider.rb +++ b/app/models/service_provider.rb @@ -5,6 +5,7 @@ class ServiceProvider < ApplicationRecord belongs_to :agency # rubocop:disable Rails/HasManyOrHasOneDependent + # maw: Do we want to filter this? has_many :identities, inverse_of: :service_provider_record, foreign_key: 'service_provider', primary_key: 'issuer', diff --git a/app/models/service_provider_identity.rb b/app/models/service_provider_identity.rb index 7c93219416d..0d2599de07f 100644 --- a/app/models/service_provider_identity.rb +++ b/app/models/service_provider_identity.rb @@ -15,6 +15,7 @@ class ServiceProviderIdentity < ApplicationRecord # rubocop:enable Rails/InverseOf scope :not_deleted, -> { where(deleted_at: nil) } + scope :consented, -> { where.not(last_consented_at: nil) } CONSENT_EXPIRATION = 1.year diff --git a/app/services/access_token_verifier.rb b/app/services/access_token_verifier.rb index 8a0ed0410d5..88b59027269 100644 --- a/app/services/access_token_verifier.rb +++ b/app/services/access_token_verifier.rb @@ -32,6 +32,7 @@ def validate_access_token end def load_identity(access_token) + # mattw: Should we change this one? identity = ServiceProviderIdentity.where(access_token: access_token).take if identity && OutOfBandSessionAccessor.new(identity.rails_session_id).ttl.positive? diff --git a/app/services/agency_identity_linker.rb b/app/services/agency_identity_linker.rb index 28467bff9c8..bb3b9c174d9 100644 --- a/app/services/agency_identity_linker.rb +++ b/app/services/agency_identity_linker.rb @@ -17,6 +17,7 @@ def self.for(user:, service_provider:) ai = AgencyIdentity.where(user: user, agency: agency).take return ai if ai.present? + # mattw: Punting on this one for the moment, because this is where we're going to make our change I think. spi = ServiceProviderIdentity.where( user: user, service_provider: service_provider.issuer, ).take @@ -32,6 +33,7 @@ def self.sp_identity_from_uuid_and_sp(uuid, service_provider) else { uuid: uuid, service_provider: service_provider } end + # mattw: Same, but I think we should change htis one ServiceProviderIdentity.where(criteria).take end diff --git a/app/services/data_requests/lookup_user_by_uuid.rb b/app/services/data_requests/lookup_user_by_uuid.rb index e8fb4baef49..58d4633bd90 100644 --- a/app/services/data_requests/lookup_user_by_uuid.rb +++ b/app/services/data_requests/lookup_user_by_uuid.rb @@ -7,8 +7,9 @@ def initialize(uuid) end def call + # mattw: This seems like a confusing use case. Used only in data_requests.rake FWIW. User.find_by(uuid: uuid) || - ServiceProviderIdentity.find_by(uuid: uuid)&.user + ServiceProviderIdentity.consented.find_by(uuid: uuid)&.user end end end diff --git a/app/services/identity_linker.rb b/app/services/identity_linker.rb index 665297f01c9..37cbbda7150 100644 --- a/app/services/identity_linker.rb +++ b/app/services/identity_linker.rb @@ -7,21 +7,44 @@ def initialize(user, service_provider) @ial = nil end - def link_identity(**extra_attrs) + def link_identity( + code_challenge: nil, + ial: nil, + nonce: nil, + rails_session_id: nil, + scope: nil, + verified_attributes: nil, + last_consented_at: nil, + clear_deleted_at: nil, + include_identity_session_attributes: true + ) return unless user && service_provider.present? - process_ial(extra_attrs) - attributes = merged_attributes(extra_attrs) - identity.update!(attributes) + process_ial(ial, include_identity_session_attributes: include_identity_session_attributes) + + identity.update!( + (include_identity_session_attributes ? identity_attributes : {}).merge( + code_challenge: code_challenge, + ial: ial, + nonce: nonce, + rails_session_id: rails_session_id, + scope: scope, + verified_attributes: combined_verified_attributes(verified_attributes), + ).tap do |hash| + hash[:last_consented_at] = last_consented_at if last_consented_at + hash[:deleted_at] = nil if clear_deleted_at + end, + ) + AgencyIdentityLinker.new(identity).link_identity identity end private - def process_ial(extra_attrs) - @ial = extra_attrs[:ial] + def process_ial(ial, include_identity_session_attributes:) + @ial = ial now = Time.zone.now - process_ial_at(now) + process_ial_at(now) if include_identity_session_attributes process_verified_at(now) end @@ -42,14 +65,11 @@ def identity @identity ||= find_or_create_identity_with_costing end + # mattw: What does this have to do with costing? def find_or_create_identity_with_costing user.identities.create_or_find_by(service_provider: service_provider.issuer) end - def merged_attributes(extra_attrs) - identity_attributes.merge(optional_attributes(**extra_attrs)) - end - def identity_attributes { last_authenticated_at: Time.zone.now, @@ -58,31 +78,7 @@ def identity_attributes } end - def optional_attributes( - code_challenge: nil, - ial: nil, - nonce: nil, - rails_session_id: nil, - scope: nil, - verified_attributes: nil, - last_consented_at: nil, - clear_deleted_at: nil - ) - { - code_challenge: code_challenge, - ial: ial, - nonce: nonce, - rails_session_id: rails_session_id, - scope: scope, - verified_attributes: merge_attributes(verified_attributes), - }.tap do |hash| - hash[:last_consented_at] = last_consented_at if last_consented_at - hash[:deleted_at] = nil if clear_deleted_at - end - end - - def merge_attributes(verified_attributes) - verified_attributes = verified_attributes.to_a.map(&:to_s) - (identity.verified_attributes.to_a + verified_attributes).uniq.sort + def combined_verified_attributes(verified_attributes) + [*identity.verified_attributes, verified_attributes.to_a.map(&:to_s)].uniq.sort end end diff --git a/app/services/push_notification/http_push.rb b/app/services/push_notification/http_push.rb index 7744fb757b4..04fc8ee32ed 100644 --- a/app/services/push_notification/http_push.rb +++ b/app/services/push_notification/http_push.rb @@ -19,6 +19,8 @@ def initialize(event, now: Time.zone.now) def deliver return unless IdentityConfig.store.push_notifications_enabled + # mattw: I think we should change this but this breaks tests, possibly because + # the test calls user.identities and we need to change that still? event.user. service_providers. merge(ServiceProviderIdentity.not_deleted). @@ -91,7 +93,8 @@ def agency_uuid(service_provider) user_id: event.user.id, agency_id: service_provider.agency_id, )&.uuid || - ServiceProviderIdentity.find_by( + # This changes breaks nothing + ServiceProviderIdentity.consented.find_by( user_id: event.user.id, service_provider: service_provider.issuer, )&.uuid diff --git a/app/services/uuid_reporter.rb b/app/services/uuid_reporter.rb index c42311397cf..7ce2cf84c3c 100644 --- a/app/services/uuid_reporter.rb +++ b/app/services/uuid_reporter.rb @@ -100,6 +100,7 @@ def collect_identities(agency, emails_to_user_ids) # Note that we use two separate queries since the inner joins don't take # advantage of the composite indexes and are highly non-performant. actual_user_ids = emails_to_user_ids.values.select(&:present?) + # mattw: We likely want to change this, but this breaks tests a lot. user_ids_with_identities = ServiceProviderIdentity. where(user_id: actual_user_ids, service_provider: issuers). pluck(:user_id) diff --git a/spec/factories/service_provider_identities.rb b/spec/factories/service_provider_identities.rb index 3e43c2e7622..bddf9ed25b6 100644 --- a/spec/factories/service_provider_identities.rb +++ b/spec/factories/service_provider_identities.rb @@ -2,14 +2,16 @@ factory :service_provider_identity do uuid { SecureRandom.uuid } service_provider { 'https://serviceprovider.com' } + last_consented_at { Time.zone.now - 5.minutes } end + # mattw: Is 'active' meaningful to us here? I think not. trait :active do last_authenticated_at { Time.zone.now } end - trait :consented do - last_consented_at { Time.zone.now - 5.minutes } + trait :non_consented do + last_consented_at { nil } end trait :soft_deleted_5m_ago do diff --git a/spec/jobs/reports/sp_user_counts_report_spec.rb b/spec/jobs/reports/sp_user_counts_report_spec.rb index 4d818298ad1..887e91c7571 100644 --- a/spec/jobs/reports/sp_user_counts_report_spec.rb +++ b/spec/jobs/reports/sp_user_counts_report_spec.rb @@ -12,9 +12,28 @@ it 'logs to analytics' do ServiceProvider.create(issuer: issuer, friendly_name: issuer, app_id: app_id) - ServiceProviderIdentity.create(user_id: 1, service_provider: issuer, uuid: 'foo2', ial: 1) - ServiceProviderIdentity.create(user_id: 2, service_provider: issuer, uuid: 'foo3', ial: 1) - ServiceProviderIdentity.create(user_id: 3, service_provider: issuer, uuid: 'foo4', ial: 2) + # maw: Look at making these use a factory but this is maybe a lot of work right now + ServiceProviderIdentity.create( + user_id: 1, + service_provider: issuer, + uuid: 'foo2', + ial: 1, + last_consented_at: Time.zone.now, + ) + ServiceProviderIdentity.create( + user_id: 2, + service_provider: issuer, + uuid: 'foo3', + ial: 1, + last_consented_at: Time.zone.now, + ) + ServiceProviderIdentity.create( + user_id: 3, + service_provider: issuer, + uuid: 'foo4', + ial: 2, + last_consented_at: Time.zone.now, + ) freeze_time do timestamp = Time.zone.now.iso8601 expect(subject).to receive(:write_hash_to_reports_log).with( @@ -55,11 +74,17 @@ it 'returns the total user counts per sp broken down by ial1 and ial2' do ServiceProvider.create(issuer: issuer, friendly_name: issuer, app_id: app_id) - ServiceProviderIdentity.create(user_id: 1, service_provider: issuer, uuid: 'foo1') - ServiceProviderIdentity.create(user_id: 2, service_provider: issuer, uuid: 'foo2') + ServiceProviderIdentity.create( + user_id: 1, service_provider: issuer, uuid: 'foo1', + last_consented_at: Time.zone.now + ) + ServiceProviderIdentity.create( + user_id: 2, service_provider: issuer, uuid: 'foo2', + last_consented_at: Time.zone.now + ) ServiceProviderIdentity.create( user_id: 3, service_provider: issuer, uuid: 'foo3', - verified_at: Time.zone.now + verified_at: Time.zone.now, last_consented_at: Time.zone.now ) result = [{ issuer: issuer, total: 3, ial1_total: 2, ial2_total: 1, app_id: app_id }].to_json diff --git a/spec/models/service_provider_identity_spec.rb b/spec/models/service_provider_identity_spec.rb index 2aa20e21fcd..28f0e8b80bd 100644 --- a/spec/models/service_provider_identity_spec.rb +++ b/spec/models/service_provider_identity_spec.rb @@ -6,8 +6,10 @@ ServiceProviderIdentity.create( user_id: user.id, service_provider: 'externalapp', + last_consented_at: Time.zone.now, ) end + subject { identity } it { is_expected.to belong_to(:user) } @@ -23,6 +25,23 @@ end end + # maw: It's not customary to test scopes and this should probably go away. + # Just using this while I work to prove that nothing silly is happening. + describe '.consented scope' do + let!(:consented_user) { create(:service_provider_identity) } + let!(:non_consented_user) { create(:service_provider_identity, :non_consented) } + + subject { described_class.consented } + + it 'includes users with last_consented_at set' do + expect(subject).to include(consented_user) + end + + it 'excludes users without last_consented_at set' do + expect(subject).to_not include(non_consented_user) + end + end + describe 'uuid validations' do it 'uses a DB constraint to enforce presence' do identity = create(:service_provider_identity) diff --git a/spec/services/data_requests/lookup_user_by_uuid_spec.rb b/spec/services/data_requests/lookup_user_by_uuid_spec.rb index 598878c944a..e66aebb995f 100644 --- a/spec/services/data_requests/lookup_user_by_uuid_spec.rb +++ b/spec/services/data_requests/lookup_user_by_uuid_spec.rb @@ -12,11 +12,22 @@ end context 'when an identity exists with the UUID' do - it 'returns the user for the identity' do - identity = create(:service_provider_identity) - uuid = identity.uuid + context 'when it has consented for the SP' do + it 'returns the user for the identity' do + identity = create(:service_provider_identity) + uuid = identity.uuid - expect(described_class.new(uuid).call).to eq(identity.user) + expect(described_class.new(uuid).call).to eq(identity.user) + end + end + + context 'when it has not consented for the SP' do + it 'does not return the identity for the user' do + identity = create(:service_provider_identity, :non_consented) + uuid = identity.uuid + + expect(described_class.new(uuid).call).to be_nil + end end end diff --git a/spec/services/identity_linker_spec.rb b/spec/services/identity_linker_spec.rb index 346b73cb011..2c2ca0501aa 100644 --- a/spec/services/identity_linker_spec.rb +++ b/spec/services/identity_linker_spec.rb @@ -26,6 +26,20 @@ expect(identity_attributes).to include new_attributes end + it 'does not write last_authenticated_at when :include_identity_session_attributes is false' do + identity = IdentityLinker.new(user, service_provider). + link_identity(include_identity_session_attributes: false) + + aggregate_failures do + expect(identity.last_authenticated_at).to be_nil + expect(identity.session_uuid).to be_nil + expect(identity.access_token).to be_nil + expect(identity.last_ial1_authenticated_at).to be_nil + expect(identity.last_ial2_authenticated_at).to be_nil + expect(identity.verified_at).to be_nil + end + end + it 'can take an additional optional attributes' do rails_session_id = SecureRandom.hex nonce = SecureRandom.hex