ATO-1248: Add AuthSession To UserContext #5590
Conversation
ghost
force-pushed
the
ATO-1248/Add-AuthSession-To-UserContext
branch
9 times, most recently
from
e3e522c to
df51ed0
Compare
ghost
marked this pull request as ready for review
There was a problem hiding this comment.
Have looked through the first few of the handlers and most of them don't have the read permissions for the auth session table. All the handlers which inherit from the BaseFrontendHandler will need this as it is querying the table to check if there is an auth session.
Also would it be possible to maybe break this PR up into several commits which apply the changes to each lambda. Just makes this PR less scary to review 😄
ghost
force-pushed
the
ATO-1248/Add-AuthSession-To-UserContext
branch
6 times, most recently
from
74e34f9 to
566d2c1
Compare
| @@ -45,6 +49,10 @@ public Session getSession() { | |||
| return session; | |||
| } | |||
|
|
|||
| public AuthSessionItem getAuthSession() { | |||
| return authSession.orElseThrow(() -> new RuntimeException("Auth session not set.")); | |||
There was a problem hiding this comment.
In what circumstance would this happen? We're trying no to throw exceptions but rather return http error responses, because exceptions cause cold starts.
There was a problem hiding this comment.
This is a good question. I actually made a mistake and was calling buildUserContext with a null auth session from StartHandler instead of optional.empty (fixed now). From the other handlers, it will always be present as the base frontend handler returns a http error if the auth session isn't present. The reason for making it optional is really just a stopgap, as if you look in the main handle method in StartHandler, the auth session is constructed with upliftRequired as a parameter, but upliftRequired is computed off of the user context, which otherwise requires an auth session. The construction loop only effects this handler and having discussed it with Bill we thought it's something to be considered in a future refactor. So in summary, this should never be thrown in practise.
ghost
force-pushed
the
ATO-1248/Add-AuthSession-To-UserContext
branch
from
566d2c1 to
5bba2f4
Compare
|
| @@ -13,6 +13,7 @@ module "frontend_api_account_interventions_role" { | |||
| aws_iam_policy.dynamo_client_registry_read_access_policy.arn, | |||
| aws_iam_policy.redis_parameter_policy.arn, | |||
| module.oidc_txma_audit.access_policy_arn, | |||
| aws_iam_policy.dynamo_auth_session_read_policy.arn, | |||
There was a problem hiding this comment.
This (and all new policies) needs to be added at the end of the list of policies in auth's roles. If the order is changed, the role is rewritten while live (not ideal infrastructure), meaning we'd have brief downtime on this lambda while it's permissions were updated. Concat can take multiple arguments, so it's best to have this added after the ticf_cri_lambda_invocation_policy just below.
ghost
force-pushed
the
ATO-1248/Add-AuthSession-To-UserContext
branch
from
5bba2f4 to
70799fb
Compare
What
Added
AuthSessiontoUserContext. The oldSessionlives on theUserContextso this will help us switch over to theAuthSessionwith less refactoring.How to review
Code Review
Notes
The auth session has been added as an optional field for the time being. This is related to how the auth session is currently initialised/updated in the start handler where the user context is used to construct one of the inputs. With a bit of refactoring we should be able to make it non-optional once the start handler is no longer dependant on the session.