From 8911681d391832628791f408fd3772d63a365652 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Thu, 12 Sep 2024 10:23:47 -0400 Subject: [PATCH 1/4] use local storage for the dev root id - needed to relax the security rules for the dev root - update user icon roll over so devs know the root - update firebase and firestore to share the same logic for computing the root id based on the appMode --- firestore.rules | 7 ++-- src/clue/components/clue-app-header.tsx | 18 +++++++-- src/lib/firebase.test.ts | 54 +++++++++++++++++++++++++ src/lib/firebase.ts | 13 ++---- src/lib/firestore.test.ts | 34 ++++++++++++++++ src/lib/firestore.ts | 22 ++-------- src/lib/root-id.ts | 46 +++++++++++++++++++++ 7 files changed, 157 insertions(+), 37 deletions(-) create mode 100644 src/lib/firebase.test.ts create mode 100644 src/lib/root-id.ts diff --git a/firestore.rules b/firestore.rules index 7f01a54011..72a79b3526 100644 --- a/firestore.rules +++ b/firestore.rules @@ -426,10 +426,9 @@ service cloud.firestore { allow read, write: if isAuthed(); } - match /dev/{userId}/{restOfPath=**} { - allow read: if isAuthed(); - // users can only write to their own folders - allow write: if matchFirebaseUserId(userId); + // Developers get a random unique key that their data is stored under + match /dev/{devId}/{restOfPath=**} { + allow read, write: if isAuthed(); } match /qa/{userId}/{restOfPath=**} { diff --git a/src/clue/components/clue-app-header.tsx b/src/clue/components/clue-app-header.tsx index 33dc5dc88e..56a09e76ab 100644 --- a/src/clue/components/clue-app-header.tsx +++ b/src/clue/components/clue-app-header.tsx @@ -9,6 +9,7 @@ import { ToggleGroup } from "@concord-consortium/react-components"; import { GroupModelType, GroupUserModelType } from "../../models/stores/groups"; import { CustomSelect } from "./custom-select"; import { useStores } from "../../hooks/use-stores"; +import { getDevId } from "../../lib/root-id"; // cf. https://mattferderer.com/use-sass-variables-in-typescript-and-javascript import styles from "./toggle-buttons.scss"; @@ -26,8 +27,17 @@ export const ClueAppHeaderComponent: React.FC = observer(function ClueAp const { showGroup } = props; const { appConfig, appMode, appVersion, db, user, problem, groups, investigation, ui, unit } = useStores(); const myGroup = showGroup ? groups.getGroupById(user.currentGroupId) : undefined; - const userTitle = appMode !== "authed" && appMode !== "demo" - ? `Firebase UID: ${db.firebase.userId}` : undefined; + const getUserTitle = () => { + switch(appMode){ + case "dev": + return `Firebase Root: ${getDevId()}`; + case "qa": + case "test": + return `Firebase UID: ${db.firebase.userId}`; + default: + return undefined; + } + }; const renderPanelButtons = () => { const { panels, onPanelChange, current} = props; @@ -150,7 +160,7 @@ export const ClueAppHeaderComponent: React.FC = observer(function ClueAp
Version {appVersion}
-
+
@@ -191,7 +201,7 @@ export const ClueAppHeaderComponent: React.FC = observer(function ClueAp
Version {appVersion}
{myGroup ? renderGroup(myGroup) : null} -
+
{user.name}
{user.className}
diff --git a/src/lib/firebase.test.ts b/src/lib/firebase.test.ts new file mode 100644 index 0000000000..f1d31a6f0a --- /dev/null +++ b/src/lib/firebase.test.ts @@ -0,0 +1,54 @@ +import { DB } from "./db"; +import { Firebase } from "./firebase"; +import { kClueDevIDKey } from "./root-id"; + +const mockStores = { + appMode: "authed", + demo: { name: "demo" }, + user: { portal: "test-portal" } +}; +const mockDB = { + stores: mockStores +} as DB; + +describe("Firebase class", () => { + describe("initialization", () => { + it("should create a valid Firebase object", () => { + const firebase = new Firebase(mockDB); + expect(firebase).toBeDefined(); + }); + }); + describe("getRootFolder", () => { + it("should handle authed mode", () => { + const firebase = new Firebase(mockDB); + expect(firebase.getRootFolder()).toBe("/authed/test-portal/portals/test-portal/"); + }); + it("should handle the dev appMode", () => { + window.localStorage.setItem(kClueDevIDKey, "random-id"); + const stores = {...mockStores, appMode: "dev"}; + const firestore = new Firebase({stores} as DB); + expect(firestore.getRootFolder()).toBe("/dev/random-id/portals/test-portal/"); + }); + describe("should handle the demo appMode", () => { + it("handles basic demo name", () => { + const stores = {...mockStores, + appMode: "demo", demo: { name: "test-demo" }}; + const firestore = new Firebase({stores} as DB); + expect(firestore.getRootFolder()).toBe("/demo/test-demo/portals/test-portal/"); + }); + it("handles empty demo name", () => { + const stores = {...mockStores, + appMode: "demo", demo: { name: "" }}; + const firestore = new Firebase({stores} as DB); + expect(firestore.getRootFolder()).toBe("/demo/test-portal/portals/test-portal/"); + }); + it("handles empty demo name and empty portal", () => { + const stores = {...mockStores, + appMode: "demo", demo: { name: "" }, user: { portal: ""}}; + const firestore = new Firebase({stores} as DB); + // FIXME + expect(firestore.getRootFolder()).toBe("/demo/demo/portals//"); + }); + }); + }); +}); diff --git a/src/lib/firebase.ts b/src/lib/firebase.ts index ef2f77393c..24e784b841 100644 --- a/src/lib/firebase.ts +++ b/src/lib/firebase.ts @@ -8,6 +8,7 @@ import { DB } from "./db"; import { escapeKey } from "./fire-utils"; import { urlParams } from "../utilities/url-params"; import { DocumentModelType } from "src/models/document/document"; +import { getRootId } from "./root-id"; // Set this during database testing in combination with the urlParam testMigration=true to // override the top-level Firebase key regardless of mode. For example, setting this to "authed-copy" @@ -65,22 +66,14 @@ export class Firebase { // dev: /dev//portals/localhost // qa: /qa//portals/qa // test: /test//portals/ - const { appMode, demo: { name: demoName }, user } = this.db.stores; + const { appMode, user } = this.db.stores; const parts = []; if (urlParams.testMigration === "true" && FIREBASE_ROOT_OVERRIDE) { parts.push(FIREBASE_ROOT_OVERRIDE); } else { parts.push(`${appMode}`); - if ((appMode === "dev") || (appMode === "test") || (appMode === "qa")) { - parts.push(this.userId); - } - else if (appMode === "demo") { - const slug = demoName && demoName.length > 0 ? escapeKey(demoName) : ""; - if (slug.length > 0) { - parts.push(slug); - } - } + parts.push(getRootId(this.db.stores, this.userId)); } parts.push("portals"); parts.push(escapeKey(user.portal)); diff --git a/src/lib/firestore.test.ts b/src/lib/firestore.test.ts index 2233c49331..78fe1f6c28 100644 --- a/src/lib/firestore.test.ts +++ b/src/lib/firestore.test.ts @@ -1,5 +1,6 @@ import { DB } from "./db"; import { Firestore } from "./firestore"; +import { kClueDevIDKey } from "./root-id"; const mockStores = { appMode: "authed", @@ -56,8 +57,41 @@ describe("Firestore class", () => { it("should create a valid Firestore object", () => { const firestore = new Firestore(mockDB); expect(firestore).toBeDefined(); + }); + }); + + describe("getRootFolder", () => { + beforeEach(() => resetMocks()); + it("should handle the authed appMode", () => { + const firestore = new Firestore(mockDB); expect(firestore.getRootFolder()).toBe("/authed/test-portal/"); }); + it("should handle the dev appMode", () => { + window.localStorage.setItem(kClueDevIDKey, "random-id"); + const stores = {...mockStores, appMode: "dev"}; + const firestore = new Firestore({stores} as DB); + expect(firestore.getRootFolder()).toBe("/dev/random-id/"); + }); + describe("should handle the demo appMode", () => { + it("handles basic demo name", () => { + const stores = {...mockStores, + appMode: "demo", demo: { name: "test-demo" }}; + const firestore = new Firestore({stores} as DB); + expect(firestore.getRootFolder()).toBe("/demo/test-demo/"); + }); + it("handles empty demo name", () => { + const stores = {...mockStores, + appMode: "demo", demo: { name: "" }}; + const firestore = new Firestore({stores} as DB); + expect(firestore.getRootFolder()).toBe("/demo/test-portal/"); + }); + it("handles empty demo name and empty portal", () => { + const stores = {...mockStores, + appMode: "demo", demo: { name: "" }, user: { portal: ""}}; + const firestore = new Firestore({stores} as DB); + expect(firestore.getRootFolder()).toBe("/demo/demo/"); + }); + }); }); describe("setFirebaseUser", () => { diff --git a/src/lib/firestore.ts b/src/lib/firestore.ts index cb867bbd8c..4575cd9611 100644 --- a/src/lib/firestore.ts +++ b/src/lib/firestore.ts @@ -1,6 +1,7 @@ import firebase from "firebase/app"; import "firebase/firestore"; import { DB } from "./db"; +import { getRootId } from "./root-id"; import { escapeKey } from "./fire-utils"; import { UserDocument } from "./firestore-schema"; @@ -40,25 +41,8 @@ export class Firestore { // public getRootFolder() { - const { appMode, demo: { name: demoName }, user: { portal } } = this.db.stores; - let rootDocId: string; - const escapedPortal = portal ? escapeKey(portal) : portal; - - // `authed/${escapedPortalDomain}` - if (appMode === "authed") { - rootDocId = escapedPortal; - } - // `demo/${escapedDemoName}` - else if ((appMode === "demo") && (demoName?.length > 0)) { - const escapedDemoName = demoName ? escapeKey(demoName) : demoName; - rootDocId = escapedDemoName || escapedPortal || "demo"; - } - // `${appMode}/${userId}` - else { // (appMode === "dev") || (appMode === "test") || (appMode === "qa") - rootDocId = this.userId; - } - - return `/${appMode}/${rootDocId}/`; + const { appMode } = this.db.stores; + return `/${appMode}/${getRootId(this.db.stores, this.userId)}/`; } public getFullPath(path = "") { diff --git a/src/lib/root-id.ts b/src/lib/root-id.ts new file mode 100644 index 0000000000..c75e476aca --- /dev/null +++ b/src/lib/root-id.ts @@ -0,0 +1,46 @@ +import { nanoid } from "nanoid"; +import { IStores } from "../models/stores/stores"; +import { escapeKey } from "./fire-utils"; + +export const kClueDevIDKey = "clue-dev-id"; + +export function getDevId() { + let devId = window.localStorage.getItem(kClueDevIDKey); + if (!devId) { + const newDevId = nanoid(); + window.localStorage.setItem(kClueDevIDKey, newDevId); + devId = newDevId; + } + return devId; +} + +type IRootDocIdStores = Pick; + +export function getRootId(stores: IRootDocIdStores, firebaseUserId: string) { + const { appMode, demo: { name: demoName }, user: { portal } } = stores; + const escapedPortal = portal ? escapeKey(portal) : portal; + + switch (appMode) { + case "authed": { + return escapedPortal; + } + case "demo": { + // Legacy Note: Previously if the demoName was "", the root id in the realtime database + // and Firestore would be different. The paths would end up being: + // database: /demo/portals/demo/ (root id is skipped) + // firestore: /demo/{firebaseUserId}/ + // Now the root id will be the default "demo" in this case so the paths will be: + // database: /demo/demo/portals/demo/ + // firestore: /demo/demo/ + const escapedDemoName = demoName ? escapeKey(demoName) : demoName; + return escapedDemoName || escapedPortal || "demo"; + } + case "dev": { + return getDevId(); + } + // "test" and "qa" + default: { + return firebaseUserId; + } + } +} From e02c6c80655c6c1b4a7366393057bd1e6138db64 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Thu, 12 Sep 2024 12:10:40 -0400 Subject: [PATCH 2/4] firebase authed root just has the portal --- src/lib/firebase.test.ts | 2 +- src/lib/firebase.ts | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/lib/firebase.test.ts b/src/lib/firebase.test.ts index f1d31a6f0a..9db8b9f986 100644 --- a/src/lib/firebase.test.ts +++ b/src/lib/firebase.test.ts @@ -21,7 +21,7 @@ describe("Firebase class", () => { describe("getRootFolder", () => { it("should handle authed mode", () => { const firebase = new Firebase(mockDB); - expect(firebase.getRootFolder()).toBe("/authed/test-portal/portals/test-portal/"); + expect(firebase.getRootFolder()).toBe("/authed/portals/test-portal/"); }); it("should handle the dev appMode", () => { window.localStorage.setItem(kClueDevIDKey, "random-id"); diff --git a/src/lib/firebase.ts b/src/lib/firebase.ts index 24e784b841..378c81d4ae 100644 --- a/src/lib/firebase.ts +++ b/src/lib/firebase.ts @@ -73,7 +73,9 @@ export class Firebase { parts.push(FIREBASE_ROOT_OVERRIDE); } else { parts.push(`${appMode}`); - parts.push(getRootId(this.db.stores, this.userId)); + if (appMode !== "authed") { + parts.push(getRootId(this.db.stores, this.userId)); + } } parts.push("portals"); parts.push(escapeKey(user.portal)); From 62d01664780772f7eb2b9774792dfd1aa1aad077 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Tue, 17 Sep 2024 09:32:15 -0400 Subject: [PATCH 3/4] remove the local storage dev root id This dev root approach requires the firebase functions to be updated and there isn't time to do that, now. The refactoring is kept so applying the change will be easier in the future. --- firestore.rules | 3 ++- src/clue/components/clue-app-header.tsx | 2 -- src/lib/db.ts | 3 ++- src/lib/firebase.test.ts | 20 ++++++-------------- src/lib/firestore.test.ts | 7 ------- src/lib/root-id.ts | 18 +----------------- src/models/stores/user-context-provider.ts | 4 ++++ 7 files changed, 15 insertions(+), 42 deletions(-) diff --git a/firestore.rules b/firestore.rules index 72a79b3526..593a59c01d 100644 --- a/firestore.rules +++ b/firestore.rules @@ -426,7 +426,8 @@ service cloud.firestore { allow read, write: if isAuthed(); } - // Developers get a random unique key that their data is stored under + // In the future, developers might use a random unique key instead of + // the firebase user id. So this rule is relaxed inorder to allow that. match /dev/{devId}/{restOfPath=**} { allow read, write: if isAuthed(); } diff --git a/src/clue/components/clue-app-header.tsx b/src/clue/components/clue-app-header.tsx index 56a09e76ab..ef39da545c 100644 --- a/src/clue/components/clue-app-header.tsx +++ b/src/clue/components/clue-app-header.tsx @@ -9,7 +9,6 @@ import { ToggleGroup } from "@concord-consortium/react-components"; import { GroupModelType, GroupUserModelType } from "../../models/stores/groups"; import { CustomSelect } from "./custom-select"; import { useStores } from "../../hooks/use-stores"; -import { getDevId } from "../../lib/root-id"; // cf. https://mattferderer.com/use-sass-variables-in-typescript-and-javascript import styles from "./toggle-buttons.scss"; @@ -30,7 +29,6 @@ export const ClueAppHeaderComponent: React.FC = observer(function ClueAp const getUserTitle = () => { switch(appMode){ case "dev": - return `Firebase Root: ${getDevId()}`; case "qa": case "test": return `Firebase UID: ${db.firebase.userId}`; diff --git a/src/lib/db.ts b/src/lib/db.ts index 826f9f5960..f24ff93fb9 100644 --- a/src/lib/db.ts +++ b/src/lib/db.ts @@ -40,6 +40,7 @@ import { urlParams } from "../utilities/url-params"; import { firebaseConfig } from "./firebase-config"; import { UserModelType } from "../models/stores/user"; import { logExemplarDocumentEvent } from "../models/document/log-exemplar-document-event"; +import { AppMode } from "../models/stores/store-types"; import { DEBUG_FIRESTORE } from "./debug"; export type IDBConnectOptions = IDBAuthConnectOptions | IDBNonAuthConnectOptions; @@ -55,7 +56,7 @@ export interface IDBAuthConnectOptions extends IDBBaseConnectOptions { rawFirebaseJWT: string; } export interface IDBNonAuthConnectOptions extends IDBBaseConnectOptions { - appMode: "dev" | "test" | "demo" | "qa"; + appMode: Exclude; } export interface UserGroupMap { [key: string]: { diff --git a/src/lib/firebase.test.ts b/src/lib/firebase.test.ts index 9db8b9f986..b8732e95e2 100644 --- a/src/lib/firebase.test.ts +++ b/src/lib/firebase.test.ts @@ -1,6 +1,5 @@ import { DB } from "./db"; import { Firebase } from "./firebase"; -import { kClueDevIDKey } from "./root-id"; const mockStores = { appMode: "authed", @@ -23,31 +22,24 @@ describe("Firebase class", () => { const firebase = new Firebase(mockDB); expect(firebase.getRootFolder()).toBe("/authed/portals/test-portal/"); }); - it("should handle the dev appMode", () => { - window.localStorage.setItem(kClueDevIDKey, "random-id"); - const stores = {...mockStores, appMode: "dev"}; - const firestore = new Firebase({stores} as DB); - expect(firestore.getRootFolder()).toBe("/dev/random-id/portals/test-portal/"); - }); describe("should handle the demo appMode", () => { it("handles basic demo name", () => { const stores = {...mockStores, appMode: "demo", demo: { name: "test-demo" }}; - const firestore = new Firebase({stores} as DB); - expect(firestore.getRootFolder()).toBe("/demo/test-demo/portals/test-portal/"); + const firebase = new Firebase({stores} as DB); + expect(firebase.getRootFolder()).toBe("/demo/test-demo/portals/test-portal/"); }); it("handles empty demo name", () => { const stores = {...mockStores, appMode: "demo", demo: { name: "" }}; - const firestore = new Firebase({stores} as DB); - expect(firestore.getRootFolder()).toBe("/demo/test-portal/portals/test-portal/"); + const firebase = new Firebase({stores} as DB); + expect(firebase.getRootFolder()).toBe("/demo/test-portal/portals/test-portal/"); }); it("handles empty demo name and empty portal", () => { const stores = {...mockStores, appMode: "demo", demo: { name: "" }, user: { portal: ""}}; - const firestore = new Firebase({stores} as DB); - // FIXME - expect(firestore.getRootFolder()).toBe("/demo/demo/portals//"); + const firebase = new Firebase({stores} as DB); + expect(firebase.getRootFolder()).toBe("/demo/demo/portals//"); }); }); }); diff --git a/src/lib/firestore.test.ts b/src/lib/firestore.test.ts index 78fe1f6c28..956c689caa 100644 --- a/src/lib/firestore.test.ts +++ b/src/lib/firestore.test.ts @@ -1,6 +1,5 @@ import { DB } from "./db"; import { Firestore } from "./firestore"; -import { kClueDevIDKey } from "./root-id"; const mockStores = { appMode: "authed", @@ -66,12 +65,6 @@ describe("Firestore class", () => { const firestore = new Firestore(mockDB); expect(firestore.getRootFolder()).toBe("/authed/test-portal/"); }); - it("should handle the dev appMode", () => { - window.localStorage.setItem(kClueDevIDKey, "random-id"); - const stores = {...mockStores, appMode: "dev"}; - const firestore = new Firestore({stores} as DB); - expect(firestore.getRootFolder()).toBe("/dev/random-id/"); - }); describe("should handle the demo appMode", () => { it("handles basic demo name", () => { const stores = {...mockStores, diff --git a/src/lib/root-id.ts b/src/lib/root-id.ts index c75e476aca..f2d14f1a1f 100644 --- a/src/lib/root-id.ts +++ b/src/lib/root-id.ts @@ -1,19 +1,6 @@ -import { nanoid } from "nanoid"; import { IStores } from "../models/stores/stores"; import { escapeKey } from "./fire-utils"; -export const kClueDevIDKey = "clue-dev-id"; - -export function getDevId() { - let devId = window.localStorage.getItem(kClueDevIDKey); - if (!devId) { - const newDevId = nanoid(); - window.localStorage.setItem(kClueDevIDKey, newDevId); - devId = newDevId; - } - return devId; -} - type IRootDocIdStores = Pick; export function getRootId(stores: IRootDocIdStores, firebaseUserId: string) { @@ -35,10 +22,7 @@ export function getRootId(stores: IRootDocIdStores, firebaseUserId: string) { const escapedDemoName = demoName ? escapeKey(demoName) : demoName; return escapedDemoName || escapedPortal || "demo"; } - case "dev": { - return getDevId(); - } - // "test" and "qa" + // "dev", "qa", and "test" default: { return firebaseUserId; } diff --git a/src/models/stores/user-context-provider.ts b/src/models/stores/user-context-provider.ts index 72f5a157b5..e81d9e0299 100644 --- a/src/models/stores/user-context-provider.ts +++ b/src/models/stores/user-context-provider.ts @@ -11,6 +11,10 @@ export class UserContextProvider { this.stores = stores; } + /** + * This user context is sent to the Firebase functions so they know the context of the + * request. + */ get userContext() { const appMode = this.stores.appMode; const { name: demoName } = this.stores.demo; From 2722e422e6b9b4fc0fded0b2e5b51b1e6b11db94 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Thu, 19 Sep 2024 10:04:17 -0400 Subject: [PATCH 4/4] Update firestore.rules Co-authored-by: Kirk Swenson --- firestore.rules | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firestore.rules b/firestore.rules index 593a59c01d..da47f6fbc7 100644 --- a/firestore.rules +++ b/firestore.rules @@ -427,7 +427,7 @@ service cloud.firestore { } // In the future, developers might use a random unique key instead of - // the firebase user id. So this rule is relaxed inorder to allow that. + // the firebase user id. So this rule is relaxed in order to allow that. match /dev/{devId}/{restOfPath=**} { allow read, write: if isAuthed(); }