firebase-js-sdk icon indicating copy to clipboard operation
firebase-js-sdk copied to clipboard

Multi-location update() that relies on preceding update(s) fails

Open Pingviinituutti opened this issue 3 years ago • 0 comments

[REQUIRED] Describe your environment

  • Operating System version: WSL Ubuntu 20.04
  • Browser version: Not relevant
  • Firebase SDK version: relevant package versions from package.json
    • "firebase": "^9.4.1"
    • "firebase-tools": "^11.0.1"
    • "@firebase/rules-unit-testing": "^2.0.4"
  • Firebase Product: database

emulator version: CLI Version: 11.0.1 Platform: linux Node Version: v16.15.0

[REQUIRED] Describe the problem

Multi-location update() with updates that rely on preceding updates fail in the situation when:

  • a validation rule in a sibling path relies that the $id exists in another location, i.e. writing under exampleData/$id requires first that example/$id exists.

Steps to reproduce:

Check the unit test below.

Relevant Code:

import {
	assertFails,
	assertSucceeds,
	initializeTestEnvironment,
	RulesTestEnvironment,
} from "@firebase/rules-unit-testing";

import { set, ref, update } from 'firebase/database';

let testEnv: RulesTestEnvironment;

const rules = {
	"rules": {
		".write": "false",
		".read": "false",
		"example": {
			"$id": {
				".write": true
			}
		},
		"exampleData": {
			"$id": {
				".write": true,
				".validate": "root.child('example/' + $id).exists()"
			}
		}
	}
}

beforeEach(async () => {
	jest.spyOn(console, 'warn').mockImplementation(() => { });
	testEnv = await initializeTestEnvironment({
		projectId: "demo-project-1234",
		database: {
			host: 'localhost',
			port: 9000,
			rules: JSON.stringify(rules),
		},
	});
});

afterEach(async () => {
	await testEnv.clearDatabase();
});

describe("Permissions for unauthorized users", () => {
	it('Test set() to check if it can handle updates that rely on preceding updates completing successfully', async () => {
		const db = testEnv.unauthenticatedContext().database();
		const id = 'test-id';

		// Basic sanity checks
		await assertSucceeds(set(ref(db, `example/${id}`), true));
		await assertFails(set(ref(db, `exampleData/id-does-not-exist`), false));
		// below should succeed because we created the entry under example two lines above
		await assertSucceeds(set(ref(db, `exampleData/${id}`), true));

		const id_2 = 'test-id-2';
		const updates = {
			[`example/${id_2}`]: true,
			[`exampleData/${id_2}`]: true,
		}
		// loop through updates and apply them with set()
		Object.entries(updates).forEach(async ([key, value]) => {
			// console.log(key, JSON.stringify(value));
			await assertSucceeds(set(ref(db, key), value));
		})
	});

	it('Test update() to check if it can handle updates that rely on preceding updates completing successfully', async () => {
		const db = testEnv.unauthenticatedContext().database();
		const id = 'test-id';

		// Basic sanity checks
		await assertSucceeds(set(ref(db, `example/${id}`), true));
		await assertFails(set(ref(db, `exampleData/id-does-not-exist`), false));
		// below should succeed because we created the entry under example two lines above
		await assertSucceeds(set(ref(db, `exampleData/${id}`), true));

		const id_2 = 'test-id-2';
		const updates = {
			[`example/${id_2}`]: true,
			[`exampleData/${id_2}`]: true,
		}
		// apply all updates nicely with update()
		// But this fails :(
		await assertSucceeds(update(ref(db), updates));
	});
});

Pingviinituutti avatar Aug 09 '22 06:08 Pingviinituutti

@Pingviinituutti - you can reference the data to be written using newData. If you modify the validate rule to be:

...
".validate": "root.child('example/' + $id).exists() || newData.parent().parent().child('example/' + $id).exists()"
...

maneesht avatar Aug 22 '22 17:08 maneesht

@Pingviinituutti - did this fix your problem?

maneesht avatar Aug 31 '22 18:08 maneesht

Hey @Pingviinituutti. We need more information to resolve this issue but there hasn't been an update in 5 weekdays. I'm marking the issue as stale and if there are no new updates in the next 5 days I will close it automatically.

If you have more information that will help us get to the bottom of this, just add a comment!

google-oss-bot avatar Sep 07 '22 01:09 google-oss-bot

@maneesht Yeah, this solution works 👍 Would not have thought of this. But then again, based on how update works, of course there is no old data to be found so the validation should also check if the previous updates are in the new data.

Thanks! 😊

PS. Validating deep trees will be very cumbersome with this haha :D Maybe there could be something like newData.root available for the newData methods. This way one could skip calling parent multiple times.

Pingviinituutti avatar Sep 10 '22 07:09 Pingviinituutti

@Pingviinituutti thank you for bringing that up. While we don't have any work for this currently planned, we will keep it in mind for the next planning phase.

maneesht avatar Sep 12 '22 16:09 maneesht