Skip to content
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

Add Session context and redirects #9908

Merged

Conversation

marshmalien
Copy link
Member

@marshmalien marshmalien commented Apr 14, 2021

SUMMARY

Issues:
#8272 #9197 #8155 #9311

  • When user is logged out due to inactivity, redirect to the Login Page with a warning alert stating that their session expired.
  • When a user logs in on one tab, redirect from login page to home on the other tabs.
  • Clear session state when it expires and/or when a user logs out.
  • Redirect users to direct link destination after authentication

@AlexSCorey is looking at possibly rolling #9915 (SSO login redirect override URL is not working) into this PR

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • UI

session

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

)} seconds due to inactivity.`
)}
<Trans>You will be logged out in</Trans> {sessionCountdown}{' '}
<Trans>seconds due to inactivity.</Trans>
Copy link
Contributor

@nixocio nixocio Apr 15, 2021

Choose a reason for hiding this comment

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

Based on another open PR t macro will be preferred syntax to select translation. See: #9897

Copy link
Member

Choose a reason for hiding this comment

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

I think we're OK with <Trans> (and in fact I prefer it in some cases) -- though the full sentence including the sessionCountdown should appear inside one <Trans> so it can all be translated together

Copy link
Member

Choose a reason for hiding this comment

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

This also needs plural because when we get to 1 left we want You will be logged out in 1 second due to inactivity

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Updated timeout message to use Plural macro.

return context;
}

// export default SessionContext;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// export default SessionContext;
// export default SessionContext;
Suggested change
// export default SessionContext;

@nixocio
Copy link
Contributor

nixocio commented Apr 15, 2021

I triggered an e2e test run.

Copy link
Contributor

@nixocio nixocio left a comment

Choose a reason for hiding this comment

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

I tested this code with different scenarios, and all of them worked. I like the usage of context API to this solution. I left minor comments. If e2e tests are green 🚢

}, [history, sessionTimeout]);

const handleSessionContinue = useCallback(async () => {
MeAPI.read();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MeAPI.read();
await MeAPI.read();

clearInterval(sessionIntervalId.current);
}, []);

const value = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@jakemcdermott jakemcdermott left a comment

Choose a reason for hiding this comment

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

👍

@jakemcdermott
Copy link
Contributor

@AlexSCorey is looking at possibly rolling #9915 (SSO login redirect override URL is not working) into this PR

@marshmalien

That sounds good to me (there's probably overlap with that issue and this PR). If we end up moving that work to this PR I'll take another look.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

clearInterval(sessionIntervalId.current);
}, []);

const value = useMemo(
Copy link
Member

Choose a reason for hiding this comment

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

Why the useMemo here?

@tiagodread
Copy link
Contributor

To test:

  • Redirect to edit form with direct link (or any other link)
  • Redirect to the job output with direct link
  • Redirect to 404 when the resource was deleted

@tiagodread tiagodread mentioned this pull request Apr 19, 2021
@unlikelyzero
Copy link

worth connecting #5278 (comment)

@AlexSCorey
Copy link
Member

@AlexSCorey is looking at possibly rolling #9915 (SSO login redirect override URL is not working) into this PR

@marshmalien

That sounds good to me (there's probably overlap with that issue and this PR). If we end up moving that work to this PR I'll take another look.

I have decided not to roll my SSO work into this PR. Both PRs work in Login.jsx but besides that there isn't much overlap in logic

@marshmalien marshmalien force-pushed the session-work branch 2 times, most recently from 61e0ece to 29f81c1 Compare April 20, 2021 20:34
@tiagodread
Copy link
Contributor

@marshmalien This looks pretty nice, we've found some feedback while verifying this:

Note: that the Idle Time Force Log Out was changed to 60 sec to test

  • Trying to visit a specific link without being logged, after the login step the user is seeing the dashboard except if you do a hard refresh before pasting the link and performing the login. (Note that on the video below before the second attempt I did a hard refresh before log in)
session1-2021-04-26_18.12.06.mp4
  • This is similar to the first item, but in this case, the user is not being redirected to a specific link trying to log in with social auth provider such as Google Oauth2 (Note that in the second login attempt I did a hard refresh):
session4-2021-04-26_18.31.17.mp4
  • If the session times out on the tab I’m logged into and i’m on a certain resource (ie templates list), when I log in back I can see the last page visited in the main tab, however, in the second tab (template list) the session modal is stuck in 1 sec on template list page.
session2-2021-04-26_18.15.12.mp4
  • Trying to login with an auth provider such as Google Oauth2 Settings, the session modal is being displayed right after the login step and the counter is stuck in 10 seconds:
session3-2021-04-26_18.25.22.mp4

We had 293 failures in e2e tests, I'm looking at the moment but seems to be the same issue.

@tiagodread
Copy link
Contributor

@marshmalien any update about this PR?

@tiagodread
Copy link
Contributor

@marshmalien I was looking into the e2e tests to understand what could be breaking the tests and I didn't found any reason why the Session modal is being displayed right after the login step, when all the cookies are being set

image

test-2021-05-19_10.12.56.mp4

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Contributor

@nixocio nixocio left a comment

Choose a reason for hiding this comment

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

We got in a call and we went over the different scenarios for third party auth providers. Good job @marshmalien.

@@ -26,6 +28,7 @@ import Metrics from './screens/Metrics';

import getRouteConfig from './routeConfig';
import SubscriptionEdit from './screens/Setting/Subscription/SubscriptionEdit';
import { SESSION_REDIRECT_URL } from './constants';
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Contributor

@tiagodread tiagodread left a comment

Choose a reason for hiding this comment

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

  • Verified that all issues found were fixed
  • Verified that all sub issues and added evidencies on each of them
  • Fixed all broken e2e tests

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:ui type:bug type:feature prioritized on a feature board
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants