Skip to content

ATO-1565: remove orch setters #6265

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Ryan-Andrews99
Copy link
Contributor

@Ryan-Andrews99 Ryan-Andrews99 commented Apr 1, 2025

Wider context of change:

We previously migrated this field to the separate session stores for both Auth and Orch. However we continued to set the value in orchestration and left in logging which checked if the values were in sync. We've stopped reading the value in auth and we only get this in Orch in a few places:

  • To log comparison with the Orch session
  • To check if null before setting
  • One usage in auditing in the authorisation handler

Once we remove these we can stop setting in Authentication.

What’s changed:

  • Swaps the audited value in the authorisation handler with the Orch session value
  • Removes comparison logging
  • Removes setting this value in Auth callback
  • Removes setting this value in Auth code response generation service
  • Removes setting in Auth code handler

Manual testing:

  • Deployed to dev and ran through an uplift journey - no issues

Checklist

  • Lambdas have correct permissions for the resources they're accessing.
  • Impact on orch and auth mutual dependencies has been checked.
  • Changes have been made to contract tests or not required.
  • Changes have been made to the simulator or not required.
  • Changes have been made to stubs or not required.
  • Successfully deployed to authdev or not required.
  • Successfully run Authentication acceptance tests against sandpit or not required.

Related PRs

@Ryan-Andrews99 Ryan-Andrews99 force-pushed the ATO-1565-remove-orch-setters branch 3 times, most recently from aa78550 to 9c568de Compare April 7, 2025 10:16
@Ryan-Andrews99 Ryan-Andrews99 force-pushed the ATO-1565-remove-orch-setters branch 2 times, most recently from 76e4f86 to da35f74 Compare April 17, 2025 14:17
@Ryan-Andrews99 Ryan-Andrews99 marked this pull request as ready for review April 25, 2025 10:59
@Ryan-Andrews99 Ryan-Andrews99 requested review from a team as code owners April 25, 2025 10:59
Copy link
Contributor Author

@Ryan-Andrews99 Ryan-Andrews99 left a comment

Choose a reason for hiding this comment

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

Deployed to dev and ran through an uplift journey - no issues

Removes the null-ing out of this value when the shared session is copied
for max age handling.
This test was added as part of this commit: 037f9b2
which added behaviour for a mismatch in bsid cookie and the session value.
The previous name was also inaccurate due to the fact that the request included
bsid cookie. I think this test was supposed to test the mismatch, and with subequent
changes has become a bit more inaccurate. This updates the tests to be a bit more
specific to the mismatch case.
@Ryan-Andrews99 Ryan-Andrews99 force-pushed the ATO-1565-remove-orch-setters branch from da35f74 to 103cd19 Compare April 29, 2025 13:05
@isaac-GDS
Copy link
Contributor

LGTM!

@isaac-GDS
Copy link
Contributor

Approved - but understanding look into some discrepancies between the shared and orch session for CCS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants