-
Notifications
You must be signed in to change notification settings - Fork 111
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
Remove slo from saml metadata #10453
Conversation
f918fab
to
c73f6da
Compare
config/application.yml.default
Outdated
@@ -496,7 +495,7 @@ production: | |||
s3_report_bucket_prefix: login-gov.reports | |||
s3_report_public_bucket_prefix: login-gov-pubdata | |||
s3_reports_enabled: true | |||
saml_endpoint_configs: '[]' | |||
saml_endpoint_configs: '[{"suffix":"2024","secret_key_passphrase":"trust-but-verify"},{"suffix":"2023","secret_key_passphrase":"trust-but-verify","comment":"this extra year is needed to demonstrate how handling multiple live years works in spec/requests/saml_requests_spec.rb"}]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] is this a testing artifact or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is needed only because the review app uses the production
config. I have included it as a separate commit that will be dropped before the merge.
@@ -156,7 +156,6 @@ in_person_outage_expected_update_date: 'October 31, 2024' | |||
in_person_outage_emailed_by_date: 'November 1, 2024' | |||
in_person_send_proofing_notifications_enabled: false | |||
in_person_stop_expiring_enrollments: false | |||
include_slo_in_saml_metadata: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[thought] i think we can't drop the config value until after the code change is deployed. 50-50 state deployment is affected by the config -- can you check to see if we need to keep this in here for deployment, and then have another PR to clean it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% confident, but I think it'd be okay, since the old boxes would still have both the config value and code paths using the config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can't be 100% confident, I am OK with breaking it up into 2 deploys. Is there a way we can get to 100% confidence, perhaps by looping in other engineers? This also indicates the need to document the details of how production configuration works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Digging into this further, I agree with @aduth that the each instance would either have the config value and code that uses it, or neither. In theory, we should be good. What do you all think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't know anything about how this works, so if you both are reasonably confident, i'm happy to trust you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just hopping on to confirm, dropping a default config value in a PR is safe, because as previously stated, each box has its own in-memory copy of the config.
Removing the value from the S3 in the deployed env before this code is deployed: unsafe (because we may need to roll back, then the re-deployed old code would look for configs that aren't there)
**Why**: - Including the SingleLogoutService endpoints with an empty Location attribute caused problems for some partners when validating the metadata **How**: - The saml_idp gem was updated to not include the SingleLogoutService endpoints in the metadata meant to change, and so that only one database call is made. - The `include_slo_in_saml_metadata` flag was not serving any real purpose after this change and was removed changelog: User-Facing Improvements, Metadata, Remove SingleLogoutService from SAML metadata
88dcc5c
to
bca4ee7
Compare
No description provided.