firebase / firebase-functions-test

MIT License
232 stars 48 forks source link

Invalid field "authType" for an https callable function #162

Closed phcoliveira closed 1 year ago

phcoliveira commented 1 year ago

https://github.com/firebase/firebase-functions-test/blob/9f401ca71cc27cf766cc6e1e45e089e348c610d5/src/v1.ts#L221

Hello there. I think I found a bug, but it seems to be a mismatch between what is shown by the typing of the wrapped function.

    const wrapped = testFunctions.wrap(addPlayer)

    const result = await wrapped(data, {authType: 'USER', auth: {uid: 'p2'}})

Here is what I have. The type of the function wrapped suggests that the field authType is valid. However, while debugging a silent error, I found out that this field is not present in the array validFields.

image

TheIronDev commented 1 year ago

Hi @phcoliveira ,

Apologies for the delay!!!

I'm chasing this down right now.

What type of function is addPlayer? (Is there a github link?)

I ask because it looks like CallableFunctions allow for a subset of options compared to non-callable functions.

Callable Options vs Non-Callable option list

Thank you for your patience!!

Tyler

phcoliveira commented 1 year ago

Hello @TheIronDev, thank you for catching this up.

addPlayer is a callable function, I will write it down bellow.

I can understand that a callable function only accepts a subset of a request function. Perhaps it is only possible to determine this at runtime. But I do believe it is possible to determine the right set of options based on a conditional typing at compile time.

In case you need the function addPlayer, here it is:

/* eslint-disable import/no-unresolved */
import type {
  AddPlayer,
  Party,
  ServerUpdate,
} from '@all-aboard/firebase-js-sdk/party'
import {serverTimestamp} from '@firebase/database'
import {getApp} from 'firebase-admin/app'
import {getAuth} from 'firebase-admin/auth'
import {getDatabase} from 'firebase-admin/database'
import * as functions from 'firebase-functions'

export const addPlayer = functions.https.onCall(
  async (data: AddPlayer, context): Promise<void> => {
    const playerUid = context.auth?.uid

    if (!playerUid) {
      throw new functions.https.HttpsError(
        'unauthenticated',
        'Must be authenticated'
      )
    }

    const {key: partyKey, password} = data

    if (!partyKey || !password) {
      throw new functions.https.HttpsError(
        'invalid-argument',
        'Must pass a partyKey and password'
      )
    }

    const serverApp = getApp('server')
    const database = getDatabase(serverApp)
    const partyRef = database.ref(`/parties/${partyKey}`)
    const partySnap = await partyRef.once('value')

    if (!partySnap.exists()) {
      throw new functions.https.HttpsError('not-found', 'Resource not found')
    }

    const partyVal = partySnap.val() as Party
    const {password: serverPassword, playerUids} = partyVal
    const isAdmin = !playerUids // The first player must be the admin

    if (serverPassword !== password) {
      throw new functions.https.HttpsError(
        'permission-denied',
        'Wrong password'
      )
    }

    const update: ServerUpdate = {
      playerUids: {
        ...playerUids,
        [playerUid]: isAdmin,
      },
      updatedAt: serverTimestamp(),
    }

    await partyRef.update(update)
    const auth = getAuth(serverApp)
    await auth.setCustomUserClaims(playerUid, {partyKey, isAdmin})
  }
)
TheIronDev commented 1 year ago

I took an initial stab at splitting the return function apart, but I was only able to get that error thrown during runtime. Which doesn't mean compile time isn't doable... it just means I haven't gotten it yet. 🤷

I'm happy to review any PRs that does get a compile-time check working though! 😄

phcoliveira commented 1 year ago

I will try to provide a PR for this. In case I am unable to do it, I will let you know.

TheIronDev commented 1 year ago

Thank you!! I'll review when it pops up ASAP!

phcoliveira commented 1 year ago

@TheIronDev, I made it work, but I can not say if it is pertinent or even acceptable.

I will push the changes to my fork, but I want to leave some pictures here to show you that it works.

image

image

So, with my changes, TypeScript is smart enough to decide which options to pick based on the function passed to be wrapped.

phcoliveira commented 1 year ago

@TheIronDev, I provided another signature for the wrap function and everything works now. Please check it out.

https://github.com/firebase/firebase-functions-test/pull/166/files