From ae181659bfcff81cd8fc5d51eb04f21eef633da9 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 8 Jul 2026 08:47:59 -0600 Subject: [PATCH 1/5] Add app-wide login and profile-photo avatars (BL-16514) Move Bloom Library login state out of the Publish screen into an app-wide account API (account/status|login|logout) that broadcasts changes over a websocket, and add a top-bar account menu that shows the signed-in user's avatar and a sign-out item. Avatars are served by a new person-keyed cache on Bloom's local server (GET /bloom/api/avatar?email=). Keyed by the MD5 of the email (Gravatar's scheme), it caches the image bytes on disk so avatars survive a restart and work offline. Source precedence per person: the user's Google/Firebase profile photo (now sent by the website in the login callback, when present), otherwise Gravatar, otherwise a 404 so the UI shows generated initials. It refetches once per Bloom run, falling back to the cached copy if the fetch fails. Co-Authored-By: Claude Opus 4.8 (1M context) --- DistFiles/localization/en/Bloom.xlf | 15 + .../LibraryPublish/LibraryPublishSteps.tsx | 20 +- .../workspaceTopRightControls/AccountMenu.tsx | 136 +++++++ .../WorkspaceTopRightControls.tsx | 51 ++- .../react_components/bloomAvatar.tsx | 33 +- .../react_components/useLoginState.ts | 21 + src/BloomExe/ProjectContext.cs | 10 + .../BloomLibrary/BloomLibraryPublishModel.cs | 10 - .../BloomLibraryBookApiClient.cs | 85 +++- .../WebLibraryIntegration/BookUpload.cs | 7 +- src/BloomExe/web/AvatarCache.cs | 382 ++++++++++++++++++ src/BloomExe/web/controllers/AccountApi.cs | 142 +++++++ src/BloomExe/web/controllers/AvatarApi.cs | 70 ++++ src/BloomExe/web/controllers/ExternalApi.cs | 28 +- .../web/controllers/LibraryPublishApi.cs | 49 --- .../BloomLibraryBookApiClientLoginTests.cs | 147 +++++++ src/BloomTests/web/AvatarApiTests.cs | 180 +++++++++ src/BloomTests/web/AvatarCacheTests.cs | 287 +++++++++++++ 18 files changed, 1560 insertions(+), 113 deletions(-) create mode 100644 src/BloomBrowserUI/react_components/TopBar/workspaceTopRightControls/AccountMenu.tsx create mode 100644 src/BloomBrowserUI/react_components/useLoginState.ts create mode 100644 src/BloomExe/web/AvatarCache.cs create mode 100644 src/BloomExe/web/controllers/AccountApi.cs create mode 100644 src/BloomExe/web/controllers/AvatarApi.cs create mode 100644 src/BloomTests/WebLibraryIntegration/BloomLibraryBookApiClientLoginTests.cs create mode 100644 src/BloomTests/web/AvatarApiTests.cs create mode 100644 src/BloomTests/web/AvatarCacheTests.cs diff --git a/DistFiles/localization/en/Bloom.xlf b/DistFiles/localization/en/Bloom.xlf index d61b563609f2..dd64c53c3e5c 100644 --- a/DistFiles/localization/en/Bloom.xlf +++ b/DistFiles/localization/en/Bloom.xlf @@ -107,6 +107,21 @@ Bloom Accessibility Checker ID: AccessibilityCheck.WindowTitle + + Account + ID: AccountMenu.Account + Tooltip and accessible label for the round avatar button in the top bar of the main window. + + + Sign in + ID: AccountMenu.SignIn + Button in the top bar of the main window. Clicking it opens the web browser to sign in to BloomLibrary.org. + + + Sign out + ID: AccountMenu.SignOut + Menu item shown under the user's avatar in the top bar; signs the user out of BloomLibrary.org. + Software Updates ID: AutoUpdateSoftwareDialog.SoftwareUpdates diff --git a/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx b/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx index deaa0a9e7acf..55e6560139d5 100644 --- a/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx +++ b/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx @@ -23,6 +23,7 @@ import { } from "../../react_components/Progress/progressBox"; import { BloomCheckbox } from "../../react_components/BloomCheckBox"; import { useL10n } from "../../react_components/l10nHooks"; +import { useLoginState } from "../../react_components/useLoginState"; import { kWebSocketContext } from "./LibraryPublishScreen"; import { useSubscribeToWebSocketForEvent, @@ -60,7 +61,6 @@ interface IReadonlyBookInfo { const kWebSocketEventId_uploadSuccessful: string = "uploadSuccessful"; const kWebSocketEventId_uploadCanceled: string = "uploadCanceled"; -const kWebSocketEventId_loginSuccessful: string = "loginSuccessful"; export const LibraryPublishSteps: React.FunctionComponent = () => { const selectedBookContext = React.useContext(SelectedBookContext); @@ -128,7 +128,6 @@ export const LibraryPublishSteps: React.FunctionComponent = () => { const [isLoading, setIsLoading] = useState(true); const [bookInfo, setBookInfo] = useState(); useEffect(() => { - post("libraryPublish/checkForLoggedInUser"); getBoolean("libraryPublish/agreementsAccepted", (result) => { setAgreedPreviously(result); setAgreementsAccepted(result); @@ -191,15 +190,7 @@ export const LibraryPublishSteps: React.FunctionComponent = () => { else hasRenderedRef.current = true; }, [agreementsAccepted]); - const [loggedInEmail, setLoggedInEmail] = useState(); - - useSubscribeToWebSocketForStringMessage( - kWebSocketContext, - kWebSocketEventId_loginSuccessful, - (email) => { - setLoggedInEmail(email); - }, - ); + const { email: loggedInEmail, signIn, signOut } = useLoginState(); function isReadyForUpload(): boolean { return isReadyForAgreements() && agreementsAccepted; @@ -593,7 +584,7 @@ export const LibraryPublishSteps: React.FunctionComponent = () => { isReadyForUpload() && !isPlaygroundBook } l10nKey="PublishTab.Upload.SignIn" - onClick={() => post("libraryPublish/login")} + onClick={signIn} > Sign in or sign up to BloomLibrary.org @@ -622,10 +613,7 @@ export const LibraryPublishSteps: React.FunctionComponent = () => { l10nKey="PublishTab.Upload.SignOut" l10nComment="The %0 will be replaced with the email address of the user." l10nParam0={loggedInEmail} - onClick={() => { - post("libraryPublish/logout"); - setLoggedInEmail(undefined); - }} + onClick={signOut} > Sign out (%0) diff --git a/src/BloomBrowserUI/react_components/TopBar/workspaceTopRightControls/AccountMenu.tsx b/src/BloomBrowserUI/react_components/TopBar/workspaceTopRightControls/AccountMenu.tsx new file mode 100644 index 000000000000..05a9453f6b8a --- /dev/null +++ b/src/BloomBrowserUI/react_components/TopBar/workspaceTopRightControls/AccountMenu.tsx @@ -0,0 +1,136 @@ +import * as React from "react"; +import { css } from "@emotion/react"; +import { ArrowDropDown } from "@mui/icons-material"; +import Menu from "@mui/material/Menu"; +import Divider from "@mui/material/Divider"; +import { BloomTooltip } from "../../BloomToolTip"; +import { TopRightMenuButton, topRightMenuArrowCss } from "./TopRightMenuButton"; +import { useL10n } from "../../l10nHooks"; +import { LocalizableMenuItem } from "../../localizableMenuItem"; +import { callOnBlur } from "../../../utils/menuCloseOnBlur"; +import { BloomAvatar } from "../../bloomAvatar"; +import { useLoginState } from "../../useLoginState"; +import { useApiObject } from "../../../utils/bloomApi"; +import { RegistrationInfo } from "../../registration/registrationTypes"; + +// The account control shown at the top of the workspace's upper-right corner. +// When signed out, it is a simple "Sign in" button that launches the external +// browser login. When signed in, it shows the user's avatar; clicking it opens +// a menu with the signed-in email and a "Sign out" item. +export const AccountMenu: React.FunctionComponent = () => { + const signInText = useL10n("Sign in", "AccountMenu.SignIn"); + const accountText = useL10n("Account", "AccountMenu.Account"); + + const { email, signIn, signOut } = useLoginState(); + + // Used to get a display name for the avatar. Fetched once; falls back gracefully + // (BloomAvatar shows initials or a generated image) if it's empty. + const registrationInfo = useApiObject>( + "registration/userInfo", + {}, + ); + const displayName = [registrationInfo.firstName, registrationInfo.surname] + .filter((namePart) => !!namePart) + .join(" "); + + const [anchorEl, setAnchorEl] = React.useState(); + + const onClose = React.useCallback(() => { + setAnchorEl(undefined); + }, []); + + const onOpen = React.useCallback(() => { + // This button is rendered by this component, so it exists when its own onClick fires. + const button = + document.getElementById("accountMenuButton") ?? undefined; + setAnchorEl(button); + callOnBlur(onClose); + }, [onClose]); + + const handleSignOut = React.useCallback(() => { + onClose(); + signOut(); + }, [onClose, signOut]); + + if (!email) { + return ( + + ); + } + + return ( + + + + + +
+ {email} +
+ + + + + ); +}; diff --git a/src/BloomBrowserUI/react_components/TopBar/workspaceTopRightControls/WorkspaceTopRightControls.tsx b/src/BloomBrowserUI/react_components/TopBar/workspaceTopRightControls/WorkspaceTopRightControls.tsx index 0d5dc865ba47..300060afd270 100644 --- a/src/BloomBrowserUI/react_components/TopBar/workspaceTopRightControls/WorkspaceTopRightControls.tsx +++ b/src/BloomBrowserUI/react_components/TopBar/workspaceTopRightControls/WorkspaceTopRightControls.tsx @@ -5,10 +5,12 @@ import { createTheme, ThemeProvider } from "@mui/material/styles"; import { ZoomControl } from "./ZoomControl"; import { UiLanguageMenu } from "./UiLanguageMenu"; import { HelpMenu } from "./HelpMenu"; +import { AccountMenu } from "./AccountMenu"; -// Every affordance in this group -- text, menu-button labels, the help icon, and the -// dropdown arrows -- is drawn in black at 80% opacity. The tab-bar background these sit -// on isn't always the same color, and black-at-80% reads well across those backgrounds. +// Every affordance in this group -- text, menu-button labels, the help icon, and all +// the dropdown arrows -- is drawn in black at 80% opacity. (The avatar image is not +// affected by color.) The tab-bar background these sit on isn't always the same color, +// and black-at-80% reads well across those backgrounds. const kTopRightControlColor = "rgba(0, 0, 0, 0.8)"; export const WorkspaceTopRightControls: React.FunctionComponent = () => { @@ -39,11 +41,18 @@ export const WorkspaceTopRightControls: React.FunctionComponent = () => {
{ } `} > - {/* This grid keeps the two menu buttons the same width which keeps the down arrows aligned horizontally */} + {/* The language chooser, help menu, and zoom control sit together on the + left so that the account menu can stand by itself on the right. */}
- - + {/* This grid keeps the two menu buttons the same width which keeps the down arrows aligned horizontally */} +
+ + +
+
- +
); diff --git a/src/BloomBrowserUI/react_components/bloomAvatar.tsx b/src/BloomBrowserUI/react_components/bloomAvatar.tsx index 9e543c22e578..bdceee16fa7f 100644 --- a/src/BloomBrowserUI/react_components/bloomAvatar.tsx +++ b/src/BloomBrowserUI/react_components/bloomAvatar.tsx @@ -2,7 +2,17 @@ import { css } from "@emotion/react"; import * as React from "react"; import Avatar, { Cache, ConfigProvider } from "react-avatar"; -import { getMd5 } from "../bookEdit/toolbox/talkingBook/md5Util"; +import { getBloomApiPrefix } from "../utils/bloomApi"; + +// react-avatar does not cache actual avatars. It only caches which source urls failed +// (whether because the user was offline or the source 404'd), and then doesn't retry the failed +// urls so long as they are valid in cache. We do want it to retry (the local server may have gained +// connectivity), so keep its cache empty. The config never changes, so this is a module-level +// constant rather than being rebuilt on every render. +const emptyAvatarCache = new Cache({ + sourceTTL: 0, // retain for 0 milliseconds + sourceSize: 0, // retain a maximum of 0 items in cache +}); export const BloomAvatar: React.FunctionComponent<{ email: string; @@ -19,14 +29,15 @@ export const BloomAvatar: React.FunctionComponent<{ ? `${borderSizeInt}px solid ${props.borderColor}` : undefined; - // react-avatar does not cache actual avatars. It only caches which gravatar urls failed - // (whether because user was offline or doesn't have a gravatar), - // and then doesn't retry the failed urls so long as they are valid in cache. - // We do want it to retry retrieving gravatars, so keep its cache empty - const cache = new Cache({ - sourceTTL: 0, // retain for 0 milliseconds - sourceSize: 0, // retain a maximum of 0 items in cache - }); + // The avatar image comes from Bloom's own local server, keyed by email: it decides the best + // source (the person's known Google/Firebase photo if we have one, otherwise Gravatar), caches + // the actual bytes so avatars survive restart and work offline, and returns 404 when it has + // nothing -- in which case react-avatar falls back to the generated initials from `name`. The + // server hashes/normalizes the email itself, so we just pass the email. + const avatarSrc = props.email + ? `${getBloomApiPrefix()}avatar?email=${encodeURIComponent(props.email)}` + : undefined; + return ( }>
- + { + const status = useWatchApiObject<{ email: string }>( + "account/status", + { email: "" }, + "account", + "loginStateChanged", + ); + return { + email: status.email || undefined, + signIn: () => post("account/login"), + signOut: () => post("account/logout"), + }; +}; diff --git a/src/BloomExe/ProjectContext.cs b/src/BloomExe/ProjectContext.cs index da6141ce1095..577c3757a908 100644 --- a/src/BloomExe/ProjectContext.cs +++ b/src/BloomExe/ProjectContext.cs @@ -192,6 +192,9 @@ IContainer parentContainer typeof(BulkBloomPubCreator), typeof(PublishApi), typeof(LibraryPublishApi), + typeof(AccountApi), + typeof(AvatarApi), + typeof(AvatarCache), typeof(WorkspaceApi), typeof(BookCollectionHolder), typeof(WorkspaceTabSelection), @@ -439,6 +442,13 @@ IContainer parentContainer _scope.Resolve().RegisterWithApiHandler(server.ApiHandler); _scope.Resolve().RegisterWithApiHandler(server.ApiHandler); _scope.Resolve().RegisterWithApiHandler(server.ApiHandler); + var accountApi = _scope.Resolve(); + accountApi.RegisterWithApiHandler(server.ApiHandler); + accountApi.RestoreSavedLoginIfAny(); + // Register the avatar endpoint. (The persisted known-photo map that gives a previously + // logged-in user their nicer avatar source across restarts is loaded lazily by AvatarCache + // on its first use -- serving the first avatar request -- not eagerly here.) + _scope.Resolve().RegisterWithApiHandler(server.ApiHandler); _scope.Resolve().RegisterWithApiHandler(server.ApiHandler); _scope.Resolve().RegisterWithApiHandler(server.ApiHandler); _scope.Resolve().RegisterWithApiHandler(server.ApiHandler); diff --git a/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs b/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs index 6356067c24c4..87bc200bb3a6 100644 --- a/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs +++ b/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs @@ -358,16 +358,6 @@ bool changeUploader } } - internal void LogIn() - { - BloomLibraryAuthentication.LogIn(); - } - - internal void LogOut() - { - _uploader.Logout(); - } - internal LicenseState LicenseType { get diff --git a/src/BloomExe/WebLibraryIntegration/BloomLibraryBookApiClient.cs b/src/BloomExe/WebLibraryIntegration/BloomLibraryBookApiClient.cs index 095d6d9c3cff..b360d4d61f4e 100644 --- a/src/BloomExe/WebLibraryIntegration/BloomLibraryBookApiClient.cs +++ b/src/BloomExe/WebLibraryIntegration/BloomLibraryBookApiClient.cs @@ -5,6 +5,7 @@ using System.Net; using System.Text; using System.Threading; +using System.Threading.Tasks; using System.Windows.Forms; using Bloom.Book; using Bloom.Properties; @@ -32,6 +33,13 @@ public VersionCannotUploadException(string message) // So we'll start with this and replace the parse-server-specific bits as we go. public class BloomLibraryBookApiClient { + /// + /// Raised whenever the logged-in state or account changes, i.e. after SetLoginData() or + /// Logout(). Subscribers (e.g. AccountApi) use this as the single source of truth for + /// broadcasting the current login state to the front end. + /// + public event EventHandler LoginDataChanged; + const string kHost = "https://api.bloomlibrary.org"; //const string kHost = "http://localhost:7071"; // For local testing @@ -373,6 +381,11 @@ private void SetCommonHeadersAndParameters( request.AddQueryParameter("env", "dev"); } + /// + /// Record a successful login: stores the account/session info both in memory and in the + /// persisted user Settings (so a later run of Bloom, or the command line, can restore it), + /// and raises LoginDataChanged so subscribers (e.g. AccountApi) can broadcast the new state. + /// public void SetLoginData( string account, string userId, @@ -388,8 +401,47 @@ string destination Settings.Default.Save(); _userId = userId; _authenticationToken = sessionToken; + LoginDataChanged?.Invoke(this, EventArgs.Empty); } + /// + /// Attempt to restore a previously-saved login from Settings, purely from persisted state + /// (no network call). Only succeeds if we have a saved session token and user id, AND the + /// saved login was for the same destination (sandbox vs. production) that is being requested + /// now -- a sandbox token is meaningless against production and vice versa. On success, this + /// calls SetLoginData (which will re-save the same values and raise LoginDataChanged) and + /// returns true. On failure, it does NOT clear any settings, because a mismatch here (e.g. + /// wrong destination) doesn't mean the saved data is invalid, just that it doesn't apply to + /// the destination requested this time. + /// + public bool TryRestoreSavedLogin(string destination) + { + if ( + string.IsNullOrEmpty(Settings.Default.LastLoginSessionToken) + || string.IsNullOrEmpty(Settings.Default.LastLoginUserId) + ) + return false; + + if (Settings.Default.LastLoginDest != destination) + return false; + + SetLoginData( + Settings.Default.WebUserId, + Settings.Default.LastLoginUserId, + Settings.Default.LastLoginSessionToken, + destination + ); + + return true; + } + + /// + /// Used by the command-line uploader (BulkUploader) to sign in using the login most recently + /// performed from the Bloom UI, since the command line itself has no way to log in + /// interactively. Delegates the settings-only restore logic to TryRestoreSavedLogin, but adds + /// the extra validation and progress reporting that the command-line caller needs: the saved + /// email must match the -u argument, and failures are reported to the given IProgress. + /// public bool AttemptSignInAgainForCommandLine( string userEmail, string destination, @@ -428,14 +480,20 @@ IProgress progress return false; } - SetLoginData( - Settings.Default.WebUserId, - Settings.Default.LastLoginUserId, - Settings.Default.LastLoginSessionToken, - destination - ); + return TryRestoreSavedLogin(destination); + } - return true; + /// + /// Validate the current session token against the server. This is currently a stub: there is + /// no "who am I" endpoint on api.bloomlibrary.org yet. TODO: once one exists (e.g. + /// GET v1/users/me), call it here, and if it gives a DEFINITIVE answer that the token is + /// invalid (e.g. an HTTP 401), call Logout(includeFirebaseLogout: false). A network failure or + /// being offline is NOT evidence the token is invalid, so it must not trigger a logout -- + /// only a clear rejection from the server should. + /// + public Task ValidateTokenAsync() + { + return Task.CompletedTask; } // Don't even THINK of making this mutable so each unit test uses a different class. @@ -555,14 +613,27 @@ public dynamic GetBookRecords(string bookInstanceId, bool includeLanguageInfo) return rawResult.results; } + /// + /// Log the user out: clears the in-memory login state as well as everything in Settings that + /// would allow TryRestoreSavedLogin/AttemptSignInAgainForCommandLine to sign back in + /// automatically. This is a security fix: since we now restore saved logins at startup, + /// leaving any of the saved login settings in place after logging out would let "sign out, + /// then restart Bloom" silently sign the user back in. Raises LoginDataChanged at the end so + /// subscribers (e.g. AccountApi) can broadcast the now-signed-out state. + /// public void Logout(bool includeFirebaseLogout = true) { Settings.Default.WebUserId = ""; // Should not be able to log in again just by restarting + Settings.Default.LastLoginSessionToken = ""; + Settings.Default.LastLoginUserId = ""; + Settings.Default.LastLoginDest = ""; + Settings.Default.Save(); _authenticationToken = null; Account = ""; _userId = ""; if (includeFirebaseLogout) BloomLibraryAuthentication.Logout(); + LoginDataChanged?.Invoke(this, EventArgs.Empty); } /// diff --git a/src/BloomExe/WebLibraryIntegration/BookUpload.cs b/src/BloomExe/WebLibraryIntegration/BookUpload.cs index 3068e64a4c97..71883345769f 100644 --- a/src/BloomExe/WebLibraryIntegration/BookUpload.cs +++ b/src/BloomExe/WebLibraryIntegration/BookUpload.cs @@ -70,7 +70,12 @@ internal static bool UseSandboxByDefault get { #if DEBUG - return true; + // TEMPORARY: the dev/sandbox backend (dev-server.bloomlibrary.org) is returning + // HTTP 500 for all requests, which breaks login and library queries. Point debug + // builds at production (bloomlibrary.org) until the dev backend is healthy again. + // Revert this to `return true;` once the sandbox is back so debug uploads don't + // hit production. + return false; #else var temp = Environment.GetEnvironmentVariable("BloomSandbox"); if (string.IsNullOrWhiteSpace(temp)) diff --git a/src/BloomExe/web/AvatarCache.cs b/src/BloomExe/web/AvatarCache.cs new file mode 100644 index 000000000000..2c8733ec6db1 --- /dev/null +++ b/src/BloomExe/web/AvatarCache.cs @@ -0,0 +1,382 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Net.Http; +using System.Threading.Tasks; +using Bloom.Utils; +using Newtonsoft.Json; +using SIL.IO; +using SIL.Reporting; + +namespace Bloom.web +{ + /// + /// A general, person-keyed cache of avatar image BYTES, served by Bloom's local server. Avatars + /// are shown both for the logged-in user and for other team members in Team Collections, so the + /// cache is keyed by person (the MD5 of a normalized email, matching Gravatar's scheme) rather + /// than being tied to the current login. Caching the actual bytes lets avatars survive a restart + /// and work offline (important for Bloom's low-connectivity users) and stops us re-pinging remote + /// avatar hosts on every render. We attempt a fresh fetch once per run of Bloom (falling back to the + /// cached bytes if it fails), so a user who changes their avatar sees the change after a restart. + /// + /// For a given key we resolve a source in this order: a "known" provider photo URL for that person + /// (a nicer source than Gravatar) if we happen to have one, otherwise Gravatar. Right now the only + /// known photo we can populate is the logged-in user's (captured from the browser login callback's + /// photoUrl); teammates never logged into this Bloom, so they resolve via Gravatar. The known-photo + /// map is persisted and built to grow later. + /// + public class AvatarCache + { + // A single shared HttpClient is the recommended pattern; reusing it avoids socket exhaustion. + // The timeout keeps a slow/hung avatar host from blocking the request thread indefinitely; on + // timeout we simply treat it as a miss (the front end then shows generated initials). + private static readonly HttpClient s_httpClient = new HttpClient + { + Timeout = TimeSpan.FromSeconds(10), + }; + + private readonly string _cacheFolder; + private readonly string _knownPhotoUrlsPath; + private readonly string _indexPath; + + // Guards the two in-memory maps and their persisted (small JSON) files. The potentially-large + // image-byte files, and network fetches, happen OUTSIDE this lock so slow disk or a slow + // download never blocks other avatar lookups. + private readonly object _lock = new object(); + + // md5(email) -> a provider photo URL (e.g. a Google/Firebase photoURL) that is a nicer source + // than Gravatar for that person. Persisted so it survives a restart. + private Dictionary _knownPhotoUrls; + + // md5(email) -> metadata about the image bytes we have cached on disk for that person. + private Dictionary _index; + + // Keys we've already refreshed (attempted a fresh fetch for) during THIS run of Bloom. NOT + // persisted: emptied on every launch, so the first lookup of each person after a restart + // re-attempts a fresh fetch. That is how a user who changed their Gravatar/Google photo sees the + // change -- by simply restarting Bloom. Within one run we then serve from the cache, so we do + // not re-hit the remote host on every render. + private readonly HashSet _refreshedThisRun = new HashSet(); + + private class CacheIndexEntry + { + public string SourceUrl; + public string ContentType; + } + + /// + /// The outcome of resolving an avatar: the image bytes and their content type. Null is used + /// (rather than an instance) to mean "we have nothing", which the endpoint turns into a 404. + /// + public class AvatarResult + { + public byte[] Bytes; + public string ContentType; + } + + /// + /// The result of fetching a source URL. Returned by FetchAsync (which tests override to avoid + /// real network traffic). Null means the fetch failed / 404 / produced no usable image. + /// + public class FetchResult + { + public byte[] Bytes; + public string ContentType; + } + + /// + /// Create the cache. cacheFolder defaults to a folder under Bloom's normal user-data area; + /// tests pass a temporary folder so they neither read nor pollute the real cache. Loads any + /// previously-persisted known-photo map and disk-cache index (this is what restores the map on + /// startup). + /// + public AvatarCache(string cacheFolder = null) + { + _cacheFolder = + cacheFolder ?? Path.Combine(ProjectContext.GetBloomAppDataFolder(), "AvatarCache"); + Directory.CreateDirectory(_cacheFolder); + _knownPhotoUrlsPath = Path.Combine(_cacheFolder, "knownPhotoUrls.json"); + _indexPath = Path.Combine(_cacheFolder, "index.json"); + Load(); + } + + /// + /// The MD5 (lowercase hex) of the normalized (trimmed, lowercased) email. This matches the + /// Gravatar scheme (and what react-avatar/md5Util use), so the logged-in user and a teammate + /// with the same email share one key. + /// + public static string Md5OfEmail(string email) + { + var normalized = (email ?? "").Trim().ToLowerInvariant(); + // MiscUtils.GetMd5HashOfString hashes the UTF8 bytes and returns lowercase hex -- exactly + // Gravatar's scheme -- so we reuse it rather than hand-rolling the hash+hex conversion. + return MiscUtils.GetMd5HashOfString(normalized); + } + + /// + /// The Gravatar URL for a key. d=404 makes Gravatar return a 404 (rather than a generic image) + /// when the person has no Gravatar, so our fetch treats "no Gravatar" as a miss and the front + /// end falls back to generated initials. + /// + public static string GravatarUrl(string md5) + { + return $"https://secure.gravatar.com/avatar/{md5}?d=404"; + } + + /// + /// Register (or, when photoUrl is null/empty, clear) the known provider photo URL for a person, + /// and persist the map. This is how the logged-in user gets a nicer-than-Gravatar source. + /// + public void SetKnownPhotoUrl(string md5, string photoUrl) + { + if (string.IsNullOrEmpty(md5)) + return; + // An empty/null photoUrl means "no known photo" -- same as clearing it. + if (string.IsNullOrEmpty(photoUrl)) + { + RemoveKnownPhotoUrl(md5); + return; + } + md5 = md5.ToLowerInvariant(); + lock (_lock) + { + if (_knownPhotoUrls.TryGetValue(md5, out var existing) && existing == photoUrl) + return; // unchanged; don't rewrite the file + _knownPhotoUrls[md5] = photoUrl; + SaveKnownPhotoUrls(); + } + } + + /// + /// Remove a person's known provider photo URL (e.g. on logout, so the logged-in user reverts to + /// Gravatar). Cached image files may remain; that's harmless. + /// + public void RemoveKnownPhotoUrl(string md5) + { + if (string.IsNullOrEmpty(md5)) + return; + md5 = md5.ToLowerInvariant(); + lock (_lock) + { + if (_knownPhotoUrls.Remove(md5)) + SaveKnownPhotoUrls(); + } + } + + /// + /// The known provider photo URL for a person, or null if we have none. Mainly for the API layer + /// and tests to inspect the map. + /// + public string GetKnownPhotoUrlOrNull(string md5) + { + if (string.IsNullOrEmpty(md5)) + return null; + md5 = md5.ToLowerInvariant(); + lock (_lock) + { + return _knownPhotoUrls.TryGetValue(md5, out var url) ? url : null; + } + } + + /// + /// Resolve the avatar bytes for a key. Once per run of Bloom (and also whenever the best source + /// has changed, e.g. a new login supplied a Google photo) we attempt a FRESH fetch, so a changed + /// avatar shows up on the next launch. If that fetch fails (offline) or the source has no image, + /// we fall back to a previously cached copy when we have one -- so an offline user keeps seeing + /// their last-known avatar and a restart will retry. Only when we have nothing at all do we + /// return null, which the endpoint turns into a 404 so the front end shows generated initials. + /// Later lookups within the same run are served from the cache without re-fetching. + /// + public async Task GetAvatarBytesAsync(string md5) + { + if (string.IsNullOrEmpty(md5)) + return null; + md5 = md5.ToLowerInvariant(); + + var desiredSource = GetSourceUrlFor(md5); + + // Attempt a fresh fetch if we haven't already this run, OR if the source we would use has + // changed since we last cached (so a newly-registered Google photo replaces a cached Gravatar + // right away). Otherwise, serve what we already have this run without re-fetching. + bool alreadyTriedThisRun; + bool sourceMatchesCache; + lock (_lock) + { + alreadyTriedThisRun = _refreshedThisRun.Contains(md5); + sourceMatchesCache = + _index.TryGetValue(md5, out var entry) + && entry != null + && entry.SourceUrl == desiredSource; + } + if (alreadyTriedThisRun && sourceMatchesCache) + return ReadCachedOrNull(md5); + + // Attempt a fresh fetch (outside the lock). A failure here is not worth alarming the user + // about; we fall back to any cached copy below. + FetchResult fetched = null; + try + { + fetched = await FetchAsync(desiredSource); + } + catch (Exception e) + { + Logger.WriteEvent( + "AvatarCache: fetch failed for " + desiredSource + ": " + e.Message + ); + } + lock (_lock) + { + _refreshedThisRun.Add(md5); // we've tried this run, whether or not it succeeded + } + + if (fetched == null || fetched.Bytes == null || fetched.Bytes.Length == 0) + { + // Offline, timeout, or the source has no image. Keep using the cached copy if we have + // one; otherwise there is nothing to show and the front end falls back to initials. + return ReadCachedOrNull(md5); + } + + // Success: update the cache (write the large file OUTSIDE the lock) and use the fresh bytes. + try + { + RobustFile.WriteAllBytes(FilePathFor(md5), fetched.Bytes); + lock (_lock) + { + _index[md5] = new CacheIndexEntry + { + SourceUrl = desiredSource, + ContentType = fetched.ContentType, + }; + SaveIndex(); + } + } + catch (Exception e) + { + // If we couldn't persist, still return the bytes we fetched; we just won't have a cache + // hit next time. + Logger.WriteEvent( + "AvatarCache: could not cache bytes for " + md5 + ": " + e.Message + ); + } + + return new AvatarResult { Bytes = fetched.Bytes, ContentType = fetched.ContentType }; + } + + // Return the bytes cached on disk for this key (from an earlier successful fetch, possibly in a + // previous run), or null if we have none. The (potentially large) file is read OUTSIDE the lock. + private AvatarResult ReadCachedOrNull(string md5) + { + string path; + string contentType; + lock (_lock) + { + if (!_index.TryGetValue(md5, out var entry) || entry == null) + return null; + path = FilePathFor(md5); + contentType = entry.ContentType; + } + try + { + if (RobustFile.Exists(path)) + return new AvatarResult + { + Bytes = RobustFile.ReadAllBytes(path), + ContentType = contentType, + }; + } + catch (Exception e) + { + Logger.WriteEvent( + "AvatarCache: could not read cached bytes for " + md5 + ": " + e.Message + ); + } + return null; + } + + /// + /// Download the bytes at the given URL. Virtual so tests can override it to return canned data + /// (and assert which source was chosen) without touching the network. Returns null on any + /// non-success status (including Gravatar's 404 for "no avatar"). + /// + protected virtual async Task FetchAsync(string url) + { + var response = await s_httpClient.GetAsync(url); + if (!response.IsSuccessStatusCode) + return null; + var bytes = await response.Content.ReadAsByteArrayAsync(); + if (bytes == null || bytes.Length == 0) + return null; + var contentType = response.Content.Headers.ContentType?.MediaType ?? "image/png"; + return new FetchResult { Bytes = bytes, ContentType = contentType }; + } + + // The source we would use for this key right now: a known provider photo URL if we have one, + // otherwise Gravatar. + private string GetSourceUrlFor(string md5) + { + lock (_lock) + { + if (_knownPhotoUrls.TryGetValue(md5, out var url) && !string.IsNullOrEmpty(url)) + return url; + } + return GravatarUrl(md5); + } + + // The on-disk path of the cached image bytes for a key. No extension: the content type lives in + // the index. The key is a 32-char hex string, so it is always a safe file name. + private string FilePathFor(string md5) + { + return Path.Combine(_cacheFolder, md5); + } + + private void Load() + { + _knownPhotoUrls = + ReadJson>(_knownPhotoUrlsPath) + ?? new Dictionary(); + _index = + ReadJson>(_indexPath) + ?? new Dictionary(); + } + + private static T ReadJson(string path) + where T : class + { + try + { + if (RobustFile.Exists(path)) + return JsonConvert.DeserializeObject(RobustFile.ReadAllText(path)); + } + catch (Exception e) + { + // A corrupt cache file should never be fatal; just start fresh. + Logger.WriteEvent("AvatarCache: could not read " + path + ": " + e.Message); + } + return null; + } + + // Callers hold _lock. + private void SaveKnownPhotoUrls() + { + WriteJson(_knownPhotoUrlsPath, _knownPhotoUrls); + } + + // Callers hold _lock. + private void SaveIndex() + { + WriteJson(_indexPath, _index); + } + + private static void WriteJson(string path, object value) + { + try + { + RobustFile.WriteAllText(path, JsonConvert.SerializeObject(value)); + } + catch (Exception e) + { + Logger.WriteEvent("AvatarCache: could not write " + path + ": " + e.Message); + } + } + } +} diff --git a/src/BloomExe/web/controllers/AccountApi.cs b/src/BloomExe/web/controllers/AccountApi.cs new file mode 100644 index 000000000000..e8f17a1b9fd0 --- /dev/null +++ b/src/BloomExe/web/controllers/AccountApi.cs @@ -0,0 +1,142 @@ +using System; +using System.Threading.Tasks; +using Bloom.Api; +using Bloom.Properties; +using Bloom.WebLibraryIntegration; + +namespace Bloom.web.controllers +{ + /// + /// App-wide API for the Bloom Library login state. This is the single place the front end goes + /// to find out whether the user is logged in to Bloom Library, and to log in or out. It is + /// intentionally independent of any particular screen (e.g. the Publish-to-Web screen): the + /// underlying login state lives in BloomLibraryBookApiClient, and this class just exposes it over + /// the API and broadcasts changes via websocket so every screen can stay in sync. + /// + public class AccountApi : IDisposable + { + // This goes out with our messages and, on the client side (typescript), messages are filtered + // down to the context that requested them. + private const string kWebSocketContext = "account"; + private const string kWebSocketEventId_loginStateChanged = "loginStateChanged"; + + private readonly BloomLibraryBookApiClient _client; + private readonly BloomWebSocketServer _webSocketServer; + private readonly AvatarCache _avatarCache; + + /// + /// Created by autofac, which creates the one instance and registers it with the server. + /// Subscribes to the BloomLibraryBookApiClient's LoginDataChanged event so that every change + /// to the login state (however it happens -- interactive login, logout, restoring a saved + /// login at startup) gets broadcast to the front end. + /// + public AccountApi( + BloomLibraryBookApiClient client, + BloomWebSocketServer webSocketServer, + AvatarCache avatarCache + ) + { + _client = client; + _webSocketServer = webSocketServer; + _avatarCache = avatarCache; + _client.LoginDataChanged += OnLoginDataChanged; + } + + // Fires whenever the login state changes (interactive login, logout, or a restored login at + // startup); re-broadcasts the current state so every screen stays in sync. + private void OnLoginDataChanged(object sender, EventArgs e) + { + BroadcastLoginState(); + } + + /// + /// Register the account/* endpoints: status (GET), login (POST), logout (POST). + /// + public void RegisterWithApiHandler(BloomApiHandler apiHandler) + { + apiHandler.RegisterEndpointHandler( + "account/status", + HandleStatus, + handleOnUiThread: false + ); + apiHandler.RegisterEndpointHandler( + "account/login", + HandleLogin, + handleOnUiThread: false + ); + apiHandler.RegisterEndpointHandler( + "account/logout", + HandleLogout, + handleOnUiThread: false + ); + } + + // GET account/status: report whether we are logged in, and to whom (the email, or empty). + private void HandleStatus(ApiRequest request) + { + request.ReplyWithJson(new { email = CurrentEmail }); + } + + // POST account/login: launch the external-browser login (the browser calls us back at + // external/login when it succeeds). + private void HandleLogin(ApiRequest request) + { + BloomLibraryAuthentication.LogIn(); + request.PostSucceeded(); + } + + private void HandleLogout(ApiRequest request) + { + // Capture the email BEFORE logging out (Logout clears it) so we can drop this user's known + // avatar photo from the cache, reverting them to Gravatar. Cached image files may remain; + // that's harmless. + var emailBeforeLogout = Settings.Default.WebUserId; + _client.Logout(); + if (!string.IsNullOrEmpty(emailBeforeLogout)) + _avatarCache.RemoveKnownPhotoUrl(AvatarCache.Md5OfEmail(emailBeforeLogout)); + // The state-change broadcast happens via the LoginDataChanged event handler above. + request.PostSucceeded(); + } + + // The email of the logged-in user, or empty when not logged in. + private string CurrentEmail => _client.LoggedIn ? Settings.Default.WebUserId : ""; + + /// + /// Called once at startup (from ProjectContext, after this API's endpoints are registered) to + /// restore a previously-saved login, if there is one for the current upload destination. + /// If a login is restored, the LoginDataChanged event fires and broadcasts the new state to + /// any UI that is already listening. Afterward, kicks off (fire-and-forget) a background + /// validation of the restored token, in case it has since been revoked. + /// + public void RestoreSavedLoginIfAny() + { + if (_client.TryRestoreSavedLogin(BookUpload.Destination)) + { + Task.Run(() => _client.ValidateTokenAsync()); + } + } + + // Send the current login state to the front end over the "account" websocket context so every + // screen listening for it updates. + private void BroadcastLoginState() + { + dynamic bundle = new DynamicJson(); + bundle.email = CurrentEmail; + _webSocketServer.SendBundle( + kWebSocketContext, + kWebSocketEventId_loginStateChanged, + bundle + ); + } + + /// + /// Unsubscribe from the client's event so this instance doesn't leak a subscription onto a + /// singleton that will outlive it (the project scope disposes IDisposable singletons it + /// created, which is when this runs). + /// + public void Dispose() + { + _client.LoginDataChanged -= OnLoginDataChanged; + } + } +} diff --git a/src/BloomExe/web/controllers/AvatarApi.cs b/src/BloomExe/web/controllers/AvatarApi.cs new file mode 100644 index 000000000000..f1209cab0924 --- /dev/null +++ b/src/BloomExe/web/controllers/AvatarApi.cs @@ -0,0 +1,70 @@ +using System.IO; +using System.Net; +using System.Threading.Tasks; +using Bloom.Api; + +namespace Bloom.web.controllers +{ + /// + /// Serves avatar images for a person (by email) from the local server, backed by AvatarCache. + /// This is deliberately independent of the login/account feature because avatars are broader than + /// the logged-in user: Team Collection UI asks for teammates' avatars too. Every BloomAvatar in + /// the front end sources its image from here, so all avatars are cached, offline-capable, and no + /// longer re-ping remote hosts on each render. + /// + public class AvatarApi + { + private readonly AvatarCache _avatarCache; + + // Created by autofac, which creates the one instance and registers it with the server. + public AvatarApi(AvatarCache avatarCache) + { + _avatarCache = avatarCache; + } + + /// + /// Register the avatar endpoint. It does not need the UI thread (pure data), and uses + /// requiresSync:false so a slow avatar download never holds the global API lock. + /// + public void RegisterWithApiHandler(BloomApiHandler apiHandler) + { + apiHandler.RegisterAsyncEndpointHandler( + "avatar", + HandleAvatar, + handleOnUiThread: false, + requiresSync: false + ); + } + + /// + /// GET /bloom/api/avatar?email=<email> → the avatar image bytes for that email, or 404 + /// when we have nothing (which lets react-avatar fall back to generated initials). The server + /// normalizes + hashes the email itself, so the front end only has to pass the email. + /// + private async Task HandleAvatar(ApiRequest request) + { + var email = request.GetParamOrNull("email"); + if (string.IsNullOrEmpty(email)) + { + request.Failed(HttpStatusCode.NotFound, "no email supplied"); + return; + } + + var md5 = AvatarCache.Md5OfEmail(email); + var result = await _avatarCache.GetAvatarBytesAsync(md5); + if (result == null) + { + // Nothing available (no known photo, no Gravatar, or offline). 404 tells the front end + // to show initials. + request.Failed(HttpStatusCode.NotFound, "no avatar"); + return; + } + + request.ReplyWithStreamContent( + new MemoryStream(result.Bytes), + result.ContentType, + result.Bytes.Length + ); + } + } +} diff --git a/src/BloomExe/web/controllers/ExternalApi.cs b/src/BloomExe/web/controllers/ExternalApi.cs index 473193851308..f969c606019e 100644 --- a/src/BloomExe/web/controllers/ExternalApi.cs +++ b/src/BloomExe/web/controllers/ExternalApi.cs @@ -16,14 +16,13 @@ namespace Bloom.web.controllers /// public class ExternalApi { - public static event EventHandler LoginSuccessful; - private BloomLibraryBookApiClient _bloomLibraryBookApiClient; private readonly CollectionModel _collectionModel; private readonly EditingModel _editingModel; private readonly WorkspaceTabSelection _tabSelection; private readonly BookServer _bookServer; private readonly CollectionSettings _collectionSettings; + private readonly AvatarCache _avatarCache; // Re-entrancy guard for process-book. ProcessBook occupies the UI thread but pumps the // Windows message loop via Application.DoEvents() (both the pre-loop below and the per-page @@ -71,7 +70,8 @@ public ExternalApi( EditingModel editingModel, WorkspaceTabSelection tabSelection, BookServer bookServer, - CollectionSettings collectionSettings + CollectionSettings collectionSettings, + AvatarCache avatarCache ) { _bloomLibraryBookApiClient = bloomLibraryBookApiClient; @@ -80,6 +80,7 @@ CollectionSettings collectionSettings _tabSelection = tabSelection; _bookServer = bookServer; _collectionSettings = collectionSettings; + _avatarCache = avatarCache; } /// @@ -117,14 +118,33 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) string token = requestData.sessionToken; string email = requestData.email; string userId = requestData.userId; + // photoUrl (the user's Google/Firebase profile picture) is OPTIONAL and read + // defensively: the website and desktop deploy independently, so an older blorg + // build will not send this field at all. A missing/null value is normal and + // simply means "no picture" (the avatar falls back to Gravatar/initials). Only + // THIS field gets this lenient treatment; the others keep the fail-fast behavior. + string photoUrl = requestData.IsDefined("photoUrl") + ? (string)requestData.photoUrl + : null; //Debug.WriteLine("Got login data " + email + " with token " + token + " and id " + userId); + + // Register the logged-in user's profile picture (if any) in the person-keyed + // avatar cache BEFORE SetLoginData, because SetLoginData broadcasts the new login + // state, which prompts the front end to request the avatar. Registering first + // guarantees that request resolves to the Google photo rather than racing ahead + // and caching Gravatar. When photoUrl is absent/empty this is a no-op and the + // avatar falls back to Gravatar/initials. + if (!string.IsNullOrEmpty(photoUrl)) + { + _avatarCache.SetKnownPhotoUrl(AvatarCache.Md5OfEmail(email), photoUrl); + } + _bloomLibraryBookApiClient.SetLoginData( email, userId, token, BookUpload.Destination ); - LoginSuccessful?.Invoke(this, null); request.PostSucceeded(); diff --git a/src/BloomExe/web/controllers/LibraryPublishApi.cs b/src/BloomExe/web/controllers/LibraryPublishApi.cs index e603c4a1b92d..121834ab5228 100644 --- a/src/BloomExe/web/controllers/LibraryPublishApi.cs +++ b/src/BloomExe/web/controllers/LibraryPublishApi.cs @@ -11,7 +11,6 @@ using Bloom.Workspace; using L10NSharp; using SIL.Progress; -using SIL.Reporting; namespace Bloom.web.controllers { @@ -28,7 +27,6 @@ class LibraryPublishApi private const string kWebSocketEventId_uploadSuccessful = "uploadSuccessful"; // must match what is in LibraryPublishSteps.tsx private const string kWebSocketEventId_uploadCanceled = "uploadCanceled"; // must match what is in LibraryPublishSteps.tsx - private const string kWebSocketEventId_loginSuccessful = "loginSuccessful"; // must match what is in LibraryPublishSteps.tsx private PublishView _publishView; private PublishModel _publishModel; @@ -53,16 +51,6 @@ PublishModel publishModel _webSocketProgress = progress.WithL10NPrefix("PublishTab.Upload."); _webSocketProgress.LogAllMessages = true; _progress = new WebProgressAdapter(_webSocketProgress); - - ExternalApi.LoginSuccessful += (sender, args) => - { - Logger.WriteEvent("External login successful. Sending message to js-land."); - _webSocketServer.SendString( - kWebSocketContext, - kWebSocketEventId_loginSuccessful, - Model?.WebUserId - ); - }; } private string CurrentSignLanguageName @@ -131,13 +119,6 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) HandleUploadAfterChangingBookId, true ); - apiHandler.RegisterEndpointHandler( - "libraryPublish/checkForLoggedInUser", - HandleCheckForLoggedInUser, - true - ); - apiHandler.RegisterEndpointHandler("libraryPublish/login", HandleLogin, true); - apiHandler.RegisterEndpointHandler("libraryPublish/logout", HandleLogout, true); apiHandler.RegisterEndpointHandler( "libraryPublish/agreementsAccepted", HandleAgreementsAccepted, @@ -467,36 +448,6 @@ private async Task HandleUploadAfterChangingBookId(ApiRequest request) await HandleUpload(request); } - private void HandleCheckForLoggedInUser(ApiRequest request) - { - // Why not just reply with the WebUserId instead? - // Because we already have this event hooked up for the user-initiated log in process. - // So it simplifies the client to just reuse this web socket event. - if (Model.LoggedIn) - { - Logger.WriteEvent("User already logged in. Sending message to js-land."); - _webSocketServer.SendString( - kWebSocketContext, - kWebSocketEventId_loginSuccessful, - Model?.WebUserId - ); - } - request.PostSucceeded(); - } - - private void HandleLogin(ApiRequest request) - { - Model.LogIn(); - Logger.WriteEvent("User attempting to login to bloomlibrary.org."); - request.PostSucceeded(); - } - - private void HandleLogout(ApiRequest request) - { - Model.LogOut(); - request.PostSucceeded(); - } - private void HandleAgreementsAccepted(ApiRequest request) { if (request.HttpMethod == HttpMethods.Get) diff --git a/src/BloomTests/WebLibraryIntegration/BloomLibraryBookApiClientLoginTests.cs b/src/BloomTests/WebLibraryIntegration/BloomLibraryBookApiClientLoginTests.cs new file mode 100644 index 000000000000..42aa2a7fa56d --- /dev/null +++ b/src/BloomTests/WebLibraryIntegration/BloomLibraryBookApiClientLoginTests.cs @@ -0,0 +1,147 @@ +using Bloom.Properties; +using Bloom.WebLibraryIntegration; +using NUnit.Framework; + +namespace BloomTests.WebLibraryIntegration +{ + /// + /// Tests for the app-wide login state added to BloomLibraryBookApiClient: SetLoginData, + /// TryRestoreSavedLogin, and Logout, along with the LoginDataChanged event that lets + /// other parts of the app (e.g. AccountApi) react to login-state changes. + /// + /// These tests read and write Bloom.Properties.Settings.Default, which is the developer's + /// actual persisted configuration, so we carefully save the original values in Setup and + /// restore them in TearDown. + /// + [TestFixture] + public class BloomLibraryBookApiClientLoginTests + { + private string _origWebUserId; + private string _origLastLoginSessionToken; + private string _origLastLoginUserId; + private string _origLastLoginDest; + + private BloomLibraryBookApiClient _client; + + [SetUp] + public void Setup() + { + _origWebUserId = Settings.Default.WebUserId; + _origLastLoginSessionToken = Settings.Default.LastLoginSessionToken; + _origLastLoginUserId = Settings.Default.LastLoginUserId; + _origLastLoginDest = Settings.Default.LastLoginDest; + + _client = new BloomLibraryBookApiClient(); + } + + [TearDown] + public void TearDown() + { + Settings.Default.WebUserId = _origWebUserId; + Settings.Default.LastLoginSessionToken = _origLastLoginSessionToken; + Settings.Default.LastLoginUserId = _origLastLoginUserId; + Settings.Default.LastLoginDest = _origLastLoginDest; + Settings.Default.Save(); + } + + [Test] + public void TryRestoreSavedLogin_NoSavedToken_ReturnsFalseAndStaysLoggedOut() + { + // Sanity check / arrange: no saved session token. + Settings.Default.LastLoginSessionToken = ""; + Settings.Default.LastLoginUserId = "someUserId"; + Settings.Default.LastLoginDest = "production"; + Assert.That(Settings.Default.LastLoginSessionToken, Is.EqualTo(string.Empty)); + + var result = _client.TryRestoreSavedLogin("production"); + + Assert.That(result, Is.False); + Assert.That(_client.LoggedIn, Is.False); + } + + [Test] + public void TryRestoreSavedLogin_DestinationDoesNotMatch_ReturnsFalseAndDoesNotClearSettings() + { + // Arrange: a fully saved login, but for a different destination than we'll request. + Settings.Default.WebUserId = "someone@example.com"; + Settings.Default.LastLoginSessionToken = "savedToken"; + Settings.Default.LastLoginUserId = "savedUserId"; + Settings.Default.LastLoginDest = "sandbox"; + Settings.Default.Save(); + + // Sanity check the arrange step actually took effect. + Assert.That(Settings.Default.LastLoginDest, Is.EqualTo("sandbox")); + + var result = _client.TryRestoreSavedLogin("production"); + + Assert.That(result, Is.False); + Assert.That(_client.LoggedIn, Is.False); + + // A destination mismatch must NOT be treated as an invalid saved login; the settings + // should be left untouched so a later call with the matching destination can still work. + Assert.That(Settings.Default.WebUserId, Is.EqualTo("someone@example.com")); + Assert.That(Settings.Default.LastLoginSessionToken, Is.EqualTo("savedToken")); + Assert.That(Settings.Default.LastLoginUserId, Is.EqualTo("savedUserId")); + Assert.That(Settings.Default.LastLoginDest, Is.EqualTo("sandbox")); + } + + [Test] + public void TryRestoreSavedLogin_MatchingDestinationAndSavedToken_ReturnsTrueAndLogsIn() + { + // Arrange: a fully saved login matching the destination we'll request. + Settings.Default.WebUserId = "someone@example.com"; + Settings.Default.LastLoginSessionToken = "savedToken"; + Settings.Default.LastLoginUserId = "savedUserId"; + Settings.Default.LastLoginDest = "production"; + Settings.Default.Save(); + + // Sanity check the arrange step actually took effect, and that we're starting logged out. + Assert.That(Settings.Default.LastLoginSessionToken, Is.EqualTo("savedToken")); + Assert.That(_client.LoggedIn, Is.False); + + var result = _client.TryRestoreSavedLogin("production"); + + Assert.That(result, Is.True); + Assert.That(_client.LoggedIn, Is.True); + Assert.That(_client.Account, Is.EqualTo("someone@example.com")); + Assert.That(_client.UserId, Is.EqualTo("savedUserId")); + } + + [Test] + public void Logout_ClearsAllSavedLoginSettingsAndLogsOut() + { + // Arrange: log in first, so there's something to log out of. + _client.SetLoginData("someone@example.com", "savedUserId", "savedToken", "production"); + + // Sanity check the arrange step actually logged us in and saved settings. + Assert.That(_client.LoggedIn, Is.True); + Assert.That(Settings.Default.LastLoginSessionToken, Is.EqualTo("savedToken")); + + // Pass includeFirebaseLogout: false so this doesn't try to open a browser window. + _client.Logout(includeFirebaseLogout: false); + + Assert.That(_client.LoggedIn, Is.False); + Assert.That(Settings.Default.WebUserId, Is.EqualTo(string.Empty)); + Assert.That(Settings.Default.LastLoginSessionToken, Is.EqualTo(string.Empty)); + Assert.That(Settings.Default.LastLoginUserId, Is.EqualTo(string.Empty)); + Assert.That(Settings.Default.LastLoginDest, Is.EqualTo(string.Empty)); + } + + [Test] + public void LoginDataChanged_FiresOnSetLoginDataAndOnLogout() + { + var invocationCount = 0; + _client.LoginDataChanged += (sender, args) => invocationCount++; + + // Sanity check we're starting from zero invocations. + Assert.That(invocationCount, Is.EqualTo(0)); + + _client.SetLoginData("someone@example.com", "savedUserId", "savedToken", "production"); + Assert.That(invocationCount, Is.EqualTo(1)); + + // Pass includeFirebaseLogout: false so this doesn't try to open a browser window. + _client.Logout(includeFirebaseLogout: false); + Assert.That(invocationCount, Is.EqualTo(2)); + } + } +} diff --git a/src/BloomTests/web/AvatarApiTests.cs b/src/BloomTests/web/AvatarApiTests.cs new file mode 100644 index 000000000000..39bd3a9f20fa --- /dev/null +++ b/src/BloomTests/web/AvatarApiTests.cs @@ -0,0 +1,180 @@ +using System.Net; +using System.Threading; +using System.Threading.Tasks; +using Bloom.Api; +using Bloom.Book; +using Bloom.Properties; +using Bloom.web; +using Bloom.web.controllers; +using Bloom.WebLibraryIntegration; +using NUnit.Framework; +using SIL.TestUtilities; + +namespace BloomTests.web +{ + /// + /// Integration tests that exercise the real HTTP surface of the avatar feature: the + /// GET /api/avatar endpoint (AvatarApi), and the external/login wiring in ExternalApi that + /// registers the logged-in user's photo URL in the AvatarCache. Fetching is stubbed via a + /// testable AvatarCache so nothing here touches the network, and a TemporaryFolder isolates the + /// on-disk cache. + /// + [TestFixture] + public class AvatarApiTests + { + private BloomServer _server; + private TemporaryFolder _folder; + private TestableAvatarCache _cache; + + // Login settings we touch via ExternalApi's SetLoginData call; saved/restored so we don't + // clobber the developer's real persisted configuration. + private string _origWebUserId; + private string _origLastLoginSessionToken; + private string _origLastLoginUserId; + private string _origLastLoginDest; + + private class TestableAvatarCache : AvatarCache + { + public byte[] BytesToReturn; + public string ContentTypeToReturn = "image/png"; + + public TestableAvatarCache(string cacheFolder) + : base(cacheFolder) { } + + protected override Task FetchAsync(string url) + { + if (BytesToReturn == null) + return Task.FromResult(null); + return Task.FromResult( + new FetchResult { Bytes = BytesToReturn, ContentType = ContentTypeToReturn } + ); + } + } + + [SetUp] + public void Setup() + { + // Share the same monitor as the other server tests so we never run two servers on the + // fixed test port at once. + Monitor.Enter(EndpointHandlerTests._portMonitor); + + _origWebUserId = Settings.Default.WebUserId; + _origLastLoginSessionToken = Settings.Default.LastLoginSessionToken; + _origLastLoginUserId = Settings.Default.LastLoginUserId; + _origLastLoginDest = Settings.Default.LastLoginDest; + + _folder = new TemporaryFolder("AvatarApiTests"); + _cache = new TestableAvatarCache(_folder.Path); + _server = new BloomServer(new BookSelection()); + new AvatarApi(_cache).RegisterWithApiHandler(_server.ApiHandler); + } + + [TearDown] + public void TearDown() + { + _server.Dispose(); + _server = null; + _folder.Dispose(); + + Settings.Default.WebUserId = _origWebUserId; + Settings.Default.LastLoginSessionToken = _origLastLoginSessionToken; + Settings.Default.LastLoginUserId = _origLastLoginUserId; + Settings.Default.LastLoginDest = _origLastLoginDest; + Settings.Default.Save(); + + Monitor.Exit(EndpointHandlerTests._portMonitor); + } + + private string AvatarUrl(string email) + { + return BloomServer.ServerUrlWithBloomPrefixEndingInSlash + + "api/avatar?email=" + + WebUtility.UrlEncode(email); + } + + [Test] + public void GetAvatar_WhenCacheHasImage_ReturnsBytes() + { + _cache.BytesToReturn = new byte[] { 10, 20, 30 }; + _server.EnsureListening(); + + byte[] data; + using (var client = new WebClient()) + { + data = client.DownloadData(AvatarUrl("user@example.com")); + } + + Assert.That(data, Is.EqualTo(new byte[] { 10, 20, 30 })); + } + + [Test] + public void GetAvatar_WhenNothingAvailable_Returns404() + { + _cache.BytesToReturn = null; // simulate no known photo, no Gravatar, offline + _server.EnsureListening(); + + using (var client = new WebClient()) + { + var ex = Assert.Throws(() => + client.DownloadData(AvatarUrl("nobody@example.com")) + ); + // 404 is the contract that makes react-avatar fall back to generated initials. + Assert.That( + ((HttpWebResponse)ex.Response).StatusCode, + Is.EqualTo(HttpStatusCode.NotFound) + ); + } + } + + [Test] + public void ExternalLogin_WithPhotoUrl_RegistersKnownPhotoMapping() + { + var client = new BloomLibraryBookApiClient(); + new ExternalApi(client, null, null, null, null, null, _cache).RegisterWithApiHandler( + _server.ApiHandler + ); + + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + // Sanity: no mapping before login. + Assert.That(_cache.GetKnownPhotoUrlOrNull(md5), Is.Null); + + var result = ApiTest.PostString( + _server, + "external/login", + "{\"sessionToken\":\"t\",\"email\":\"user@example.com\",\"userId\":\"u\",\"photoUrl\":\"https://photos.example/pic.jpg\"}", + ApiTest.ContentType.JSON + ); + + Assert.That(result, Is.EqualTo("OK")); + Assert.That( + _cache.GetKnownPhotoUrlOrNull(md5), + Is.EqualTo("https://photos.example/pic.jpg") + ); + } + + [Test] + public void ExternalLogin_WithoutPhotoUrl_DoesNotRegisterAndDoesNotThrow() + { + var client = new BloomLibraryBookApiClient(); + new ExternalApi(client, null, null, null, null, null, _cache).RegisterWithApiHandler( + _server.ApiHandler + ); + + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + // Sanity: no mapping before login. + Assert.That(_cache.GetKnownPhotoUrlOrNull(md5), Is.Null); + + // A login body that omits photoUrl entirely (an older website build). This must succeed + // and leave no mapping, so the avatar falls back to Gravatar/initials. + var result = ApiTest.PostString( + _server, + "external/login", + "{\"sessionToken\":\"t\",\"email\":\"user@example.com\",\"userId\":\"u\"}", + ApiTest.ContentType.JSON + ); + + Assert.That(result, Is.EqualTo("OK")); + Assert.That(_cache.GetKnownPhotoUrlOrNull(md5), Is.Null); + } + } +} diff --git a/src/BloomTests/web/AvatarCacheTests.cs b/src/BloomTests/web/AvatarCacheTests.cs new file mode 100644 index 000000000000..6e00a85e7711 --- /dev/null +++ b/src/BloomTests/web/AvatarCacheTests.cs @@ -0,0 +1,287 @@ +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using Bloom.web; +using NUnit.Framework; +using SIL.TestUtilities; + +namespace BloomTests.web +{ + /// + /// Tests for AvatarCache, the person-keyed cache of avatar image bytes. Fetching is stubbed via a + /// testable subclass so nothing here touches the network; a TemporaryFolder isolates the on-disk + /// cache so we neither read nor pollute the real one. + /// + [TestFixture] + public class AvatarCacheTests + { + private TemporaryFolder _folder; + + /// + /// An AvatarCache whose network fetch is replaced with a canned response, recording every URL + /// it was asked to fetch so tests can assert which source was chosen and how many times we + /// fetched (to prove cache hits avoid refetching). + /// + private class TestableAvatarCache : AvatarCache + { + public readonly List FetchedUrls = new List(); + + // null => simulate a miss (offline / 404 / no image). + public byte[] BytesToReturn; + public string ContentTypeToReturn = "image/png"; + + public TestableAvatarCache(string cacheFolder) + : base(cacheFolder) { } + + protected override Task FetchAsync(string url) + { + FetchedUrls.Add(url); + if (BytesToReturn == null) + return Task.FromResult(null); + return Task.FromResult( + new FetchResult { Bytes = BytesToReturn, ContentType = ContentTypeToReturn } + ); + } + } + + [SetUp] + public void Setup() + { + _folder = new TemporaryFolder("AvatarCacheTests"); + } + + [TearDown] + public void TearDown() + { + _folder.Dispose(); + } + + [Test] + public void Md5OfEmail_NormalizesTrimAndCase_AndMatchesGravatarScheme() + { + // Trimming + lowercasing must yield the same key regardless of surrounding whitespace/case. + Assert.That( + AvatarCache.Md5OfEmail(" User@Example.COM "), + Is.EqualTo(AvatarCache.Md5OfEmail("user@example.com")) + ); + + // And it must equal Gravatar's own documented example hash, proving we use the same scheme + // (so a person's key here lines up with their Gravatar). + Assert.That( + AvatarCache.Md5OfEmail("MyEmailAddress@example.com "), + Is.EqualTo("0bc83cb571cd1c50ba6f3e8a78ef1346") + ); + } + + [Test] + public void GetAvatarBytes_WithKnownPhotoUrl_FetchesThatSource() + { + var cache = new TestableAvatarCache(_folder.Path); + cache.BytesToReturn = new byte[] { 1, 2, 3 }; + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + + // Sanity: no known photo before we register one. + Assert.That(cache.GetKnownPhotoUrlOrNull(md5), Is.Null); + + cache.SetKnownPhotoUrl(md5, "https://photos.example/pic.jpg"); + Assert.That( + cache.GetKnownPhotoUrlOrNull(md5), + Is.EqualTo("https://photos.example/pic.jpg") + ); + + var result = cache.GetAvatarBytesAsync(md5).Result; + + Assert.That(result, Is.Not.Null); + Assert.That(result.Bytes, Is.EqualTo(new byte[] { 1, 2, 3 })); + Assert.That(result.ContentType, Is.EqualTo("image/png")); + Assert.That(cache.FetchedUrls, Has.Count.EqualTo(1)); + Assert.That(cache.FetchedUrls[0], Is.EqualTo("https://photos.example/pic.jpg")); + } + + [Test] + public void GetAvatarBytes_WithoutKnownPhotoUrl_FallsBackToGravatar() + { + var cache = new TestableAvatarCache(_folder.Path); + cache.BytesToReturn = new byte[] { 4, 5 }; + var md5 = AvatarCache.Md5OfEmail("teammate@example.com"); + + // Sanity: no known photo for this person (the normal teammate case). + Assert.That(cache.GetKnownPhotoUrlOrNull(md5), Is.Null); + + var result = cache.GetAvatarBytesAsync(md5).Result; + + Assert.That(result, Is.Not.Null); + Assert.That(cache.FetchedUrls, Has.Count.EqualTo(1)); + Assert.That(cache.FetchedUrls[0], Is.EqualTo(AvatarCache.GravatarUrl(md5))); + } + + [Test] + public void GetAvatarBytes_CacheHit_DoesNotRefetch() + { + var cache = new TestableAvatarCache(_folder.Path); + cache.BytesToReturn = new byte[] { 7 }; + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + + var first = cache.GetAvatarBytesAsync(md5).Result; + Assert.That(first, Is.Not.Null, "sanity: first lookup should populate the cache"); + Assert.That(cache.FetchedUrls, Has.Count.EqualTo(1)); + + var second = cache.GetAvatarBytesAsync(md5).Result; + + Assert.That(second, Is.Not.Null); + Assert.That(second.Bytes, Is.EqualTo(new byte[] { 7 })); + // The second lookup must be served from the on-disk cache, so no additional fetch. + Assert.That(cache.FetchedUrls, Has.Count.EqualTo(1)); + } + + [Test] + public void GetAvatarBytes_FailedSource_ReturnsNull() + { + var cache = new TestableAvatarCache(_folder.Path); + cache.BytesToReturn = null; // simulate 404 / offline + var md5 = AvatarCache.Md5OfEmail("nobody@example.com"); + + var result = cache.GetAvatarBytesAsync(md5).Result; + + // null → the endpoint returns 404 → the front end shows generated initials. + Assert.That(result, Is.Null); + Assert.That(cache.FetchedUrls, Has.Count.EqualTo(1), "sanity: we did attempt a fetch"); + } + + [Test] + public void GetAvatarBytes_KnownPhotoUrlChanged_Refetches() + { + var cache = new TestableAvatarCache(_folder.Path); + cache.BytesToReturn = new byte[] { 1 }; + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + + cache.SetKnownPhotoUrl(md5, "https://photos.example/old.jpg"); + cache.GetAvatarBytesAsync(md5).Wait(); + Assert.That(cache.FetchedUrls.Last(), Is.EqualTo("https://photos.example/old.jpg")); + + // A new login supplies a different Google photo; the next lookup must refetch from it, + // not serve the stale cached bytes. + cache.SetKnownPhotoUrl(md5, "https://photos.example/new.jpg"); + cache.BytesToReturn = new byte[] { 2 }; + var result = cache.GetAvatarBytesAsync(md5).Result; + + Assert.That(result.Bytes, Is.EqualTo(new byte[] { 2 })); + Assert.That(cache.FetchedUrls.Last(), Is.EqualTo("https://photos.example/new.jpg")); + Assert.That(cache.FetchedUrls, Has.Count.EqualTo(2)); + } + + [Test] + public void KnownPhotoUrlMap_PersistsAcrossSaveRestore() + { + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + + var cache1 = new TestableAvatarCache(_folder.Path); + // Sanity: nothing persisted yet. + Assert.That(cache1.GetKnownPhotoUrlOrNull(md5), Is.Null); + cache1.SetKnownPhotoUrl(md5, "https://photos.example/pic.jpg"); + + // A fresh instance over the SAME folder simulates restarting Bloom; it must reload the map. + var cache2 = new TestableAvatarCache(_folder.Path); + Assert.That( + cache2.GetKnownPhotoUrlOrNull(md5), + Is.EqualTo("https://photos.example/pic.jpg") + ); + } + + [Test] + public void RemoveKnownPhotoUrl_ClearsMapping_AndRevertsToGravatar() + { + var cache = new TestableAvatarCache(_folder.Path); + cache.BytesToReturn = new byte[] { 1 }; + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + + cache.SetKnownPhotoUrl(md5, "https://photos.example/pic.jpg"); + // Sanity: populated before the "logout". + Assert.That( + cache.GetKnownPhotoUrlOrNull(md5), + Is.EqualTo("https://photos.example/pic.jpg") + ); + + cache.RemoveKnownPhotoUrl(md5); + + Assert.That(cache.GetKnownPhotoUrlOrNull(md5), Is.Null); + + // With the mapping gone the person now resolves via Gravatar. + cache.GetAvatarBytesAsync(md5).Wait(); + Assert.That(cache.FetchedUrls.Last(), Is.EqualTo(AvatarCache.GravatarUrl(md5))); + } + + [Test] + public void SetKnownPhotoUrl_WithEmptyOrNull_IsNoOpAndDoesNotThrow() + { + var cache = new TestableAvatarCache(_folder.Path); + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + + // Mirrors the login handler's rule for a login WITHOUT a photoUrl (older website build): + // registering an empty/null value must not create a mapping and must not throw. + Assert.DoesNotThrow(() => cache.SetKnownPhotoUrl(md5, null)); + Assert.DoesNotThrow(() => cache.SetKnownPhotoUrl(md5, "")); + Assert.That(cache.GetKnownPhotoUrlOrNull(md5), Is.Null); + } + + [Test] + public void GetAvatarBytes_RefreshesOncePerRun() + { + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + + // First run: two lookups, but only the first fetches; the second is served from cache. + var run1 = new TestableAvatarCache(_folder.Path); + run1.BytesToReturn = new byte[] { 1 }; + run1.GetAvatarBytesAsync(md5).Wait(); + run1.GetAvatarBytesAsync(md5).Wait(); + Assert.That( + run1.FetchedUrls, + Has.Count.EqualTo(1), + "within one run we should fetch only once, then serve from cache" + ); + + // A new run (fresh instance over the same folder) re-attempts a fresh fetch even though the + // bytes are already cached from the previous run -- this is how a changed avatar is picked up + // by restarting Bloom -- and uses the newly fetched bytes. + var run2 = new TestableAvatarCache(_folder.Path); + run2.BytesToReturn = new byte[] { 2 }; + var result = run2.GetAvatarBytesAsync(md5).Result; + Assert.That(run2.FetchedUrls, Has.Count.EqualTo(1), "a new run should fetch fresh"); + Assert.That(result.Bytes, Is.EqualTo(new byte[] { 2 }), "and use the fresh bytes"); + } + + [Test] + public void GetAvatarBytes_FetchFailsButHaveCachedCopy_ReturnsCachedCopy() + { + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + + // First run caches a successful fetch. + var run1 = new TestableAvatarCache(_folder.Path); + run1.BytesToReturn = new byte[] { 9 }; + var first = run1.GetAvatarBytesAsync(md5).Result; + Assert.That( + first?.Bytes, + Is.EqualTo(new byte[] { 9 }), + "sanity: the first run should have cached the avatar" + ); + + // Second run with the fetch failing (offline): we attempt a fresh fetch, it fails, and we + // fall back to the cached copy rather than showing nothing (initials). + var run2 = new TestableAvatarCache(_folder.Path); + run2.BytesToReturn = null; // simulate offline / fetch failure + var second = run2.GetAvatarBytesAsync(md5).Result; + + Assert.That( + run2.FetchedUrls, + Has.Count.EqualTo(1), + "sanity: we should have attempted a fresh fetch this run" + ); + Assert.That( + second, + Is.Not.Null, + "should fall back to the cached copy when the fetch fails" + ); + Assert.That(second.Bytes, Is.EqualTo(new byte[] { 9 })); + } + } +} From 695d4f66042bf5a962c9c710d442c160ae7edc3f Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 8 Jul 2026 13:06:41 -0600 Subject: [PATCH 2/5] Address review feedback: sandbox default in debug; AvatarCache singleton (BL-16514) - BookUpload.UseSandboxByDefault: revert the temporary DEBUG override so debug builds default to the sandbox again (flagged by all three reviewers as an accidental production-upload risk). - Register AvatarCache as SingleInstance instead of InstancePerLifetimeScope, so its shared in-memory known-photo map and disk cache can't be duplicated by a child-scope resolve. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomExe/ProjectContext.cs | 7 ++++++- src/BloomExe/WebLibraryIntegration/BookUpload.cs | 7 +------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/BloomExe/ProjectContext.cs b/src/BloomExe/ProjectContext.cs index 577c3757a908..614ae9d11de3 100644 --- a/src/BloomExe/ProjectContext.cs +++ b/src/BloomExe/ProjectContext.cs @@ -194,7 +194,6 @@ IContainer parentContainer typeof(LibraryPublishApi), typeof(AccountApi), typeof(AvatarApi), - typeof(AvatarCache), typeof(WorkspaceApi), typeof(BookCollectionHolder), typeof(WorkspaceTabSelection), @@ -327,6 +326,12 @@ IContainer parentContainer builder.RegisterType().AsSelf().SingleInstance(); + // The avatar cache holds an in-memory known-photo map and disk cache shared across + // the app (account menu + Team Collection avatars); it must be a single shared + // instance, not one-per-lifetime-scope, so a child-scope resolve can't get a second + // copy with a stale map. + builder.RegisterType().AsSelf().SingleInstance(); + // Enhance: may need some way to test a release build in the sandbox. builder.Register(c => CreateBloomS3Client()).AsSelf().SingleInstance(); builder.RegisterType().AsSelf().SingleInstance(); diff --git a/src/BloomExe/WebLibraryIntegration/BookUpload.cs b/src/BloomExe/WebLibraryIntegration/BookUpload.cs index 71883345769f..3068e64a4c97 100644 --- a/src/BloomExe/WebLibraryIntegration/BookUpload.cs +++ b/src/BloomExe/WebLibraryIntegration/BookUpload.cs @@ -70,12 +70,7 @@ internal static bool UseSandboxByDefault get { #if DEBUG - // TEMPORARY: the dev/sandbox backend (dev-server.bloomlibrary.org) is returning - // HTTP 500 for all requests, which breaks login and library queries. Point debug - // builds at production (bloomlibrary.org) until the dev backend is healthy again. - // Revert this to `return true;` once the sandbox is back so debug uploads don't - // hit production. - return false; + return true; #else var temp = Environment.GetEnvironmentVariable("BloomSandbox"); if (string.IsNullOrWhiteSpace(temp)) From 2e9c611ff7ab9e87c0a01a78740f2c7eafd4c67a Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 8 Jul 2026 16:28:34 -0600 Subject: [PATCH 3/5] Fix avatar cache folder DI bug; accept photoURL casing on login (BL-16514) Autofac was injecting the registered editableCollectionDirectory string into AvatarCache's optional `cacheFolder` constructor parameter, so the avatar cache wrote its files into the user's open collection folder instead of the app-data AvatarCache folder -- polluting the collection and tying the persisted known-photo map to whichever collection happened to be open. Register AvatarCache via a `new AvatarCache()` factory so the parameterless (app-data) path is used. Also accept Firebase's native `photoURL` casing as well as `photoUrl` in the external/login POST, since the website (a separate repo that deploys independently) may forward `user.photoURL` verbatim. Adds a photoURL-casing test and a DI-registration regression test (the latter with a sanity assertion proving the Autofac optional-parameter gotcha is real). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomExe/ProjectContext.cs | 11 ++++- src/BloomExe/web/AvatarCache.cs | 7 +++ src/BloomExe/web/controllers/ExternalApi.cs | 11 ++++- src/BloomTests/web/AvatarApiTests.cs | 29 +++++++++++++ src/BloomTests/web/AvatarCacheTests.cs | 47 +++++++++++++++++++++ 5 files changed, 102 insertions(+), 3 deletions(-) diff --git a/src/BloomExe/ProjectContext.cs b/src/BloomExe/ProjectContext.cs index 614ae9d11de3..7a681fcbc377 100644 --- a/src/BloomExe/ProjectContext.cs +++ b/src/BloomExe/ProjectContext.cs @@ -330,7 +330,16 @@ IContainer parentContainer // the app (account menu + Team Collection avatars); it must be a single shared // instance, not one-per-lifetime-scope, so a child-scope resolve can't get a second // copy with a stale map. - builder.RegisterType().AsSelf().SingleInstance(); + // + // Register with an explicit factory rather than RegisterType(). Its + // constructor has an OPTIONAL `string cacheFolder = null` parameter (a test seam), + // and a plain string IS registered in this container (editableCollectionDirectory, + // just below). Autofac injects a registered service into an optional parameter when + // one exists, so RegisterType would hand AvatarCache the collection directory as its + // cache folder -- scattering avatar cache files into the user's collection folder and + // defeating the "app-wide, survives restart" design. `new AvatarCache()` forces the + // parameterless path so it uses the intended app-data folder. + builder.Register(c => new AvatarCache()).AsSelf().SingleInstance(); // Enhance: may need some way to test a release build in the sandbox. builder.Register(c => CreateBloomS3Client()).AsSelf().SingleInstance(); diff --git a/src/BloomExe/web/AvatarCache.cs b/src/BloomExe/web/AvatarCache.cs index 2c8733ec6db1..708ffa7f29c7 100644 --- a/src/BloomExe/web/AvatarCache.cs +++ b/src/BloomExe/web/AvatarCache.cs @@ -100,6 +100,13 @@ public AvatarCache(string cacheFolder = null) Load(); } + /// + /// The folder this cache persists into. Exposed for the DI regression test that verifies the + /// container gives the production (parameterless) instance the app-data folder, rather than + /// accidentally injecting some other registered string as the cacheFolder argument. + /// + internal string CacheFolder => _cacheFolder; + /// /// The MD5 (lowercase hex) of the normalized (trimmed, lowercased) email. This matches the /// Gravatar scheme (and what react-avatar/md5Util use), so the logged-in user and a teammate diff --git a/src/BloomExe/web/controllers/ExternalApi.cs b/src/BloomExe/web/controllers/ExternalApi.cs index f969c606019e..da3c2fb8c559 100644 --- a/src/BloomExe/web/controllers/ExternalApi.cs +++ b/src/BloomExe/web/controllers/ExternalApi.cs @@ -123,8 +123,15 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) // build will not send this field at all. A missing/null value is normal and // simply means "no picture" (the avatar falls back to Gravatar/initials). Only // THIS field gets this lenient treatment; the others keep the fail-fast behavior. - string photoUrl = requestData.IsDefined("photoUrl") - ? (string)requestData.photoUrl + // + // Accept either casing. Firebase's own user property is `photoURL` (capital URL) + // while our other fields are camelCase, so a website build that forwards + // `user.photoURL` verbatim is an easy and likely mismatch. Tolerating both spellings + // (checking IsDefined, which is case-sensitive, for each) means the nicer avatar + // lights up regardless of which the website happens to send. + string photoUrl = + requestData.IsDefined("photoUrl") ? (string)requestData.photoUrl + : requestData.IsDefined("photoURL") ? (string)requestData.photoURL : null; //Debug.WriteLine("Got login data " + email + " with token " + token + " and id " + userId); diff --git a/src/BloomTests/web/AvatarApiTests.cs b/src/BloomTests/web/AvatarApiTests.cs index 39bd3a9f20fa..15e0d0827994 100644 --- a/src/BloomTests/web/AvatarApiTests.cs +++ b/src/BloomTests/web/AvatarApiTests.cs @@ -176,5 +176,34 @@ public void ExternalLogin_WithoutPhotoUrl_DoesNotRegisterAndDoesNotThrow() Assert.That(result, Is.EqualTo("OK")); Assert.That(_cache.GetKnownPhotoUrlOrNull(md5), Is.Null); } + + [Test] + public void ExternalLogin_WithPhotoUrlCapitalUrl_RegistersKnownPhotoMapping() + { + var client = new BloomLibraryBookApiClient(); + new ExternalApi(client, null, null, null, null, null, _cache).RegisterWithApiHandler( + _server.ApiHandler + ); + + var md5 = AvatarCache.Md5OfEmail("user@example.com"); + // Sanity: no mapping before login. + Assert.That(_cache.GetKnownPhotoUrlOrNull(md5), Is.Null); + + // The website forwards Firebase's own property name, `photoURL` (capital URL), rather than + // our camelCase `photoUrl`. The login handler must tolerate that casing and still register + // the mapping, otherwise the Google avatar silently never appears. + var result = ApiTest.PostString( + _server, + "external/login", + "{\"sessionToken\":\"t\",\"email\":\"user@example.com\",\"userId\":\"u\",\"photoURL\":\"https://photos.example/pic.jpg\"}", + ApiTest.ContentType.JSON + ); + + Assert.That(result, Is.EqualTo("OK")); + Assert.That( + _cache.GetKnownPhotoUrlOrNull(md5), + Is.EqualTo("https://photos.example/pic.jpg") + ); + } } } diff --git a/src/BloomTests/web/AvatarCacheTests.cs b/src/BloomTests/web/AvatarCacheTests.cs index 6e00a85e7711..5ff892dd1498 100644 --- a/src/BloomTests/web/AvatarCacheTests.cs +++ b/src/BloomTests/web/AvatarCacheTests.cs @@ -1,6 +1,8 @@ using System.Collections.Generic; +using System.IO; using System.Linq; using System.Threading.Tasks; +using Autofac; using Bloom.web; using NUnit.Framework; using SIL.TestUtilities; @@ -283,5 +285,50 @@ public void GetAvatarBytes_FetchFailsButHaveCachedCopy_ReturnsCachedCopy() ); Assert.That(second.Bytes, Is.EqualTo(new byte[] { 9 })); } + + [Test] + public void ProductionRegistration_UsesAppDataFolder_NotAnUnrelatedRegisteredString() + { + // Reproduces ProjectContext's container shape: a plain string is registered (there it is + // the editable collection directory). AvatarCache's constructor has an OPTIONAL + // `string cacheFolder = null` parameter, and Autofac injects a registered service into an + // optional parameter when one exists. This guards the ProjectContext fix (register via a + // `new AvatarCache()` factory, NOT RegisterType) against being "cleaned up" back into the + // bug, which would scatter the avatar cache into the user's collection folder. + var unrelatedDir = _folder.Path; // stands in for editableCollectionDirectory + + // Sanity check that the gotcha is real: RegisterType DOES adopt the registered string as + // the cache folder. If this ever stops happening, the guard below would pass vacuously. + var buggyBuilder = new ContainerBuilder(); + buggyBuilder.Register(c => unrelatedDir).InstancePerLifetimeScope(); + buggyBuilder.RegisterType().AsSelf().SingleInstance(); + using (var buggy = buggyBuilder.Build()) + { + Assert.That( + buggy.Resolve().CacheFolder, + Is.EqualTo(unrelatedDir), + "sanity: RegisterType is expected to (mis)inject the registered string here" + ); + } + + // The production registration must NOT adopt that string; it uses the app-data folder. + var fixedBuilder = new ContainerBuilder(); + fixedBuilder.Register(c => unrelatedDir).InstancePerLifetimeScope(); + fixedBuilder.Register(c => new AvatarCache()).AsSelf().SingleInstance(); + using (var fixedContainer = fixedBuilder.Build()) + { + var folder = fixedContainer.Resolve().CacheFolder; + Assert.That( + folder, + Is.Not.EqualTo(unrelatedDir), + "AvatarCache must not adopt an unrelated registered string as its cache folder" + ); + Assert.That( + folder, + Does.EndWith(Path.Combine("Bloom", "AvatarCache")), + "AvatarCache should default to the app-data AvatarCache folder" + ); + } + } } } From 983e84a2b4a60bfd7e9cbaa53cf7e67fac9151ce Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 8 Jul 2026 16:53:41 -0600 Subject: [PATCH 4/5] Dispose HttpResponseMessage in AvatarCache.FetchAsync (BL-16514) Devin flagged that the HttpResponseMessage returned by s_httpClient.GetAsync was never disposed on any path, leaking its pooled network connection. Since we fetch one avatar per person, a Team Collection with many members could eventually starve the shared HttpClient's connection pool. Wrap it in a using declaration. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomExe/web/AvatarCache.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/BloomExe/web/AvatarCache.cs b/src/BloomExe/web/AvatarCache.cs index 708ffa7f29c7..b2d7e340d72b 100644 --- a/src/BloomExe/web/AvatarCache.cs +++ b/src/BloomExe/web/AvatarCache.cs @@ -307,7 +307,10 @@ private AvatarResult ReadCachedOrNull(string md5) /// protected virtual async Task FetchAsync(string url) { - var response = await s_httpClient.GetAsync(url); + // Dispose the response on every path: HttpResponseMessage owns the response stream and its + // pooled network connection, and we fetch one per person (a Team Collection can have many), + // so leaking these would eventually starve the shared HttpClient's connection pool. + using var response = await s_httpClient.GetAsync(url); if (!response.IsSuccessStatusCode) return null; var bytes = await response.Content.ReadAsByteArrayAsync(); From cea64b117e2a1d44c4bf158b19662c8703ceb0c1 Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 9 Jul 2026 10:31:23 -0600 Subject: [PATCH 5/5] Avoid nested ternary; document the preference (BL-16514) Replace the stacked ternary for photoUrl/photoURL casing with an if/else-if chain per review feedback, and add the no-nested-ternary rule to AGENTS.md so it's documented as a codebase convention. --- AGENTS.md | 2 ++ src/BloomExe/web/controllers/ExternalApi.cs | 9 +++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4bca0eb2e4bf..940f177144a6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,8 @@ The front-end uses yarn 1.22.22. Never ever use npm. - Where possible style things using @emotion/react rather than using sx objects. +- Avoid stacking/nesting ternary (`? :`) operators (e.g. `a ? x : b ? y : z`). They're too hard for humans to read. Use an if/else-if chain (or a switch) instead. A single, non-nested ternary is fine. + - For Typescript coding style, see ./src/BloomBrowserUI/AGENTS.md # Testing diff --git a/src/BloomExe/web/controllers/ExternalApi.cs b/src/BloomExe/web/controllers/ExternalApi.cs index da3c2fb8c559..ea3565de4c29 100644 --- a/src/BloomExe/web/controllers/ExternalApi.cs +++ b/src/BloomExe/web/controllers/ExternalApi.cs @@ -129,10 +129,11 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) // `user.photoURL` verbatim is an easy and likely mismatch. Tolerating both spellings // (checking IsDefined, which is case-sensitive, for each) means the nicer avatar // lights up regardless of which the website happens to send. - string photoUrl = - requestData.IsDefined("photoUrl") ? (string)requestData.photoUrl - : requestData.IsDefined("photoURL") ? (string)requestData.photoURL - : null; + string photoUrl = null; + if (requestData.IsDefined("photoUrl")) + photoUrl = (string)requestData.photoUrl; + else if (requestData.IsDefined("photoURL")) + photoUrl = (string)requestData.photoURL; //Debug.WriteLine("Got login data " + email + " with token " + token + " and id " + userId); // Register the logged-in user's profile picture (if any) in the person-keyed