goldibex / targaryen

Test Firebase security rules without connecting to Firebase.
ISC License
242 stars 36 forks source link

newData.parent().parent() not referencing the root #121

Closed christianlacerda closed 7 years ago

christianlacerda commented 7 years ago

If I have the following rules:

{
  "rules": {
    "users": {},
    "groups": {
      "$groupId": {
        ".write": "newData.parent().parent().child('users').child(auth.uid).exists()"
      }  
    }
  }
}

The new newData.parent().parent() is not correcty pointing to the root, in my test logs I get:

newData.parent().parent() = {"path":"","exists":true}
newData.parent().parent().child('users') = {"path":"users","exists":false}

The rule is working fine using on Firebase.

Thanks!

dinoboff commented 7 years ago

@christianlacerda I cannot reproduce the bug; this test is passing:

https://github.com/dinoboff/targaryen/commit/924b9ee46b3b877daa182323c747a87d72e861bd

Is there something wrong with my test? which version are you using?

christianlacerda commented 7 years ago

@dinoboff It seems you're right. I've setup a new clean test and it worked as expected. It must be something else with my rules.

I appreciated your time.

dinoboff commented 7 years ago

You're welcome.

Could it be something to do with your initial data since the debug message says "/users" is empty.

christianlacerda commented 7 years ago

You might be correct. But what if my initial data is empty and I am sending my /users in a multi-path update. Shouldn't newData.parent().parent().child('users') be considered then?

dinoboff commented 7 years ago

Yes and AFAIK it does.

dinoboff commented 7 years ago

@christianlacerda I added a test to illustrate multi-location update:

    it('should fix 121 2/2', function() {
      const rules = {
        rules: {
          users: {
            $uid: {
              '.write': true
            }
          },
          groups: {
            $groupId: {
              '.write': 'newData.parent().parent().child("users").child(auth.uid).exists()'
            }
          }
        }
      };
      const data = {};
      const db = database.create(rules, data).as({uid: 'bob'}).with({debug: true});

      expect(db.update('/', {
        'users/bob': true,
        'groups/users': true
      }).allowed).to.be.true();
    });
christianlacerda commented 7 years ago

Thanks a lot @dinoboff ! I was definitely not writing my multi-path test like that, I'll give it try then.

dinoboff commented 7 years ago

With the chai plugin, it's something like:

expect({uid: 'bob'}).can.patch({
  'users/bob': true,
  'groups/users': true
}).to.path('/');

The important thing is, like with ref.update(data), each patch data key must be set to a path instead of using nested object; e.g. this is probably wrong:

expect({uid: 'bob'}).can.patch({
  users: {bob: true},
  groups: {users: true}
}).to.path('/');

It simulates setting 'users' to '{bob: true}' and 'groups' to '{users: true}', not to add items to the 'users' and 'groups' collections.

christianlacerda commented 7 years ago

Thanks @dinoboff That simply just worked!