bbc / digital-paper-edit-firebase

Firebase version of DPE
https://digital-paper-edit-dev.web.app/
Other
8 stars 5 forks source link

Tighten Firestore security rules #276

Open tamsingreen opened 3 years ago

tamsingreen commented 3 years ago

As alerted by Pietro, in firestore,rules: match /projects/{document=**} {
allow read, write } Allows any authenticated user to read/write to a project.

We need to lock this down to admin users only and check where this rule is in use in the app & rewrite where this would prevent legitimate access.

pietrop commented 3 years ago

Describe the bug

I think on your end the data is is also organized as follows(?):

120530971-16d85680-c3ac-11eb-9213-cebffaca15f9

At the moment client side we query the /projects collection and only display to the users the once they are associated with.

by using list for projects In the current security rules and locking down further other operations and nested routes.

In some situations, it's useful to break down read and write into more granular operations. For example, your app may want to enforce different conditions on document creation than on document deletion. Or you may want to allow single document reads but deny large queries.

A read rule can be broken into get and list, while a write rule can be broken into create, update, and delete

From firestore docs Granular operations

This is because a collection, especially at the top level, cannot have tighter security without disallowing the list operation.

All match statements should point to documents, not collections.

From firestore Basic read/write rules

The problem is that when we list a project, "anyone" can theoretically see the project title, description but most importantly the emails of the users associated with that project and their corresponding roles.

Expected behavior

Ideally we'd want to be able to lock down the project list or restructure the data to be more secure.

Additional context

looking at the firestore doc Securely query data & Secure data access for users and groups there's another pattern, we can consider where the /projects/ collection could be locked down, and a /users/{userId} collection/document could contain the info to match users to projects.

emettely commented 3 years ago

We did need the read write initially for users to be able to create the projects collection. The reason for this is because of this flow (see https://github.com/bbc/digital-paper-edit-firebase/blob/master/firestore.rules) also: Users login -> users authenticated using the users collection. Users need to be allowed to read projects collection to check if their project exists & write if it doesn't the user needs to create a project. ... etc.

      match /projects/{document=**} {    
          allow read, write
      }
  match /projects/{pId} {    
        allow read, write: if isOnProject(pId) || isAdmin();        
      }

      match /projects/{pId}/{document=**} {
        allow read, write: if isOnProject(pId) || isAdmin();        
            } 
    }
}
}

The get / create operation seem like a good evolution / only using create rather than an entire read and write :)