bug(populate): Populating with missing permissions

[nopol]

What is the current behavior? When using populate-hook without permission, it raises an unhandled error. "Unhandled Rejection (Error): permission_denied at /... Client doesn't have permission to access the desired data."

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via codesandbox or similar.

database.rules.json

{
  "rules": {
    "data": {
      "$id": {
        ".read": "data.child('visibility').val() === 'public' || data.child('owner').val() === auth.uid"
      },
    "users": {
      "$id": {
        ".read": "$id === auth.uid"
      }
    }
}

SomeComponent.js

const populates = [
  { child: 'data', root: 'data', keyProp: 'key' },
]

const enhance = compose(
  firebaseConnect((props) => [
    { path: \`users/${props.id}\`, populates }
  ]),
  connect(
    ({ firebaseReducer }) => ({
      users: populate(firebaseReducer, 'users', populates),
    })
  )
)

What is the expected behavior? The to populate object should be provided without populating.

Which versions of dependencies, and which browser and OS are affected by this issue? Did this work in previous versions or setups? V3.7.0

[nopol]

As far as I can tell it would be an easy fix:

src/utils/populate.js

/**
 * @private
 * @param {object} firebase - Internal firebase object
 * @param {object} populate - Object containing root to be populate
 * @param {object} populate.root - Firebase root path from which to load populate item
 * @param {string} id - String id
 * @returns {Promise} Resolves with populate child
 */
export function getPopulateChild(firebase, populate, id) {
  return firebase
    .database()
    .ref()
    .child(`${populate.root}/${id}`)
    .once('value')
    // Return id if population value does not exist
    .then((snap) => snap.val())
    // return id, when error occurred (permission missing)
    .catch(() => id)
}

since i can not run tests on windows with this project i can't submit a push-request

cheers

[prescottprue]

That change would prevent an error from being thrown. Not sure of the impact, but I'm thinking we may want to have an option to preserve backwards compatibility

[nopol]

The design does not specify errorhandling. But I think there is a error handling missing. As far as I understand this topic, populate is designed to be used as middleware and there is no other way to handle errors as inside the middleware. I mean, how can you handle the errors else then there? So as far as I can tell, it can not break things but help using permissions on database.

[prescottprue]

Well - to my knowledge an unhandled error would be thrown without placing a catch there (which I believe is where the message that you originally saw came from). I know it isn't traditional "error handling", but by not directly handling the error a message will be displayed to the developer in the console. Adding the catch will mean that the error is not thrown, which will mean that this message will not appear, it doesn't mean that the invalid/blocked query isn't still happening.

In the case you are describing you are actually querying data which you do not have permission to view which is usually not intended - you are ok with this, but it would make it more difficult to debug if this is not the intention.

What is the reason that a user would have keys that they do not have permission to view on their object? Shouldn't that list just be of keys that the user does have access to? It seems like otherwise you are attempting to use rules as a sort of filter, which they are not meant for. Or Maybe I'm misunderstanding?

As for the pull request - the tests will be run within the CI when you create the pull request, so you don't have to run them locally. We can debug failures from there before merging in

[nopol]

As of React V16 unhandled Promise.reject() will crash the app. https://reactjs.org/blog/2017/07/26/error-handling-in-react-16.html

The user is holding own data and can turn visibility public, to make the data visible to other users. Users can view other users and see also keys they are not allowed to query. And you're correct: I should be able to handle this by extending the query with a filter. I guess it is possible to extend the populate by a query, is it?

[prescottprue]

Yeah the crash or hitting an error boundary you have set is the intention so that you notice this during development - thats part of why I'm saying we shouldn't just return a value in the catch and not throw. The error is indicating that you are attempting to read data that your security rules have specified should not be allowed - as mentioned this can appear if you are attempting to use your security rules as a filter (not what they are for).

Still not fully understanding your use case, but it sounds like you might want to reorganize the data a bit - it is usually not a great practice to be sharing user objects with all users. If you can describe the intention of the app/feature a bit more I might be able to give some advice for reorganizing the data

You can extend the populate query, but I think that organizing the data would be a better approach

[nopol]

In my understanding populate should enable the developer to nest distributed data. As DaaS need to be secured by rules, the data needs to be accessed by query/filter. the combination is intended way, to ensure security. I couldn't figure out how to add a filter/query to populate. perhaps i misused populate and in this case it's not intended. I'll instead use a filtered query. the reference in the user object does provide the information that the user has data, but also can be provided by a separated flag and be removed/not used. thank you for assist, have a good time. :-)