Skip to content

Avoid extraneous read for SpMetadataInSession after saving #7644

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.

Sign up for GitHub

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

Merged
mitchellhenke merged 1 commit into from
Jan 17, 2023
Merged

Avoid extraneous read for SpMetadataInSession after saving #7644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions app/services/service_provider_request_handler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,12 @@ def call
pull_request_id_from_current_sp_session_id

delete_sp_request_if_session_has_matching_request_id
ServiceProviderRequestProxy.create!(attributes)

StoreSpMetadataInSession.new(session: session, request_id: request_id).call
service_provider_request = ServiceProviderRequestProxy.create!(attributes)
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separate topic, but while we're here: I think that it's probably time to combine ServiceProviderRequest and ServiceProviderRequestProxy. The proxy is leftover from a migration from an AR model to a redis model, and now that we only have the redis one, we could probably simplify

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, we can probably pull the classes together since ServiceProviderRequest is basically a struct at this point.


StoreSpMetadataInSession.new(session: session, request_id: request_id).call(
service_provider_request: service_provider_request,
)
end

private
Expand Down
4 changes: 2 additions & 2 deletions app/services/store_sp_metadata_in_session.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ def initialize(session:, request_id:)
@request_id = request_id
end

def call
Rails.logger.info(event_attributes)
def call(service_provider_request: nil)
@sp_request = service_provider_request if service_provider_request

return if sp_request.is_a?(NullServiceProviderRequest)

Expand Down
34 changes: 0 additions & 34 deletions spec/services/store_sp_metadata_in_session_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,15 @@
describe '#call' do
context 'when a ServiceProviderRequestProxy is not found' do
it 'does not set the session[:sp] hash' do
allow(Rails.logger).to receive(:info)
app_session = {}
instance = StoreSpMetadataInSession.new(session: app_session, request_id: 'foo')
info_hash = {
event: 'StoreSpMetadataInSession',
request_id_present: true,
sp_request_class: 'NullServiceProviderRequest',
}.to_json

expect { instance.call }.to_not change(app_session, :keys)
expect(Rails.logger).to have_received(:info).with(info_hash)
end
end

context 'when a ServiceProviderRequestProxy is found' do
it 'sets the session[:sp] hash' do
allow(Rails.logger).to receive(:info)

app_session = {}
request_id = SecureRandom.uuid
ServiceProviderRequestProxy.find_or_create_by(uuid: request_id) do |sp_request|
Expand All @@ -32,12 +23,6 @@
end
instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id)

info_hash = {
event: 'StoreSpMetadataInSession',
request_id_present: true,
sp_request_class: 'ServiceProviderRequest',
}.to_json

app_session_hash = {
issuer: 'issuer',
aal_level_requested: nil,
Expand All @@ -52,15 +37,12 @@
}

instance.call
expect(Rails.logger).to have_received(:info).with(info_hash)
expect(app_session[:sp]).to eq app_session_hash
end
end

context 'when IAL2 and AAL3 are requested' do
it 'sets the session[:sp] hash' do
allow(Rails.logger).to receive(:info)

app_session = {}
request_id = SecureRandom.uuid
ServiceProviderRequestProxy.find_or_create_by(uuid: request_id) do |sp_request|
Expand All @@ -72,12 +54,6 @@
end
instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id)

info_hash = {
event: 'StoreSpMetadataInSession',
request_id_present: true,
sp_request_class: 'ServiceProviderRequest',
}.to_json

app_session_hash = {
issuer: 'issuer',
aal_level_requested: 3,
Expand All @@ -92,15 +68,12 @@
}

instance.call
expect(Rails.logger).to have_received(:info).with(info_hash)
expect(app_session[:sp]).to eq app_session_hash
end
end

context 'when IAL2 and phishing-resistant are requested' do
it 'sets the session[:sp] hash' do
allow(Rails.logger).to receive(:info)

app_session = {}
request_id = SecureRandom.uuid
ServiceProviderRequestProxy.find_or_create_by(uuid: request_id) do |sp_request|
Expand All @@ -112,12 +85,6 @@
end
instance = StoreSpMetadataInSession.new(session: app_session, request_id: request_id)

info_hash = {
event: 'StoreSpMetadataInSession',
request_id_present: true,
sp_request_class: 'ServiceProviderRequest',
}.to_json

app_session_hash = {
issuer: 'issuer',
aal_level_requested: 2,
Expand All @@ -132,7 +99,6 @@
}

instance.call
expect(Rails.logger).to have_received(:info).with(info_hash)
expect(app_session[:sp]).to eq app_session_hash
end
end
Expand Down