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

upcoming: [M3-7716] - Add session expiry confirmation dialog for proxy users #10152

Merged
merged 16 commits into from
Feb 13, 2024

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Feb 6, 2024

Description πŸ“

When a proxy user wants to switch back to their parent account, the parent account's authentication token must still be valid in order for them to switch back to the parent's account or log into any other proxy accounts displayed in the drawer.

If the parent token is not valid, a proxy user should be given adequate notice that they must re-log in to the parent account. We also will disabled the "Switch Account" buttons if this is the case.

Changes πŸ”„

  • Updated the useDialogContext.ts hook API. The original API was kind of rigid, whereas now we can have dynamic state that we can extend if we need to. The original API and references to open and closed still work, but I will clean that up after this PR.
useDialogContext({
    continueSession: false,
    isOpen: false,
});
  • Added SwitchAccountSessionDialog.tsx for when parent session expires
  • Added useParentTokenManagement.tsx for checking expiration and showing dialog. We still may need to poll the expiry or at least check every time the user menu opens or a user lands on the account page.
  • Improved isRestrictedGlobalGrantType to take into consideration account_access which can be null | 'read_only' | 'read_write'

Preview πŸ“·

Screenshots
localhost_3000_account_billing (2)
localhost_3000_account_billing
localhost_3000_account_billing (1)

How to test πŸ§ͺ

Should NOT show session expired dialog

Prerequisites

  • Turn on MSW and Parent/Child feature flag
  • By default you should be a parent user, if not, update: serverHandlers

Reproduction steps

  • Open user menu
  • Click Switch Account button
  • Select a child company

Verification steps

  • Observe parent/proxy tokens are created in local storage
  • Set user_type to proxy in serverHandlers
  • Observe no dialog appears since we have an active parent token

Should show session expired dialog

Prerequisites

  • Clear application cookies
  • Turn on MSW and Parent/Child feature flag
  • Set user_type to proxy in serverHandlers

Reproduction steps

  • Since there is no parent token defined, it's invalid and you should see the dialog

Verification steps

  • Close the dialog and observe that it should not appear again
  • Observe that both "Switch Account" buttons in user menu and account page are disabled

As an Author I have considered πŸ€”

Check all that apply

  • πŸ‘€ Doing a self review
  • ❔ Our contribution guidelines
  • 🀏 Splitting feature into small PRs
  • βž• Adding a changeset
  • πŸ§ͺ Providing/Improving test coverage
  • πŸ” Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • πŸ‘£ Providing comprehensive reproduction steps
  • πŸ“‘ Providing or updating our documentation
  • πŸ•› Scheduling a pair reviewing session
  • πŸ“± Providing mobile support
  • β™Ώ Providing accessibility support

@jaalah-akamai jaalah-akamai requested a review from a team as a code owner February 6, 2024 19:01
@jaalah-akamai jaalah-akamai requested review from jdamore-linode and hana-linode and removed request for a team February 6, 2024 19:01
@jaalah-akamai jaalah-akamai self-assigned this Feb 6, 2024
isSideMenuOpen={!desktopMenuIsOpen}
openSideMenu={() => toggleMenu(true)}
username={username}
<SwitchAccountSessionProvider value={switchAccountSessionContextValue}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only change here was adding the Provider wrapper (rest is context shift)

() => setState((prevState) => ({ ...prevState, isOpen: true })),
[]
);
const close = useCallback(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added TODO since open and close are no longer necessary, but I didn't want to break existing API.

[key: string]: boolean;
};

export const defaultContext: DialogContextProps = {
close: () => void 0,
isOpen: false,
open: () => void 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll be able to remove these 3 as well in upcoming PR

}
}, [isParentTokenExpired, sessionContext]);

return { isParentTokenExpired };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to input/thoughts here

Copy link

github-actions bot commented Feb 6, 2024

Coverage Report: βœ…
Base Coverage: 81.13%
Current Coverage: 81.14%

Copy link
Contributor

@hana-linode hana-linode left a comment

Choose a reason for hiding this comment

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

If a user is able to continue working on an account with an expired parent token, I think it would make more sense to only display this dialog when they try to switch accounts?

image

@jaalah-akamai
Copy link
Contributor Author

If a user is able to continue working on an account with an expired parent token, I think it would make more sense to only display this dialog when they try to switch accounts?

The goal was to prevent users from attempting to switch accounts once the token expired - so the Switch Account button would be disabled at expiry. I'll spend some more time thinking about the UX, but that was the intention.

@hana-linode
Copy link
Contributor

@jaalah What if we don't disable the Switch Account button at expiry but just have it open the session expiry confirmation dialog instead? That way the dialog doesn't display when the user isn't trying to switch accounts but still prevents users from switching accounts

@jaalah-akamai
Copy link
Contributor Author

@hana-linode πŸ™Œ

reviewupdates.mp4

@jaalah-akamai
Copy link
Contributor Author

jaalah-akamai commented Feb 8, 2024

I'm keeping things in context because we're going to implement a way to refresh parent tokens as expiry nears an end, so I'll swap the dialog and save me some work.

@hana-linode
Copy link
Contributor

hana-linode commented Feb 9, 2024

Liking this flow a lot better. I was thinking we can keep the Switch Account buttons enabled even after the user hits close. And just keep having the button open the session expiry confirmation dialog until they re-login to the parent account

Screen.Recording.2024-02-09.at.11.02.09.AM.mov

@mjac0bs
Copy link
Contributor

mjac0bs commented Feb 9, 2024

Liking this flow a lot better. I was thinking we can keep the Switch Account buttons enabled even after the user hits close

Hmm, I agree with this too. We're currently doing a bit of both flows when we only disable after the first click of the button/appearance of the dialog, which feels like an odd UX. It'd be more consistent to either (1) always allow the user to click the enabled button and see the modal or (2) always disable the buttons, which was the original behavior but I think Hana's suggestion is better -- less intrusive for the user.

A minor note: if the parent token is expired and if the parent profile request is not cached, then we might end up with the parent username no longer displaying in the user menu above the company name.

@jaalah-akamai
Copy link
Contributor Author

@hana-linode @mjac0bs βœ… thanks for feedback!

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the changes - the expiry dialog looks good and is displayed for a proxy user clicking either Switch Account button who doesn't have a valid parent token in local storage.

Left a few comments about minor clean up - mostly related to naming, a couple related to the tooltip we were displaying when disabling the button.

jaalah-akamai and others added 3 commits February 12, 2024 16:13
…537679.md

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Feb 12, 2024
@hana-linode hana-linode added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Feb 13, 2024
@jaalah-akamai jaalah-akamai merged commit 22eb025 into linode:develop Feb 13, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Parent / Child Account
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants