Skip to content

Commit

Permalink
queryAuthStatus fixes:
Browse files Browse the repository at this point in the history 
* Break up 'checkUser' util into smaller chunks (for fine-grained control
* Rename 'checkUser' to 'checkIsAdmin'
* Abbreviate 'queryAuthStatus'
* Remove duplicate admnin status check
* Handle the function in a safer (error-wise) way
* Add test for the regression
  • Loading branch information
@ikusteu
ikusteu committed Jan 27, 2024
1 parent 6236829 commit 21099cc
Show file tree
Hide file tree
Showing 9 changed files with 96 additions and 102 deletions.
11 changes: 11 additions & 0 deletions packages/client/src/__tests__/auth.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,5 +93,16 @@ describe("Test authentication", () => {
}
}
);

testWithEmulator(
"should not explode if user is not logged in",
async () => {
const { data } = await httpsCallable(
functions,
CloudFunction.QueryAuthStatus
)({ organization: "idk" });
expect(data).toEqual({ isAdmin: false, secretKeys: [] });
}
);
});
});
58 changes: 25 additions & 33 deletions packages/functions/src/auth.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import admin from "firebase-admin";
import {
AuthStatus,
Collection,
OrganizationData,
OrgSubCollection,
QueryAuthStatusPayload,
wrapIter,
Expand All @@ -13,7 +12,12 @@ import {
import { __functionsZone__ } from "./constants";

import { wrapHttpsOnCallHandler } from "./sentry-serverless-firebase";
import { checkRequiredFields, checkUser } from "./utils";
import {
checkRequiredFields,
getAuthStrings,
getOrgAdmins,
isOrgAdmin,
} from "./utils";

export const queryAuthStatus = functions
.runWith({
Expand All @@ -35,50 +39,38 @@ export const queryAuthStatus = functions
checkRequiredFields(payload, ["organization"]);
const { organization } = payload;

await checkUser(organization, auth);

// It's safe to cast this to non-null as the auth check has already been done
const { email, phone_number: phone } = auth!.token!;
// At least one of the two will be defined
const authString = (email || phone) as string;
const authStrings = getAuthStrings(auth);
if (!authStrings.length) {
return { isAdmin: false, secretKeys: [] };
}

const authStatus: AuthStatus = {
isAdmin: false,
};
// Check if user is admin of the organization
const isAdmin = isOrgAdmin(
authStrings,
await getOrgAdmins(organization)
);

const orgRef = admin
// Get secret keys associated with the user's auth strings
const customersRef = admin
.firestore()
.collection(Collection.Organizations)
.doc(organization);
const customersRef = orgRef.collection(OrgSubCollection.Customers);
const customersByEmail = customersRef.where("email", "==", authString);
const customersByPhone = customersRef.where("phone", "==", authString);
const getCustomers = () =>
Promise.all(
[customersByEmail, customersByPhone].flatMap((ref) => ref.get())
);
.doc(organization)
.collection(OrgSubCollection.Customers);
const customersByEmail = customersRef.where("email", "in", authStrings);
const customersByPhone = customersRef.where("phone", "in", authStrings);

const [org, customers] = await Promise.all([
orgRef.get(),
getCustomers(),
const customers = await Promise.all([
customersByEmail.get(),
customersByPhone.get(),
]);

// query admin status
const orgData = org.data() as OrganizationData;
if (orgData) {
authStatus.isAdmin = orgData.admins.includes(authString);
}

// query customer status
const secretKeys = wrapIter(customers)
.flatMap(({ docs }) => docs)
.map((doc) => doc.data())
.map(({ secretKey }) => secretKey)
._array();

authStatus.secretKeys = secretKeys;

return authStatus;
return { isAdmin, secretKeys };
}
)
);
18 changes: 9 additions & 9 deletions packages/functions/src/checks/https.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { SanityCheckKind } from "@eisbuk/shared";
import { __functionsZone__ } from "../constants";
import { wrapHttpsOnCallHandler } from "../sentry-serverless-firebase";

import { checkUser, throwUnauth } from "../utils";
import { checkIsAdmin, throwUnauth } from "../utils";

import { newSanityChecker } from "./api";

Expand All @@ -28,7 +28,7 @@ export const dbSlotAttendanceCheck = functions
.region(__functionsZone__)
.https.onCall(
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const db = admin.firestore();
return newSanityChecker(
Expand All @@ -52,7 +52,7 @@ export const dbSlotSlotsByDayCheck = functions
wrapHttpsOnCallHandler(
"dbSlotSlotsByDayCheck",
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const db = admin.firestore();
return newSanityChecker(
Expand All @@ -71,7 +71,7 @@ export const dbSlotBookingsCheck = functions
.region(__functionsZone__)
.https.onCall(
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const db = admin.firestore();
return newSanityChecker(
Expand All @@ -91,7 +91,7 @@ export const dbBookedSlotsAttendanceCheck = functions
wrapHttpsOnCallHandler(
"dbBookedSlotsAttendanceCheck",
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const db = admin.firestore();
return newSanityChecker(
Expand All @@ -112,7 +112,7 @@ export const dbSlotAttendanceAutofix = functions
wrapHttpsOnCallHandler(
"dbSlotAttendanceAutofix",
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const db = admin.firestore();
const checker = newSanityChecker(
Expand Down Expand Up @@ -147,7 +147,7 @@ export const dbSlotSlotsByDayAutofix = functions
wrapHttpsOnCallHandler(
"dbSlotSlotsByDayAutofix",
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const db = admin.firestore();
const checker = newSanityChecker(
Expand Down Expand Up @@ -182,7 +182,7 @@ export const dbSlotBookingsAutofix = functions
wrapHttpsOnCallHandler(
"dbSlotBookingsAutofix",
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const db = admin.firestore();
const checker = newSanityChecker(
Expand Down Expand Up @@ -212,7 +212,7 @@ export const dbBookedSlotsAttendanceAutofix = functions
wrapHttpsOnCallHandler(
"dbBookedSlotsAttendanceAutofix",
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const db = admin.firestore();
const checker = newSanityChecker(
Expand Down
16 changes: 8 additions & 8 deletions packages/functions/src/migrations.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
import { __functionsZone__ } from "./constants";
import { wrapHttpsOnCallHandler } from "./sentry-serverless-firebase";

import { checkUser, getCustomerStats, throwUnauth } from "./utils";
import { checkIsAdmin, getCustomerStats, throwUnauth } from "./utils";

/**
* Goes through all 'slotsByDay' entries, checks each date to see if there are no slots in the day and deletes the day if empty.
Expand All @@ -29,7 +29,7 @@ export const pruneSlotsByDay = functions.region(__functionsZone__).https.onCall(
wrapHttpsOnCallHandler(
"pruneSlotsByDay",
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

try {
const db = admin.firestore();
Expand Down Expand Up @@ -94,7 +94,7 @@ export const deleteOrphanedBookings = functions
wrapHttpsOnCallHandler(
"deleteOrphanedBookings",
async ({ organization }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const orgRef = admin
.firestore()
Expand Down Expand Up @@ -130,7 +130,7 @@ export const populateDefaultEmailTemplates = functions
wrapHttpsOnCallHandler(
"populateDefaultEmailTemplates",
async ({ organization }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const batch = admin.firestore().batch();

Expand Down Expand Up @@ -166,7 +166,7 @@ export const removeInvalidCustomerPhones = functions
wrapHttpsOnCallHandler(
"removeInvalidCustomerPhones",
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const db = admin.firestore();

Expand Down Expand Up @@ -206,7 +206,7 @@ export const clearDeletedCustomersRegistrationAndCategories = functions
wrapHttpsOnCallHandler(
"clearDeletedCustomersRegistrationAndCategories",
async ({ organization }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const allCustomers = await admin
.firestore()
Expand Down Expand Up @@ -243,7 +243,7 @@ export const calculateBookingStatsThisAndNextMonths = functions
wrapHttpsOnCallHandler(
"calculateBookingStatsThisAndNextMonths",
async ({ organization }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();
try {
const db = admin.firestore();
const orgRef = db
Expand Down Expand Up @@ -336,7 +336,7 @@ export const normalizeExistingEmails = functions
wrapHttpsOnCallHandler(
"normalizeExistingEmails",
async ({ organization }: { organization: string }, { auth }) => {
if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

const db = admin.firestore();

Expand Down
4 changes: 2 additions & 2 deletions packages/functions/src/sendEmail/https.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
import { __functionsZone__ } from "../constants";

import {
checkUser,
checkIsAdmin,
checkSecretKey,
throwUnauth,
EisbukHttpsError,
Expand All @@ -39,7 +39,7 @@ export const sendEmail = functions
const { organization } = payload;

if (
!(await checkUser(organization, auth)) &&
!(await checkIsAdmin(organization, auth)) &&
!(
payload.type === ClientMessageType.SendCalendarFile &&
(await checkSecretKey({
Expand Down
4 changes: 2 additions & 2 deletions packages/functions/src/sendSMS/https.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { __functionsZone__ } from "../constants";

import { SMSStatusPayload } from "./types";

import { checkUser, throwUnauth } from "../utils";
import { checkIsAdmin, throwUnauth } from "../utils";

import { validateSMSPayload } from "./utils";

Expand All @@ -37,7 +37,7 @@ export const sendSMS = functions
) => {
const { organization } = payload;

if (!(await checkUser(organization, auth))) throwUnauth();
if (!(await checkIsAdmin(organization, auth))) throwUnauth();

// check payload
const validatedPayload = validateSMSPayload(payload);
Expand Down
6 changes: 3 additions & 3 deletions packages/functions/src/testData.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {

import { __functionsZone__ } from "./constants";

import { checkUser, throwUnauth } from "./utils";
import { checkIsAdmin, throwUnauth } from "./utils";

const uuidv4 = v4;

Expand All @@ -43,7 +43,7 @@ export const createTestData = functions.region(__functionsZone__).https.onCall(
wrapHttpsOnCallHandler(
"createTestData",
async ({ numUsers = 1, organization }: CreateTestDataPayload, context) => {
if (!(await checkUser(organization, context.auth))) throwUnauth();
if (!(await checkIsAdmin(organization, context.auth))) throwUnauth();

functions.logger.info(`Creating ${numUsers} test users`);
functions.logger.error(`Creating ${numUsers} test users`);
Expand Down Expand Up @@ -187,7 +187,7 @@ export const setupEmailForTesting = functions
{ organization, smtpHost = "localhost", smtpPort = 5000 },
context
) => {
await checkUser(organization, context.auth);
await checkIsAdmin(organization, context.auth);

const smtpConfig = {
smtpUser: "Foo",
Expand Down
4 changes: 2 additions & 2 deletions packages/functions/src/testSlots.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {

import { __functionsZone__ } from "./constants";

import { checkUser, throwUnauth } from "./utils";
import { checkIsAdmin, throwUnauth } from "./utils";

const CATEGORIES = Object.values(Category);
const NOTES = ["", "Pista 1", "Pista 2"];
Expand Down Expand Up @@ -101,7 +101,7 @@ const fillDay = async (day: DateTime, organization: string): Promise<void> => {
export const createTestSlots = functions
.region(__functionsZone__)
.https.onCall(async ({ organization }: { organization: string }, context) => {
if (!(await checkUser(organization, context.auth))) throwUnauth();
if (!(await checkIsAdmin(organization, context.auth))) throwUnauth();

functions.logger.info("Creating test slots...");

Expand Down
Loading

0 comments on commit 21099cc

Please sign in to comment.