-
Notifications
You must be signed in to change notification settings - Fork 166
LG-7058 | IrsAttemptsApi::Tracker sets up base parameters #6664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
880dbc1
f3042be
b7aa0a1
3c100a2
5224547
4f3f48e
1c1f3c6
695d5a0
6444d82
0e2e861
0b61cbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,22 @@ def link_identity | |
| AgencyIdentity.new(user_id: @sp_identity.user_id, uuid: @sp_identity.uuid) | ||
| end | ||
|
|
||
| # @return [AgencyIdentity, ServiceProviderIdentity] the AgencyIdentity for this user at this | ||
| # service provider or falls back to the ServiceProviderIdentity if one does not exist. | ||
| def self.for(user:, service_provider:) | ||
| agency = service_provider.agency | ||
|
|
||
| ai = AgencyIdentity.where(user: user, agency: agency).take | ||
| return ai if ai.present? | ||
|
|
||
| spi = ServiceProviderIdentity.where( | ||
| user: user, service_provider: service_provider.issuer, | ||
| ).take | ||
|
|
||
| return nil unless spi.present? | ||
| new(spi).link_identity | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would love a version of this method that is read-only and doesn't attempt to link? But I'm not sure that would work here, for the AttemptsAPI.... since usually the ID is only created after an account is created + consented to? Maybe we put in a pin and circle back
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was unclear on some of the existing semantics here. Just to be clear, are you suggesting to leave this as-is for now but consider circling back on this later? Or are you saying that this is will prematurely create these links and should be changed now?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry that was a mess of a comment. Let's leave this as-is |
||
| end | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works and is in keeping with some of the existing patterns, but it feels a bit ugly to me. |
||
|
|
||
| def self.sp_identity_from_uuid_and_sp(uuid, service_provider) | ||
| ai = AgencyIdentity.where(uuid: uuid).take | ||
| criteria = if ai | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,29 +6,29 @@ | |
| before(:each) { init_env(user) } | ||
|
|
||
| it 'links identities from 2 sps' do | ||
| sp1 = create_identity(user, 'http://localhost:3000', 'UUID1') | ||
| create_identity(user, 'urn:gov:gsa:openidconnect:test', 'UUID2') | ||
| sp1 = create_service_provider_identity(user, 'http://localhost:3000', 'UUID1') | ||
| create_service_provider_identity(user, 'urn:gov:gsa:openidconnect:test', 'UUID2') | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I renamed this which created a lot of noise. |
||
| ai = AgencyIdentityLinker.new(sp1).link_identity | ||
| expect(ai.uuid).to eq('UUID1') | ||
| ai = AgencyIdentity.where(user_id: user.id).first | ||
| expect(ai.uuid).to eq('UUID1') | ||
| end | ||
|
|
||
| it 'does not allow 2 sp uuids to be reused after user deletes account' do | ||
| create_identity(user, 'http://localhost:3000', 'UUID1') | ||
| create_identity(user, 'urn:gov:gsa:openidconnect:test', 'UUID2') | ||
| create_service_provider_identity(user, 'http://localhost:3000', 'UUID1') | ||
| create_service_provider_identity(user, 'urn:gov:gsa:openidconnect:test', 'UUID2') | ||
| user.destroy! | ||
| expect(User.where(id: user.id).count).to eq(0) | ||
| user2 = create(:user) | ||
| expect { create_identity(user2, 'sp3', 'UUID1') }. | ||
| expect { create_service_provider_identity(user2, 'sp3', 'UUID1') }. | ||
| to raise_error ActiveRecord::RecordNotUnique | ||
| expect { create_identity(user2, 'sp4', 'UUID2') }. | ||
| expect { create_service_provider_identity(user2, 'sp4', 'UUID2') }. | ||
| to raise_error ActiveRecord::RecordNotUnique | ||
| end | ||
|
|
||
| it 'does not allow agency_identity uuid to be reused after user deletes account' do | ||
| sp1 = create_identity(user, 'http://localhost:3000', 'UUID1') | ||
| create_identity(user, 'urn:gov:gsa:openidconnect:test', 'UUID2') | ||
| sp1 = create_service_provider_identity(user, 'http://localhost:3000', 'UUID1') | ||
| create_service_provider_identity(user, 'urn:gov:gsa:openidconnect:test', 'UUID2') | ||
| AgencyIdentityLinker.new(sp1).link_identity | ||
| expect(AgencyIdentity.where(user_id: user.id).count).to eq(1) | ||
| expect(AgencyIdentity.where(uuid: 'UUID1').count).to eq(1) | ||
|
|
@@ -37,22 +37,22 @@ | |
| expect(AgencyIdentity.where(user_id: user.id).count).to eq(0) | ||
| expect(AgencyIdentity.where(uuid: 'UUID1').count).to eq(0) | ||
| user2 = create(:user) | ||
| expect { create_identity(user2, 'sp3', 'UUID1') }. | ||
| expect { create_service_provider_identity(user2, 'sp3', 'UUID1') }. | ||
| to raise_error ActiveRecord::RecordNotUnique | ||
| expect { create_identity(user2, 'sp4', 'UUID2') }. | ||
| expect { create_service_provider_identity(user2, 'sp4', 'UUID2') }. | ||
| to raise_error ActiveRecord::RecordNotUnique | ||
| end | ||
|
|
||
| it 'links identity with 1 sp' do | ||
| sp1 = create_identity(user, 'http://localhost:3000', 'UUID1') | ||
| sp1 = create_service_provider_identity(user, 'http://localhost:3000', 'UUID1') | ||
| ai = AgencyIdentityLinker.new(sp1).link_identity | ||
| expect(ai.uuid).to eq('UUID1') | ||
| ai = AgencyIdentity.where(user_id: user.id).first | ||
| expect(ai.uuid).to eq('UUID1') | ||
| end | ||
|
|
||
| it 'does not link identity without an agency_id' do | ||
| sp1 = create_identity(user, 'sp:with:no:agency_id', 'UUID1') | ||
| sp1 = create_service_provider_identity(user, 'sp:with:no:agency_id', 'UUID1') | ||
| ai = AgencyIdentityLinker.new(sp1).link_identity | ||
| expect(ai.agency_id).to eq(nil) | ||
| expect(ai.uuid).to eq('UUID1') | ||
|
|
@@ -61,7 +61,7 @@ | |
| end | ||
|
|
||
| it 'returns the existing agency_identity if it exists' do | ||
| sp1 = create_identity(user, 'http://localhost:3000', 'UUID1') | ||
| sp1 = create_service_provider_identity(user, 'http://localhost:3000', 'UUID1') | ||
| ai = AgencyIdentityLinker.new(sp1).link_identity | ||
| expect(ai.uuid).to eq('UUID1') | ||
| ai = AgencyIdentity.where(user_id: user.id).first | ||
|
|
@@ -73,7 +73,7 @@ | |
| before(:each) { init_env(user) } | ||
|
|
||
| it 'returns sp_identity if it exists' do | ||
| create_identity(user, 'http://localhost:3000', 'UUID1') | ||
| create_service_provider_identity(user, 'http://localhost:3000', 'UUID1') | ||
| AgencyIdentity.create(user_id: user.id, agency_id: 1, uuid: 'UUID2') | ||
| sp_identity = AgencyIdentityLinker.sp_identity_from_uuid_and_sp( | ||
| 'UUID2', | ||
|
|
@@ -92,12 +92,60 @@ | |
| end | ||
| end | ||
|
|
||
| describe '#for' do | ||
| before(:each) { init_env(user) } | ||
| let(:sp) { create(:service_provider) } | ||
| let(:agency) { sp.agency } | ||
| let(:uuid) { SecureRandom.uuid } | ||
|
|
||
| subject { described_class.for(user: user, service_provider: sp) } | ||
|
|
||
| context 'when there is already an agency identity' do | ||
| before { create_agency_identity(user, agency, uuid) } | ||
|
|
||
| it 'returns the existing agency identity' do | ||
| expect(subject).not_to be_nil | ||
| expect(subject.user_id).to eq user.id | ||
| expect(subject.agency_id).to eq agency.id | ||
| expect(subject.uuid).to eq uuid | ||
| end | ||
| end | ||
|
|
||
| context 'when there is not an agency identity' do | ||
| context 'but there is a service provider identity' do | ||
| before { create_service_provider_identity(user, sp.issuer, uuid) } | ||
| it 'returns the service provider identity' do | ||
| expect(subject).not_to be_nil | ||
| expect(subject.user_id).to eq user.id | ||
| expect(subject.agency_id).to eq agency.id | ||
| expect(subject.uuid).to eq uuid | ||
| end | ||
|
|
||
| it 'persists the service provider identity as an agency identity' do | ||
| expect(subject.uuid).to eq uuid | ||
| ai = AgencyIdentity.where(user: user, agency: agency).take | ||
| expect(subject).to eq ai | ||
| end | ||
| end | ||
|
|
||
| context 'and there is no service provider identity' do | ||
| it 'returns nil' do | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this seem like reasonable behavior? |
||
| expect(subject).to be_nil | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def init_env(user) | ||
| ServiceProviderIdentity.where(user_id: user.id).delete_all | ||
| AgencyIdentity.where(user_id: user.id).delete_all | ||
| end | ||
|
|
||
| def create_identity(user, service_provider, uuid) | ||
| def create_service_provider_identity(user, service_provider, uuid) | ||
| ServiceProviderIdentity.create(user_id: user.id, service_provider: service_provider, uuid: uuid) | ||
| end | ||
|
|
||
| def create_agency_identity(user, agency, uuid) | ||
| AgencyIdentity.create!(user: user, agency: agency, uuid: uuid) | ||
| end | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only used in one place, but I wanted to follow the pattern of the existing |
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,13 +5,30 @@ | |
| allow(IdentityConfig.store).to receive(:irs_attempt_api_enabled).and_return( | ||
| irs_attempt_api_enabled, | ||
| ) | ||
| allow(request).to receive(:user_agent).and_return('example/1.0') | ||
| allow(request).to receive(:remote_ip).and_return('192.0.2.1') | ||
| end | ||
|
|
||
| let(:irs_attempt_api_enabled) { true } | ||
| let(:session_id) { 'test-session-id' } | ||
| let(:enabled_for_session) { true } | ||
| let(:request) { instance_double(ActionDispatch::Request) } | ||
| let(:service_provider) { create(:service_provider) } | ||
| let(:device_fingerprint) { 'device_id' } | ||
| let(:sp_request_uri) { 'https://example.com/auth_page' } | ||
| let(:user) { create(:user) } | ||
|
|
||
| subject { described_class.new(session_id: session_id, enabled_for_session: enabled_for_session) } | ||
| subject do | ||
| described_class.new( | ||
| session_id: session_id, | ||
| request: request, | ||
| user: user, | ||
| sp: service_provider, | ||
| device_fingerprint: device_fingerprint, | ||
| sp_request_uri: sp_request_uri, | ||
| enabled_for_session: enabled_for_session, | ||
| ) | ||
| end | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear, this isn't testing anything new, just passing in everything now needed. Trying to reason about here a good test for this should go, though... I feel like there should probably be an end-to-end test somewhere for this. |
||
|
|
||
| describe '#track_event' do | ||
| it 'records the event in redis' do | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.