useFirestoreDocData & useFirestoreDoc won't throw a promise on permission denied

[RodyGL]

Version info

React: 16.12.0

Firebase: 7.5.2

ReactFire: 2.0.0-canary.1fce6b9

Test case

I want to use role based authentication on documents, so when a document is successfully read in the app and then I change the access of that user, the listener throws an error instead of a Promise.

const CompanyCheck: React.FC = ({ children }) => {
  const { companyId, setCompany } = useCompanyContext();

  const firestore: Firebase['Firestore'] = useFirestore();

  const companyRef = useRef(
    firestore()
      .collection('companies')
      .doc(companyId)
  );

  useEffect(() => {
    companyRef.current = firestore()
      .collection('companies')
      .doc(companyId);
  }, [companyId, firestore]);

  // This won't throw a promise on permission changes after successfull read.
  const companyDoc = useFirestoreDocData<Company>(companyRef.current);

  useEffect(() => {
    setCompany(companyDoc);
  }, [companyDoc, setCompany]);

  return <>{children}</>;
};

Steps to reproduce

Use a correct example of read operation with role access, then once the data is displayed remove the access to the document and error boundary won't catch any error.

Expected behavior

Throw a promise like when initial read fails so ErrorBoundary can catch the error.

Actual behavior

Don't throw a Promise and error can't be handled. Error message is: 'permission-denied'

[jhuleatt]

👋 Hi @RodyGL! You can already catch the permission denied error with an error boundary: CodeSandbox example.

To clarify ReactFire's promise-throwing behavior, it does this to integrate with Suspense so that React knows to render a fallback until the read is ready. It's not related to permissions, only loading state.

Does that solve this issue, or am I misunderstanding the problem?

[RodyGL]

Hi @jhuleatt! Thanks for your reply. You're almost there with the issue, the example you provided works because the first render works as expected. To replicate the issue, first you need to read successfully the document, once the data is loaded then you remove the user access to that document (firestore.rules). So, what I would expect after that is to see the fallback render once the listener realized you don't have access to that query anymore but instead throws the error not a Promise to further catch by the ErrorBoundary.

Allow access:

rules_version = '2';
service cloud.firestore {
  match /databases/{database}/documents {
    match /{document=**} {
      allow read, write: if true;
    }
  }
}

Remove access:

rules_version = '2';
service cloud.firestore {
  match /databases/{database}/documents {
    match /{document=**} {
      allow read, write: if false;
    }
  }
}

[jhuleatt]

Throwing a Promise is only for loading state management, not rules evaluation/error handling. Throwing an error on a read error (in this case, failing a rules check) is intended behavior and should be handled by an error boundary, not Suspense.

Read more about Suspense vs Error Boundaries in the React docs.

[RodyGL]

That's what I meant. After changing the access an error is triggered due to permission denied but the Error Boundary is not catching any error and because of that the fallback is not showing.

[jhuleatt]

Thanks for clarifying. I managed to repro:

1. Enabled read permissions
2. Reloaded the page, and got a successful read.
3. Set the rules back to denying the read, waited a minute to let the new rules propagate
4. Waited a minute or so (for the Firebase library to poll the backend?). <-- this is the part I didn't do before
5. The page gets into a weird state. An error is logged, but the UI still shows the value that was read successfully
6. Refreshed the page and the error boundary works fine

This is really weird, thanks for reporting! I'll try to figure out what's going on.

[RodyGL]

Thanks a lot for your reply and effort @jhuleatt. I apologize if I wasn't clear enough with the issue.

[chdsbd]

This is still an issue. I would expect that the hooks wouldn't throw any errors, because they have the error attribute in their response

[steveoh]

Yeah this just confused my expectations for how the library would be used. I expected the status to be loading, and then error or something opposite of success and the error property would contain the error that is currently being thrown. This would be similar behavior to something like tanstack/query. They also support suspense so maybe things have changed since suspense was an experimental feature for some time?

You can't try catch a hook because of the rules of hooks. I'm not using concurrent mode with suspense and it is an opt in feature. An error boundary is fine for my situation but I can't imagine that is the case for everyone.

<FirebaseAppProvider firebaseConfig={firebaseConfig} suspense={true}>

Shouldn't suspense === true be checked and conditionally throw if it's true and otherwise let the status reflect the error and the error property can give more details?

It appears there is some question in the design based on some comments in the code

https://github.com/FirebaseExtended/reactfire/blob/9429194d9dfed9a4b37129661d230ebcd0071e89/src/useObservable.ts#L135-L139

cc @jhuleatt since you're referenced in the code todo question