add an 'auto-logout toggle' (take 3 - proper way) #448
Conversation
twrichards
force-pushed
the
auto-logout-proper
branch
3 times, most recently
from
63f68db
to
2e4dc26
Compare
| @@ -98,7 +100,20 @@ class Janus( | |||
| ) | |||
| loginUrl = Federation.loginUrl(credentials, host, stsClient) | |||
| } yield { | |||
| SeeOther(loginUrl) | |||
| val redirectUrl = request.cookies.get("autoLogout") match { | |||
There was a problem hiding this comment.
I'd love to see this logic be tested, which would also give us the opportunity to enforce some of the things you've discovered (like the us-east-1 constrain). Could this logic move to the logic package, perhaps we can create a new object in there if there isn't a good existing candidate already. With this as a function we can write some tests that takes the "state" (currently from the cookie but see below) and confirm that we have returned the correct URL. If we do that, let's focus on the properties we're checking with a helpfully named test for each? I'll be happy to look at this with you if you'd like?
Additionally, I wonder whether this logic could be part of the work Federation.loginUrl achieves. That function returns the login URL, so it might make sense for it to take the "logout state" and return the correct URL based on that? This function performs side-effects (Amazon provide the signed console link from the credentials) so we'd still want this logic to be available elsewhere and tested, it's just a question of where it gets called. I think having the loginUrl on line 101 be the answer from this controller's point of view would be a little clearer, but let me know what you think!
| .split(";") | ||
| .find(_ => _.trim().startsWith(`${COOKIE__AUTO_LOGOUT}=true`)); | ||
| autoLogoutSwitchElement.onchange = (event) => { | ||
| document.cookie = `${COOKIE__AUTO_LOGOUT}=${event.target.checked}; expires=Fri, 31 Dec 9999 23:59:59 GMT; path=/` |
There was a problem hiding this comment.
Would it be simpler to add a parameter to the consoleLogin action, rather than have us synchronise the reading and writing of this cookie? We could let the UI choose the most effective way to collect and persist the user's preference, and have that be communicated explicitly when it talks to the server?
In this case, that might mean sticking to localStorage for the preference and including a querystring parameter or optional path parameter to request a "logout-first" console URL?
Side note: The "favourites" feature does use a cookie so there is some prioir art here! However, this is because the favourites feature needs to change the way the homepage renders on the server. The favourites feature is inherently a piece of server-state, so the choice there is infra-side state via a database or to have that state be client-persisted-server-available via a cookie.
There was a problem hiding this comment.
I think this is a great approach. Really nice catch that this becomes possible if we stick to using us-east-1, that's such a weird detail that unlocks doing this in a very sensible way! 🤪
I've added a few comments about the code, happy to chat them over with you! The only other thing is that it'd be gerat to double check that we're good to use the redirect_uri parameter for this kind of secret information. Generally we see the world moving towards using Headers instead of QS params for this kind of sensitive information. I think this is the established way to do console login type things with AWS (the getSigninToken stuff works a similar way already) so I expect it's fine, but let's chat to AWS to make sure before merging this. I'll want to hear that AWS themselves are treating this parameter the same way they'd treat API keys in general.
Yeah I agree, although there's no guarantee AWS won't change their internals and start logging it where they shouldn't (if perhaps a team were interested in where people were redirecting too, I could picture it coming out on a dashboard somewhere)... according to the docs they support POST... |
|
This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days |
… AWS Console links] the server redirects to `https://us-east-1.signin.aws.amazon.com/oauth?Action=logout` with a `redirect_uri` query param containing the federated login link Janus would normally redirect to directly (see https://serverfault.com/a/1097528)
twrichards
force-pushed
the
auto-logout-proper
branch
from
2e4dc26
to
997b4d1
Compare
|
Commenting to remove stale label, since apparently I can't the normal way. @adamnfish I don't recall seeing any reply on support case. I'm not working for 6 weeks, I know you said you might add some commits on this branch/PR? |
Looking to achieve the same as #439 and #447 were trying to do, but achieving it server-side, if toggle is active (persisted via cookie now, so server can see the preference on the request) [for AWS Console buttons] the server redirects to
https://us-east-1.signin.aws.amazon.com/oauth?Action=logoutwith aredirect_uriquery param containing the federated login link Janus would normally redirect to directly (see https://serverfault.com/a/1097528).✅ TESTED the logout with
redirect_uriapproach manually in the browser console...... obviously with
REDACTEDbeing a real token (which I got by observing/copying from thefederationlink via the Network tab of browser developer tools)