appy-one / acebase-server

A fast, low memory, transactional, index & query enabled NoSQL database server for node.js that easily syncs with browser and node.js clients and servers
MIT License
32 stars 14 forks source link

Auth rules don't cascade on error #92

Closed fahhem closed 10 months ago

fahhem commented 1 year ago

Hello! I'm trying to migrate an existing Firebase app and it seems auth rules don't cascade when a shallow rule errors (such as when auth is null and the rule mentions auth.token).

This seems to be the case because in rules.ts, in isOperationAllowed, if the rule throws an exception then there's no chance to cascade down. If I replace the return { ... code: 'exception'... }; line with a continue; then I get the expected behavior.

Use case:

I have a mix of authenticated and unauthenticated/anonymous users, and I would like anonymous users to be able to read a few specific paths. An example is minClientBuild. However, I would also like to restrict the ability to read/write the root path (you can move these fields under client/ and limit the ability to read/write /client/ but make /client/minClientBuild anonymous readable):

Here's my rules.json:

{
  "rules": {
    "minClientBuild": {
      ".read": "true",
      ".validate": "newData.isNumber()",
      "$other": {
        ".validate": false
      }
    },
    ".read": "((auth.token.expires == null || auth.token.expires > now) && auth.token.provider == null ? auth.uid : null) == 'server'",
    ".write": "((auth.token.expires == null || auth.token.expires > now) && auth.token.provider == null ? auth.uid : null) == 'server'",
    "$other": {
      ".validate": false
    }
  }
}

In Firebase, this works, as the deeper and more-permissive rule (".read": "true"} trumps all the shallower rules. However, in acebase-server, if I write that to the rules.json, and add logging, I see that the top-level .read rule is being run and fails since auth is null, and it never gets around to trying /minClientBuild/.read out.

According to: https://firebase.google.com/docs/rules/insecure-rules#improperly_inherited_rules

appy-one commented 1 year ago

@fahhem In AceBase, rule functions should be written using the correct Javascript syntax. Rules causing an exception because they have errors, should not allow access - this is a deliberate decision. I notice I wrote that the rules in AceBase are identical to those in Firebase in the documentation, but I will change that sentence because it is not true. See https://github.com/appy-one/acebase-server#setup-authorization-rules for more info!

fahhem commented 1 year ago

That's unfortunate! I'm trying to migrate an existing product to AceBase, and the closer the compatibility, the better. We're hoping to migrate over fully and basically switch our payments to Google to funding acebase instead, but if migration requires too many changes we'll have to stop

appy-one commented 1 year ago

@fahhem I'm not sure that is unfortunate!

The initial plan was to be 100% compatible with Firebase's rules but while implementing I found that they were very unfriendly to code, and extremely hard to read for others. Because AceBase is entirely JS based, it made sense to let the rules be executed by V8 instead of manually parsing and executing.

The result is that you can now write your rules in JS (Or TypeScript), include them in your project's source code so you can debug, step through, console.log, execute misc. (async) code such as external API calls, and add comments. Bonus: you can even unit test your rules!

I'm sure that if you migrate and convert your rules to JS/TS, you will never look back again!

fahhem commented 1 year ago

The downside of this is that a project that works in Firebase can't migrate over other than completely. We would like to migrate certain instances over to Acebase first, then make sure any bugs/perf issues/differences are hammered out, then migrate over other instances. Would you consider a firebase_compat flag that would alter things like this? Once we're fully migrated, I'd happily work on switching off the firebase_compat flag in our codebase and report back.

appy-one commented 10 months ago

Sorry for the wait, but I have to say no. I made a deliberate choice not to support Firebase's rule syntax because they are such a mess. If you want to migrate your rules to AceBase, you'll have to rewrite them to javascript.