From fac49a9be0e59dce39768cc0c5a183959f467eaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20K=C3=BCnzel?= <simonk@fsmpi.rwth-aachen.de> Date: Sat, 10 May 2025 00:21:26 +0200 Subject: [PATCH] Adjust OAuth to fix some problems where OAuth didn't finish (probably some race condition, RWTH servers caching problem) --- lang/de.slf | 3 +- lang/en.slf | 3 +- .../authentication/ViewPermissions.tsx | 117 ++++++++++-------- src/videoag/miscellaneous/PromiseHelpers.tsx | 1 + 4 files changed, 70 insertions(+), 54 deletions(-) diff --git a/lang/de.slf b/lang/de.slf index a612263..41b41b5 100644 --- a/lang/de.slf +++ b/lang/de.slf @@ -214,7 +214,8 @@ ui.video_player.login_moodle.description = "Für Teilnehmer der Veranstaltung ve ui.video_player.login_moodle.not_in_course = "Du bist kein Teilnehmer des Moodle-Kurses!" ui.video_player.login_moodle.refresh_course = "Kurse aktualisieren" ui.video_player.login_oauth.finished_no_access = "OAuth erfolgreich allerdings hast Du keine Berechtigung für diese Vorlesung" -ui.video_player.login_oauth.unfinished_popup_closed = "Popup wurde geschlossen bevor der OAuth fertig war" +ui.video_player.login_oauth.waiting_popup_closed = "Popup wurde geschlossen. Warten auf Bestätigung des OAuth Anbieter..." +ui.video_player.login_oauth.unfinished_popup_closed = "Popup wurde geschlossen aber der OAuth wurde nicht erfolgreich abgeschlossen" ui.video_player.login_oauth.waiting_for_finish = """ Bitte schließe den OAuth in dem neuen Fenster ab, welches sich geöffnet hat, und schließe es dann. """ diff --git a/lang/en.slf b/lang/en.slf index 90499cd..ba00f9c 100644 --- a/lang/en.slf +++ b/lang/en.slf @@ -214,7 +214,8 @@ ui.video_player.login_moodle.description = "Available to participants of the Moo ui.video_player.login_moodle.not_in_course = "You are not enrolled in the Moodle course" ui.video_player.login_moodle.refresh_course = "Refresh course" ui.video_player.login_oauth.finished_no_access = "OAuth finished but you don't have permissions to access this lecture" -ui.video_player.login_oauth.unfinished_popup_closed = "Popup closed without finishing OAuth" +ui.video_player.login_oauth.waiting_popup_closed = "Popup was closed. Waiting for OAuth Provider to confirm login..." +ui.video_player.login_oauth.unfinished_popup_closed = "Popup was closed but OAuth was completed successfully" ui.video_player.login_oauth.waiting_for_finish = "Please finish the OAuth in the new window which has opened, then close it." ui.video_player.login_oauth.error_start = "Error while starting OAuth" ui.video_player.login_oauth.error_finish = "Error while finishing OAuth" diff --git a/src/videoag/authentication/ViewPermissions.tsx b/src/videoag/authentication/ViewPermissions.tsx index dca98f6..bd1cbac 100644 --- a/src/videoag/authentication/ViewPermissions.tsx +++ b/src/videoag/authentication/ViewPermissions.tsx @@ -163,6 +163,11 @@ function PasswordAuthComponent({ course, lecture }: { course: course; lecture: l ); } +const _OAUTH_FINISH_PERIODIC_CHECK_INTERVAL_SECONDS = 3; +const _OAUTH_FINISH_PERIODIC_CHECK_MAX_ATTEMPTS = Math.ceil( + 30 / _OAUTH_FINISH_PERIODIC_CHECK_INTERVAL_SECONDS, +); + function OAuthComponent({ type, course, @@ -178,7 +183,6 @@ function OAuthComponent({ const { language } = useLanguage(); const [isOAuthRunning, setIsOAuthRunning] = useState(false); - const [waitingForFinish, setWaitingForFinish] = useState(false); const remainingFinishAttempts = useRef<number>(0); const popupClosed = useRef<boolean>(false); @@ -188,70 +192,82 @@ function OAuthComponent({ useEffect(() => { return () => { remainingFinishAttempts.current = 0; + popupClosed.current = true; }; }, []); - const [fetchStatusDeferred, fetchStatusNow] = useDebouncedCall<void>(1000, () => - api - .getAuthenticationStatus({ lecture_id: lecture.id }) - .then((res) => { - if (res.is_lecture_authenticated) { - setIsOAuthRunning(false); - setWaitingForFinish(false); - authStatus.addAuthenticatedMethod(type); - authStatus.setCurrentlyAuthenticatedLectureId(lecture.id); - } else if (res.in_progress_authentication !== type) { - setIsOAuthRunning(false); - setWaitingForFinish(false); - setOauthInfo( - <span className="text-warning"> - {language.get("ui.video_player.login_oauth.finished_no_access")} - </span>, - ); - } else { - if (remainingFinishAttempts.current > 0) { - remainingFinishAttempts.current--; - fetchStatusDeferred(); - } else if (popupClosed.current) { + // We had a periodic check in here, however, this caused some problems as the RWTH OAuth servers didn't always + // return the new logged in status immediately (there seems to be some caching of at least 20 seconds) + // Instead we now only rely on someone closing the popup, and then start a periodic timer for a few attempts. + const [fetchStatusDeferred, fetchStatusNow] = useDebouncedCall<void>( + _OAUTH_FINISH_PERIODIC_CHECK_INTERVAL_SECONDS * 1000, + () => + api + .getAuthenticationStatus({ lecture_id: lecture.id }) + .then((res) => { + if (res.is_lecture_authenticated) { + setIsOAuthRunning(false); + authStatus.addAuthenticatedMethod(type); + authStatus.setCurrentlyAuthenticatedLectureId(lecture.id); + } else if (res.in_progress_authentication !== type) { setIsOAuthRunning(false); - setWaitingForFinish(false); setOauthInfo( <span className="text-warning"> - {language.get( - "ui.video_player.login_oauth.unfinished_popup_closed", - )} + {language.get("ui.video_player.login_oauth.finished_no_access")} </span>, ); - } else if (remainingFinishAttempts.current <= 0) { - console.log("Stopping automatic authentication status fetching"); + } else { + if (remainingFinishAttempts.current > 0) { + remainingFinishAttempts.current--; + if ( + remainingFinishAttempts.current <= + _OAUTH_FINISH_PERIODIC_CHECK_MAX_ATTEMPTS - 2 + ) { + setOauthInfo( + language.get( + "ui.video_player.login_oauth.waiting_popup_closed", + ), + ); + } + fetchStatusDeferred(); + } else { + setIsOAuthRunning(false); + setOauthInfo( + <span className="text-warning"> + {language.get( + "ui.video_player.login_oauth.unfinished_popup_closed", + )} + </span>, + ); + } } - } - }) - .catch((e) => { - showError(e, language.get("ui.video_player.login_oauth.error_finish")); - remainingFinishAttempts.current = 0; - if (popupClosed.current) { - setIsOAuthRunning(false); - setWaitingForFinish(false); - setOauthInfo(undefined); - } - }), + }) + .catch((e) => { + showError(e, language.get("ui.video_player.login_oauth.error_finish")); + remainingFinishAttempts.current = 0; + if (popupClosed.current) { + setIsOAuthRunning(false); + setOauthInfo(undefined); + } + }), ); const startFinishListener = (popup: any) => { - setWaitingForFinish(true); - setOauthInfo(undefined); + setOauthInfo(language.get("ui.video_player.login_oauth.waiting_for_finish")); popupClosed.current = false; - remainingFinishAttempts.current = 120; - fetchStatusDeferred(); let popInterval = setInterval(() => { - if (popup.closed) { - remainingFinishAttempts.current = 1; - popupClosed.current = true; - fetchStatusNow(); - clearInterval(popInterval); + if (!popup.closed) { + return; } + clearInterval(popInterval); + if (popupClosed.current) { + // Also set on unmounting. Don't start a new timer then. + return; + } + remainingFinishAttempts.current = _OAUTH_FINISH_PERIODIC_CHECK_MAX_ATTEMPTS; + popupClosed.current = true; + fetchStatusNow(); }, 100); }; @@ -305,9 +321,6 @@ function OAuthComponent({ {isOAuthRunning && ( <div className="flex-shrink-0 spinner-border text-primary" role="status" /> )} - {waitingForFinish && ( - <span>{language.get("ui.video_player.login_oauth.waiting_for_finish")}</span> - )} {oauthInfo && <span>{oauthInfo}</span>} </div> </> diff --git a/src/videoag/miscellaneous/PromiseHelpers.tsx b/src/videoag/miscellaneous/PromiseHelpers.tsx index 62ef762..afa5b40 100644 --- a/src/videoag/miscellaneous/PromiseHelpers.tsx +++ b/src/videoag/miscellaneous/PromiseHelpers.tsx @@ -84,6 +84,7 @@ export function useDebouncedProcessing<D, R>( function doProcessing(): Promise<R> | undefined { if (processingPromiseRef.current !== undefined) { + // TODO this is wrong as it might return a promise for a processor which started before now() was called return processingPromiseRef.current; } if (unprocessedDataRef.current instanceof NoDataMarker) { -- GitLab