Skip to content

Web: Redirect to login upon missing session cookie#33726

Merged
kimlisa merged 2 commits intomasterfrom
lisa/logout-on-missing-cookie
Oct 23, 2023
Merged

Web: Redirect to login upon missing session cookie#33726
kimlisa merged 2 commits intomasterfrom
lisa/logout-on-missing-cookie

Conversation

@kimlisa
Copy link
Copy Markdown
Contributor

@kimlisa kimlisa commented Oct 20, 2023

fixes #20996

Why: We use non-persistent (session) cookie. When you close a browser, session cookies get cleared from the browser. Then when you re-open browser, load the teleport app, try to make a teleport api call to a protected endpoint WithAuth, it fails to authn because a cookie wasn't sent with the request.

There is three kind of dialogue shown depending on which webapp route you take.

AppLauncher component catches error (from trying to getFqdn)

Reproduce (chrome, but works on any browser):

  1. login to teleport and successfully launch an app
  2. close the browser
  3. reopen browser and launch the app (copy and paste app URL)

image

Enterprise: the WaitingRoom catches error (from trying to getUserContext)

  1. login to teleport
  2. close the browser
  3. reopen browser and go to some teleport URL
image

Open source: the root CatchError catches error (from trying to getUserPreferences)

image

Our component tree looks like this:

<CatchError> // our root error catcher if the childrens did not  handle it
  <Authenticated>  // <----- where we should be checking validity of cookie and session
    <AppLauncher/>
    <WaitingRoom>   // only in enterprise
        {... childrens subjected to waiting room}
    </WaitingRoom>
  </Authenticated>
</CatchError>

Prior to this PR, in the Authenticated layer we were only checking for valid session stored in local storage. But gives false positives. Now we just make the api call to the /status endpoint right away and fail earlier in the tree (instead of 15 seconds later from starting an interval timer that polls this endpoint)

All the above scenarios, the user will be redirected to the login right away. On rare occassions where the /status just fails to fetch, this is what the user will see:

image

Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

LGTM, I left a few minor comments.

</DialogHeader>
<DialogContent>
<Alert kind="danger" children={errMsg} />
<Text mb={3}>Try again by refreshing the browser.</Text>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Text mb={3}>Try again by refreshing the browser.</Text>
<Text mb={3}>Try again by refreshing the page.</Text>


import history from 'teleport/services/history';

export function ErrorDialogue({ errMsg }: { errMsg: string }) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ErrorDialog?

const { attempt, setAttempt } = useAttempt('processing');

useEffect(() => {
const checkUserIsAuthenticated = async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const checkUserIsAuthenticated = async () => {
const checkIfUserIsAuthenticated = async () => {

Comment on lines +52 to +57
if (!session.isValid()) {
logger.warn('invalid session');
session.clear();
history.goToLogin(true);
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can move it above try.

}

return <>{children}</>;
return null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

WDYT of showing an <Indicator /> during loading?

Comment on lines +208 to +209
* which just checks if the cookie sent from browser is valid and user
* session is still valid.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd shorten it a bit:

Suggested change
* which just checks if the cookie sent from browser is valid and user
* session is still valid.
* which checks if the cookie and the user session are still valid.

@kimlisa kimlisa enabled auto-merge October 23, 2023 06:56
@kimlisa kimlisa added this pull request to the merge queue Oct 23, 2023
@kimlisa kimlisa removed this pull request from the merge queue due to a manual request Oct 23, 2023
@kimlisa kimlisa force-pushed the lisa/logout-on-missing-cookie branch from 6233ed8 to 84c5cee Compare October 23, 2023 07:06
@kimlisa kimlisa enabled auto-merge October 23, 2023 07:08
@kimlisa kimlisa added this pull request to the merge queue Oct 23, 2023
Merged via the queue into master with commit 794773c Oct 23, 2023
@kimlisa kimlisa deleted the lisa/logout-on-missing-cookie branch October 23, 2023 07:30
@public-teleport-github-review-bot
Copy link
Copy Markdown

@kimlisa See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Access Denied - missing session cookie

3 participants