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.

Sign up for GitHub

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

Jump to bottom

fix(u2f): Have authenticators GET wait for preloading to finish #71142

Merged
merged 25 commits into from
May 22, 2024
Merged

fix(u2f): Have authenticators GET wait for preloading to finish #71142

merged 25 commits into from
May 22, 2024

Conversation

schew2381
Copy link
Member

@schew2381 schew2381 commented May 17, 2024 •
edited

In order to have the U2F state be correctly set in the client browser session cookie, we need to wait for preloading requests to finish first to avoid a race condition. This was done by:

  1. Converting the sudo modal to a functional component so we can use useContext in it chore(staff): Convert sudo modal to functional #71100
  2. Keeping track of when the preloading finishes with a hook in the organizationLoadingContext chore(context): Track progress of preloading requests #71112
  3. Finally this PR, which finally has the U2F utilize the hook to wait for preloading to finish before making an authenticators GET request

Note that the PRs mentioned above must be merged first

Local Example

take_two.mov

@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels May 17, 2024
@schew2381 schew2381 changed the title WIP: Wait for preloading fix(u2f): Have authenticators GET wait for preloading to finish May 17, 2024
@schew2381 schew2381 self-assigned this May 17, 2024
@schew2381 schew2381 force-pushed the seiji/chore/wait-for-preloading-orgs-to-finish branch from e3ad931 to ef969ee Compare May 20, 2024 16:50
@codecov Codecov
Copy link

codecov bot commented May 20, 2024 •
edited

Bundle Report

Changes will increase total bundle size by 20.34kB ⬆️

Bundle name Size Change
app-webpack-bundle-array-push 27.69MB 20.34kB ⬆️

@schew2381 schew2381 requested review from saponifi3d, a team and cathteng May 20, 2024 17:00
@schew2381 schew2381 marked this pull request as ready for review May 20, 2024 17:02
saponifi3d
saponifi3d approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

just nits, generally lgtm.

static/app/components/modals/sudoModal.tsx Outdated Show resolved Hide resolved
static/app/components/modals/sudoModal.tsx Outdated Show resolved Hide resolved
static/app/views/organizationContext.tsx Outdated Show resolved Hide resolved
static/app/actionCreators/organization.tsx Outdated Show resolved Hide resolved
static/app/components/modals/sudoModal.tsx Outdated Show resolved Hide resolved
static/app/views/organizationContext.tsx Outdated Show resolved Hide resolved
static/app/views/organizationContext.tsx Outdated
@@ -16,6 +16,15 @@ import useApi from 'sentry/utils/useApi';
import {useParams} from 'sentry/utils/useParams';
import {useRoutes} from 'sentry/utils/useRoutes';

interface OrganizationLoaderContextProps {
isLoading: boolean;
loadOrganization: () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be cool to have this return a Promise rather than void, then you wouldn't need to maintain a separate isLoading variable, instead you could just continue the promise chain and use the promises state to for the load state.

Copy link
Member Author

Choose a reason for hiding this comment

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

is this how to do it? I suspect I might have it wrong using the promise directly in the useEffect dependency array

Copy link
Contributor

Choose a reason for hiding this comment

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

🔥 i'd say this is like 99% there, i left a comment on the useEffect but i think you can basically remove it and be good to go.

static/app/views/organizationContext.tsx Outdated Show resolved Hide resolved
@schew2381 schew2381 removed the Scope: Backend Automatically applied to PRs that change backend components label May 21, 2024
@getsentry getsentry deleted a comment from github-actions bot May 21, 2024
@schew2381 schew2381 requested a review from saponifi3d May 21, 2024 21:03
saponifi3d
saponifi3d reviewed May 21, 2024
View reviewed changes
static/app/components/modals/sudoModal.tsx Outdated
...prevState,
authenticators: fetchedAuthenticators ?? [],
}));
loadOrganization().then(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we'll need this inside of a useEffect, i believe you can just grab the loadOrganization.then(() here instead.

This is because the OrganizationContextLoader will initialize the promise chain, then here we're referencing the promise directly, so whenever the promise resolves it will call .then.

static/app/views/organizationContext.tsx Outdated
@@ -16,6 +16,15 @@ import useApi from 'sentry/utils/useApi';
import {useParams} from 'sentry/utils/useParams';
import {useRoutes} from 'sentry/utils/useRoutes';

interface OrganizationLoaderContextProps {
isLoading: boolean;
loadOrganization: () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥 i'd say this is like 99% there, i left a comment on the useEffect but i think you can basically remove it and be good to go.

@cathteng cathteng merged commit d20df6a into master May 22, 2024
41 checks passed
@cathteng cathteng deleted the seiji/chore/wait-for-preloading-orgs-to-finish branch May 22, 2024 17:26
@cathteng cathteng added the Trigger: Revert add to a merged PR to revert it (skips CI) label May 22, 2024
@getsentry-bot
Copy link
Contributor

PR reverted: 1fc431c

getsentry-bot added a commit that referenced this pull request May 22, 2024
@getsentry-bot @cathteng
Revert "fix(u2f): Have authenticators GET wait for preloading to fini…
1fc431c 
…sh (#71142)"

This reverts commit d20df6a.

Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>
@cathteng cathteng mentioned this pull request May 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saponifi3d saponifi3d saponifi3d approved these changes

@cathteng cathteng Awaiting requested review from cathteng

Assignees

@schew2381 schew2381

Labels
Scope: Frontend Automatically applied to PRs that change frontend components Trigger: Revert add to a merged PR to revert it (skips CI)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@schew2381 @getsentry-bot @saponifi3d @cathteng @michellewzhang