Mattw/lg 8058 default scope by n1zyy · Pull Request #7418 · 18F/identity-idp

n1zyy · 2022-12-01T19:50:40Z

DO NOT MERGE THIS until the IdentitiesBackfillJob has completed and we've ensured that there are no rows in prod where deleted_at IS NULL AND last_consented at IS NULL. (At the moment this is a crude WIP anyway, but the warning remains when this is finished.)

And also basic test / factory update changelog: Internal, Attempts API, ServiceProviderIdentity has consented scope

This introduces linter error on too-long comments that won't stay, so ignore them for now.

app/controllers/concerns/saml_idp_logout_concern.rb

zachmargolis · 2022-12-01T20:08:58Z

app/controllers/concerns/verify_sp_attributes_concern.rb

    )
  end

+  # maw: come back and look at this some more.


now that we have backfilled the whole table, we can drop the last_estimated_consent || logic entirely

zachmargolis · 2022-12-01T20:10:19Z

app/forms/openid_connect_token_form.rb

  def find_identity_with_code
    return if code.blank? || code.include?("\x00")

+    # mattw: I suspect we want to change this one


zachmargolis · 2022-12-01T20:10:48Z

app/services/access_token_verifier.rb


  def load_identity(access_token)
+    # mattw: Should we change this one?
    identity = ServiceProviderIdentity.where(access_token: access_token).take


yes change this one

zachmargolis · 2022-12-01T20:14:04Z

app/services/agency_identity_linker.rb

                 { uuid: uuid, service_provider: service_provider }
               end
+    # mattw: Same, but I think we should change htis one
    ServiceProviderIdentity.where(criteria).take


yes yes change this

zachmargolis · 2022-12-01T20:15:02Z

app/services/data_requests/lookup_user_by_uuid.rb

+      # 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


im thinking maybe we take this out for better reporting, because the agency already has the UUID

Oh! Good catch.

There's a certain myopia that seeps in when one hyperfocuses on this stuff, it seems.

zachmargolis · 2022-12-01T20:15:27Z

app/services/identity_linker.rb

    @identity ||= find_or_create_identity_with_costing
  end

+  # mattw: What does this have to do with costing?


it probably use to have a costing method but I bet we removed it, maybe we should rename this

zachmargolis · 2022-12-01T20:16:50Z

app/services/push_notification/http_push.rb

+      # 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).


Suggested change

# 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).

event.user.

service_providers.

merge(ServiceProviderIdentity.not_deleted).

merge(ServiceProviderIdentity.consented).

yes definitely should be here, we need to fix the tests

mitchellhenke · 2022-12-01T20:23:55Z

app/models/service_provider_identity.rb

  # rubocop:enable Rails/InverseOf

  scope :not_deleted, -> { where(deleted_at: nil) }
+  scope :consented, -> { where.not(last_consented_at: nil) }


I'm generally against it since it can be kind of surprising, but is this a situation for default_scope that defaults to consented identities?

The risks involved are not small, and I don't think we should commit to using it long-term, but the column is a big shift in the assumptions we can make about an identity.

This was actually my first instinct, but a previous project I was on ran into a whole lot of trouble that ultimately convinced me it was evil.

The idea of a short-term default scope is a little tempting, though, especially with some urgency around this. What say ye, @zachmargolis?

I've been bitten by default_scope... I think our uses cases here are mixed enough that we don't want to use that here

I don't have any strong opinions on "short-term" but will say that I feel like saying something is short term doesn't guarantee we'll actually remove it quickly 😂

Yeah, I'm mostly looking for safeguards to not treat users that have not consented as though they have

zachmargolis · 2022-12-02T17:28:00Z

app/forms/security_event_form.rb

    )
  end

+  # Not changing; this is inbound RISC calls


let's remove this comment? since it's useful in the scope of this PR, but as a permanent comment, not really

Suggested change

# Not changing; this is inbound RISC calls

it's useful in the scope of this PR, but as a permanent comment, not really

I was planning on leaving these comments in while I work, and then pulling them all out when I take this out of draft status. Right now it's a note-to-self as I go through these. Agree 💯 that it should come out before this lands, though.

zachmargolis · 2022-12-02T17:30:06Z

app/services/push_notification/http_push.rb

      )&.uuid ||
-        ServiceProviderIdentity.find_by(
+        # This changes breaks nothing
+        ServiceProviderIdentity.consented.find_by(
          user_id: event.user.id,
          service_provider: service_provider.issuer,
        )&.uuid


honestly this code could probably be refactored to use AgencyIdentity.for instead of redoing the logic here

mitchellhenke

Still taking a look into this, will revisit once we've exhausted other options!

mitchellhenke · 2022-12-05T21:32:36Z

app/models/service_provider_identity.rb

  # rubocop:enable Rails/InverseOf

  scope :not_deleted, -> { where(deleted_at: nil) }
+  scope :consented, -> { where.not(last_consented_at: nil) }


Dropping a "requested change" as something of a formality while we re-evaluate this set of changes internally

zachmargolis · 2022-12-30T17:56:30Z

Closing this out since we're relying on session id/TID instead

n1zyy added 3 commits November 30, 2022 18:23

LG-8058 | Adds ServiceProviderIdentity scope

f795c00

And also basic test / factory update changelog: Internal, Attempts API, ServiceProviderIdentity has consented scope

Various fixes

ab88891

This introduces linter error on too-long comments that won't stay, so ignore them for now.

Merge branch 'main' into mattw/LG-8058_default_scope

95a4a3e

zachmargolis reviewed Dec 1, 2022

View reviewed changes

mitchellhenke reviewed Dec 1, 2022

View reviewed changes

Some user.identities fixes

13d8b56

n1zyy added the status - do not merge label Dec 1, 2022

Some more cleanup + a little breakage

a0aa40f

This was referenced Dec 2, 2022

Remove some indirection in IdentityLinker #7420

Merged

Add option to include_identity_session_attributes in IdentityLinker #7421

Closed

zachmargolis reviewed Dec 2, 2022

View reviewed changes

Miscellaneous changes; breaks some tests to look at on Monday.

292b5d2

mitchellhenke suggested changes Dec 5, 2022

View reviewed changes

zachmargolis closed this Dec 30, 2022

aduth deleted the mattw/LG-8058_default_scope branch January 3, 2023 15:12

Conversation

n1zyy commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchellhenke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented Dec 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

n1zyy commented Dec 1, 2022 •

edited

Loading