Add option to include_identity_session_attributes in IdentityLinker

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

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?

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

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'),

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

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',

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
 

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?

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
 

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

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

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

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)

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

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
 

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)

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
 

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