We were unable to load Disqus. If you are a moderator please see our troubleshooting guide.

Johnny Oshika • 4 years ago

Great article, thank you! Just a note that your fieldNotUpdated(field) has an unintended security hole. If a field is defined but the user deletes the field, it'll be accepted. For example:

ref.set(
{
foo: firebase.firestore.FieldValue.delete(),
},
{ merge: true },
)

The expected behaviour for fieldNotUpdated is that it can't be changed by the user, so removing shouldn't be allowed.

Johnny Oshika • 4 years ago

I should have explained why it resolves to true with a field is deleted. It's because this resolves to true: !(field in request.resource.data)

Nico • 4 years ago

Thanks, Johnny! Good catch, and thanks for sharing.

You could also delete the field when omitting it in a `set()` call.

I've created a new version of the function and renamed it to better reflect its usage. It should work for all use cases: create, update, resource exists or doesn't exist, field deleted etc. It gets quite convoluted, but that's one of the disadvantages to Firestore (no wonder why so many Firebase apps have unintended security flaws). Feel free to check it out :)

Johnny Oshika • 4 years ago

Thanks Nico, your changes look good and it looks very similar to the solution I came up with. I have a slight variation with `toSet()` in my `allow create` rule whereas you don't. I didn't realize that this would work without the `toSet()`. I also have one function that supports both creating and updating. Here's my solution:


function isCreating() {
return resource == null;
}

function onlyAffectingTheseFields(fields) {
return isCreating()
? request.resource.data.keys()
.toSet()
.hasOnly(fields)
: request.resource.data.diff(resource.data)
.affectedKeys()
.hasOnly(fields);
}

I can call those functions like this in my rule:

allow create, update: if onlyAffectingTheseFields(['foo', 'bar']);

Nico • 4 years ago

Good idea to merge them into one function and use the isCreating() helper. I don't actually use these in production, but if (when) I do, I'll probably also include that function for convenience.

And yes, I didn't see the need to include toSet() since keys() returns a list, and `hasOnly()` takes a list:

request.resource.data.keys().hasOnly(fields)

Did you keep toSet()?

Johnny Oshika • 4 years ago

I just tested it and request.resource.data.keys() and request.resource.data.keys().toSet() are identical for both create and update. Interesting. I thought they would differ for update if we were only doing a partial set during an update, but I was wrong. I also ran all my unit tests with and without .toSet() and they all pass in both scenarios. I kept the .toSet() there in my production code for now.

Nasser Parooei • 5 years ago

Thank you brother you saved my life

Christian Valadez • 4 years ago

Saved me hours after searching through the firestore docs. Thank you!

Johnny Oshika • 4 years ago

Not sure if you know, but the grey text of your comments against the dark blue background makes them impossible to read without highlight the text with my mouse. It looks like this for me: https://uploads.disquscdn.c...

Nico • 4 years ago

I created a "dark mode" for the website which is automatically enabled if the visitor prefers dark color schemes (`prefers-color-scheme`). I didn't check the comment section, though.

But it seems that Disqus has setting that selects a color scheme automatically, which I just turned on. Looks much better! Thanks a lot for letting me know!