-
Notifications
You must be signed in to change notification settings - Fork 6
Handle ExpiredTokenException correctly and restructure flow paths #150
Conversation
This will accomodate all possible MaxSession lengths that AWS could have
This emits clearer and more deterministic exceptions if the AWS STS call fails
* get_id_token was a method that did far more than getting an id token. This change explodes the functionality in that method into multiple methods. * `get_id_token` * `validate_id_token` * `get_role_map` * `exchange_token_for_credentials` * `print_output` * Add checking for when AWS responds with ExpiredTokenException and initiate a new auth flow to fetch a new ID token. * This would come into play when we have a cached ID token that is older than the AWS undocumented max ID token validity of 1 day * This means that listener.py can now initiate a redirect for the user to start the auth flow over * This includes a new `state` value called "restart_auth" that triggers the redirect to the IdP to restart the auth flow * Break out login path from 2 to 3 paths * Previously when going through the login method the user would either * Initiate an IdP auth flow if either the ID token was invalid or if the ID token was totally valid but the user hadn't asserted a role_arn * This meant that each time a user was running the tool and not passing a role_arn, there was an unnecessary IdP auth flow slowing things down (if there was a local cached valid ID token) * Skip the IdP auth flow if the user had both a cached ID token *and* had passed a role_arn as a command line argument * Now there are 3 paths * We have a cached ID token and the role was passed as an argument * We have a cached ID token but the role passed on the command line wasn't valid. Show the role picker * Either the cached ID token was invalid or missing and we need to get a new one, or the user passed no role_arn on the command line so we need to spawn the role picker * Move token and id_token_dict from ephemeral variables into attributes of the Login class. This allows for persisting these values.
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.
Random comments, mostly looks great.
logger.error( | ||
'AWS STS Call failed {status} {Type} {Code} : {Message}'.format( | ||
status=resp.status_code, **error)) | ||
if (error['Code'] == 'ValidationError' |
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.
Can we just key off of Code == ValidationError
? I'm sure it's not a 1:1 mapping, but if it's the only thing in our code that triggers it then it seems fine?
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.
No, since there are other things that can cause a ValidationError
besides this max duration issue. If we did, we could detect other, non-duration-related, validation errors and start our duration backoff which would be wrong.
Handle ExpiredTokenException correctly and restructure flow paths
This change explodes the functionality in that method into multiple
methods.
get_id_token
validate_id_token
get_role_map
exchange_token_for_credentials
print_output
initiate a new auth flow to fetch a new ID token.
older than the AWS undocumented max ID token validity of 1 day
to start the auth flow over
state
value called "restart_auth" that triggersthe redirect to the IdP to restart the auth flow
if the ID token was totally valid but the user hadn't asserted a
role_arn
passing a role_arn, there was an unnecessary IdP auth flow slowing
things down (if there was a local cached valid ID token)
had passed a role_arn as a command line argument
wasn't valid. Show the role picker
to get a new one, or the user passed no role_arn on the command
line so we need to spawn the role picker
the Login class. This allows for persisting these values.
Fixes #146
Fixes #141