Repository navigation
fix: restore the Studio session without leaving the page when Studio requests fail - #3290
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @blarghmatey! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3290 +/- ##
==========================================
+ Coverage 96.20% 96.22% +0.02%
==========================================
Files 1446 1449 +3
Lines 35391 35613 +222
Branches 8468 8528 +60
==========================================
+ Hits 34048 34270 +222
- Misses 1285 1301 +16
+ Partials 58 42 -16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@blarghmatey Can you please link to the corresponding platform PR? Ideally, we'd convert all these endpoints to JWT authentication. Such work is ongoing but there's a lot of legacy endpoints and I think it's still worth merging this PR in the meantime. There are also a few things like course export downloads, taxonomy export downloads, and library export downloads which send requests to the backend as regular browser navigation, without auth headers. One blocking concern though: a 401 on a save, such as Also, content within iframes, like the unit page content, won't be intercepted. I guess this is fine. |
) * fix: restore the Studio session without leaving the page (LP-1313) Several Studio views that this MFE calls (course updates, handouts, the xblock and outline handlers, assets, certificates, ...) need the Studio session and ignore the JWT cookie. A user who is signed in to the LMS but not to Studio gets those requests redirected to Studio's login. The browser blocks that cross-origin redirect at the LMS, the request fails with no response, and the page shows "Failed to load course updates" or "Unable to save changes". Add a response interceptor on the authenticated HTTP client, based on openedx#3290: - A Studio request that fails with a 401 or with no response is held back when a check shows there is no Studio session. - A no-cors request to Studio's /login/ follows the OAuth redirects through the LMS and signs the user in to Studio without leaving the page. The held requests are then sent again, so the page gets its data and saves go through. The cached CSRF token is cleared, as Studio replaces it at login. - If the user is signed out of the LMS too, a page that was loading data goes to Studio's login and comes back. A page with a save in progress asks first ("Sign in to Studio again"), so the change is not lost without warning. - Studio requests made meanwhile are held, so they cannot break the login in progress, and React Query does not retry these failures. * fix: harden the Studio session recovery after review (LP-1313) - Count Studio saves from when they are sent until they are answered. A page with one asks before going to the login, even when only a read has failed so far. - Treat a 401 from the session check as no session. - Give the session check and the background sign-in a 10 s deadline. When Studio does not answer, the held requests go on as before, with no sign-in, redirect or dialog. - Cancel in the dialog fails the requests that waited to be sent, without sending them. The hold timeout after a redirect still sends them. - Stamp each Studio request with the number of background sign-ins done when it was sent. A failure from before the last sign-in is sent again once if it is a read, and only on a 401 if it is a save. * fix: drop the sign-in dialog and use the HTTP client for the Studio session (LP-1313) - When the user is signed out of the LMS too, the page goes to the Studio login and comes back, for reads and saves alike. The "Sign in to Studio again" dialog, its messages and its tests are removed, and so is the count of saves in progress, which only chose between the dialog and the redirect. If the last login did not restore the session, the held requests fail as before instead of going to the login again. - The session check and the background sign-in go through frontend-platform's getHttpClient() with axios's fetch adapter and fetchOptions (redirect: 'manual', mode: 'no-cors') instead of calling fetch directly. axios's timeout replaces the AbortController helper. - Tests give the plain client's fetch adapter their fetch mock through defaults.env.fetch, as axios finds no fetch in jsdom when it loads. New tests cover the client options, a failed save going to the login, the loop guard, isStudioUnauthorizedError and redirectToStudioLogin. * fix: send a Studio request again only if it was refused (LP-1313) A request that got no response now counts as refused for lack of a Studio session only on Studio's older views, which redirect to the login before doing anything. Studio's REST APIs (under /api/) answer 401 instead, so one of their requests that got no response may have been carried out. It now goes to the page as before, and is not held or sent again after the background sign-in. * fix: wait for Studio requests already sent before the background sign-in (LP-1313) A Studio request sent before the background sign-in can still follow Studio's login redirect while the sign-in runs. Each one that reaches the login starts a login of its own and replaces the OAuth state that the sign-in needs, so the sign-in could fail and send a user who is still signed in to the LMS to the login page. The interceptor now counts requests to Studio's older views from when they are sent until they are answered, and the sign-in waits for them, for at most WAIT_FOR_SENT_REQUESTS_MS (5 s). A request stops counting when its answer arrives, before it is held, so held requests do not keep the sign-in waiting. Tests cover a redirect that overlaps the sign-in, the 5 s fallback, and REST API requests that get no response before and during the sign-in.
|
Hi @bradenmacdonald, @blarghmatey At 2U we hit the same problem (#1080) on our fork and fixed it in the MFE, building on this PR: edx#126. It is merged on our It keeps this PR's module (
How would you like to proceed?
Whichever you prefer, we are happy to port it to |
|
I think we want "all of the above" (fix the platform to return 401 for non-HTML responses, fix Authoring MFE to handle the error whether or not studio returns 401, auto-sign in to Studio using the LMS creds if possible, direct user to sign in to Studio if not). But I do think we should add a modal popup to prompt the user rather than just redirecting them and potentially losing their work. As to the order of the PRs or which incorporates which, or merging it as several different PRs, it's up to you two. |
Many endpoints this app calls are authenticated by the Studio session and not by the JWT cookies, so the Studio session can expire while the user is still signed in to the LMS. Nothing in this app handles a 401 from Studio. This adds a response interceptor that sends the browser to Studio's /login/ with `next` set to the current page when Studio answers 401. One page fails many requests together, so they share a single navigation, and a sessionStorage timestamp stops a login that did not restore the session from being retried in a loop. React Query no longer retries those 401s. Claude-Session: https://claude.ai/code/session_01Rr6xsQQbrmFiSgQWMHJSA2
… login Today Studio answers a request without a Studio session by redirecting it to its login. Cross-origin, the browser blocks that redirect at the LMS, so the request fails with no response and the 401 handling never runs. Going to the login also loses a save in progress, even when the user is still signed in to the LMS. This ports the recovery from edx#126, which runs in 2U's production. A Studio request that fails with a 401, or with no response from one of Studio's older views, triggers a session check (GET /xblock/ through the fetch adapter with redirect: 'manual'). With no session, Studio requests are held, the requests already sent are given up to 5s to finish, and a no-cors GET to Studio's /login/ signs in through the LMS without leaving the page. The held requests are then sent again. Only when the LMS session is gone too does the page go to the login, as before. The code is 2U's, with oxlint and dprint fixes for master. Co-authored-by: Aditya Bhalsod <abhalsod-sonata@2u.com> Claude-Session: https://claude.ai/code/session_01JxJWMHVXsuXaChbeEpKT8t
…g the page When the background sign-in finds that the user is signed out of the LMS too, the page went to the Studio login, and a save in progress was lost on the way. This opens a dialog instead, and Studio requests stay held while it is open. Sign in opens the Studio login in a new tab. When this page is shown or focused again, it checks for a Studio session (running the background sign-in if the user only got as far as the LMS) and sends the held requests, so a pending save goes through. Continue does the same by hand. Cancel fails the held requests without sending them, so the page keeps its unsaved changes and shows its usual errors. Only the Cancel button closes the dialog (an Escape or a click outside would drop a pending save), and it is disabled during the check, so a check cannot outlive the dialog. Cancel also clears the CSRF token cache: the user may have signed in from the other tab anyway, which rotates Studio's CSRF token, and every later save would get a 403. With no automatic navigation left, the redirect loop guard and the hold timer go too. Claude-Session: https://claude.ai/code/session_01JxJWMHVXsuXaChbeEpKT8t
af45439 to
9881999
Compare
blarghmatey
changed the title
|
@bradenmacdonald I took 2U's changes into this PR (edx#126, credited in e3f71b7) and replaced the redirect with a dialog (9881999). The page first tries to sign in to Studio in the background. If the user is signed out of the LMS too, the dialog's Sign in opens the login in a new tab while Studio requests stay held, and when the user comes back the held save is sent, so the edit is not lost. Cancel keeps the page with its unsaved changes. The platform change is openedx/openedx-platform#39296. Content inside iframes is still not covered. |
|
@abhalsod-sonata I went with option 1: your recovery from edx#126 is in this PR as e3f71b7, with you as co-author and only oxlint/dprint fixes for master. The one change on top (9881999) is the signed-out-of-the-LMS case, which now opens a dialog that signs in in a new tab and then sends the held requests, instead of the redirect. Your review would help, especially from what you have seen in production. |
|
Thanks @blarghmatey for taking this in, and @bradenmacdonald for the direction. Option 1 works for us. I checked e3f71b7 against edx#126 and it is the same code, only reformatted. The dialog in 9881999 looks good to me and solves the lost-save problem. Our version (#126) has been running in production since Oct 16. Two small suggestions, not blocking:
Thanks again! |
The dialog's Sign in link came back to the page that opened it, so the user ended up with two copies of the page. An edit made in the new tab could then be overwritten by the save still held in the first one. Without a `next`, Studio's login goes to its LOGIN_REDIRECT_URL (Studio home) instead. Claude-Session: https://claude.ai/code/session_01JxJWMHVXsuXaChbeEpKT8t
Pages that poll Studio (e.g. the export page every 3s, the course optimizer every 2s) opened the dialog again right after the user cancelled it. For 30s after Cancel, a read that fails for lack of a Studio session now fails without a background sign-in or the dialog. A save still opens it, as that is the user asking for it. Claude-Session: https://claude.ai/code/session_01JxJWMHVXsuXaChbeEpKT8t
|
@abhalsod-sonata both are in. 49021a2 sends the Sign in tab to Studio home: without a |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
It's a lot of new code for something as basic as authentication, but the current situation is quite messy and the resulting user experience is a million times better with this PR, so I think it's well worth merging. I tried it out as suggested and it worked really nicely in each situation.
I do hope that the API Revamp Project and perhaps a better API client in frontend-base or as part of that revamp will reduce the need for this sort of thing.
Description
Many endpoints this app calls are authenticated by the Studio session and not by the JWT cookies (e.g.
/xblock/outline/<usage key>,/course_info_update/,/assets/). The Studio session can expire, or not exist yet when the user signed in through the LMS and came straight to this app, while the user is still signed in to the LMS. Today Studio redirects those requests to its login. On the same origin, each one starts an OAuth login of its own and they break each other; across origins, the browser blocks the redirect at the LMS and the request fails with no response. Either way the page shows a load or save error.This adds a response interceptor on the authenticated HTTP client that gets the Studio session back:
GET /xblock/through axios'sfetchadapter withredirect: 'manual').no-corsrequest to Studio's/login/signs in through the LMS without leaving the page, and the held requests are sent again.The background sign-in is ported from edx#126 (2U, running in their production on
release-ulmo), credited in the commit. The dialog replaces the full-page redirect that both versions had.This impacts Course Authors. It works with Studio as it is today. openedx/openedx-platform#39296 makes Studio answer 401 instead of redirecting when the client is not asking for HTML, which this handles too.
Supporting information
/xblock/outline/requests within a minute. Studio redirected each one into its own OAuth login against the same session. social-core keeps the OAuthstatein a single session key, so the logins overwrote each other: 7 completed and the rest failed invalidate_statewithAuthStateForbiddenorAuthStateMissing, which Studio served as 262 HTTP 500 responses in that minute.Testing instructions
Use a devstack with production-like CORS so the cross-origin failure shows. In
lms/envs/private.pyandcms/envs/private.pysetCORS_ORIGIN_ALLOW_ALL = FalseandCORS_ORIGIN_REGEX_WHITELIST = [r'^http://localhost:\d+$'].no-corsrequest to<studio>/login/?next=/xblock/, and the page stays put.studio_sessionidcookie and post a course update. It saves.npx jest src/generic/studio-login-redirect: 51 tests, covering every line ofindex.tsandStudioLoginModal.tsx.npm run lintandnpm run typespass. I have not run steps 1-4 against a running Studio; 2U ran steps 1-2 with their version (results in edx#126).Other information
getCsrfTokenService(), which frontend-platform documents as mainly for testing. Without it, every save after a sign-in is refused with 403.GET /xblock/, which answers 400 with a session and redirects (or, with the platform change, answers 401) without one.Best Practices Checklist
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'https://claude.ai/code/session_01JxJWMHVXsuXaChbeEpKT8t