Embed metadata directly into the RPC call #979

ryanpbrewster · 2018-07-03T22:42:26Z

This is an alternative approach to #978 with less duplicated code.

ryanpbrewster · 2018-07-03T22:44:46Z

packages/firestore/src/platform_node/grpc_connection.ts

+    const f = rpc.bind(stub);
+    return function() {
+      return f(arguments, metadata);
+    }


The linter is complaining about this. Any ideas? I'm struggling with the fact that rpc.bind(stub) is a potentially 0-, 1-, or 2-ary function.

mikelehen

I'm generally a fan of this approach, but I'd like Gil to weigh in on this before we move forward.

mikelehen · 2018-07-03T22:51:17Z

packages/firestore/src/platform_node/grpc_connection.ts

+      'google-cloud-resource-prefix',
+      `projects/${this.databaseInfo.databaseId.projectId}/` +
+        `databases/${this.databaseInfo.databaseId.database}`
+    );


FWIW, I think it's probably worth keeping this metadata creation in a helper. createMetadata(this.databaseInfo, token) or whatever.

mikelehen · 2018-07-03T22:55:40Z

packages/firestore/src/platform_node/grpc_connection.ts

+    const f = rpc.bind(stub);
+    return function() {
+      return f(arguments, metadata);
+    };


I'd be a bit surprised if this works. I think arguments will be an array-like object containing the arguments, which probably isn't what the RPC function expects? I think you can do something like:

return function(...args) { f(...args, metadata); }

and you could get rid of f and just do:

return function(...args) { rpc.bind(stub, ...args, metadata); }

I think that's right anyway... I could be slightly off.

Nice, that did the trick! ((It was definitely broken the way I had it.))

ryanpbrewster · 2018-07-03T23:14:35Z

packages/firestore/src/platform_node/grpc_connection.ts

-  return grpc.credentials.combineChannelCredentials(
-    channelCredentials,
-    callCredentials
-  );


Cool upside of this change: ssl: false now works with the Node SDK 😄

wilhuff

I’m not a fan of this change. It essentially circumvents an intentional design choice of gRPC. The ssl: false thing was only ever intended to work against backends that did not require auth (i.e. the v1alpha1 emulator and pre-auth-integrated hexa).

A better path forward would be to plumb a non-credentialed identity claim through when ssl is off. That way we respect the default guarantee that gRPC won’t disclose credentials over an insecure channel but still allow ourselves to make claims to the emulator that allows rule simulation to work.

ryanpbrewster · 2018-07-09T23:59:55Z

Talked in-person about this and clarified a few things, plus identified a few key concerns:

What wire-level differences are there between metadata embedded in the call-credentials vs metadata directly attached to the RPC?
Do those wire-level differences manifest in different behavior on the backend (specifically, will the backend have to make more frequent authorization checks if we provide the auth token directly in the RPC metadata instead of embedding it in the call credentials)?

I believe there are no differences in the wire-level behavior. Looking at the gRPC source:

For a simple RPC, we start at: client.ts: makeUnaryCall() --- https://github.com/grpc/grpc-node/blob/master/packages/grpc-js-core/src/client.ts#L156
which sets up a stream to handle the RPC channel.ts: createStream() --- https://github.com/grpc/grpc-node/blob/master/packages/grpc-js-core/src/channel.ts#L307
which calls channel.ts: startHttp2Stream() --- https://github.com/grpc/grpc-node/blob/master/packages/grpc-js-core/src/channel.ts#L256
which generates metadata from the call credentials, then merges it with the per-RPC metadata: call-credentials-filter.ts: sendMetadata() --- https://github.com/grpc/grpc-node/blob/master/packages/grpc-js-core/src/call-credentials-filter.ts#L28

so regardless of where you store the metadata, it ends up on the wire in the same place. The main differences, as far as I can tell, are:

The call credentials cannot be used with an insecure channel (by design!)
The call credentials expose a "generator" API, so you get token refreshes and such "for free". If we want to attach the metadata directly to each RPC we have to manually call the "generator" function. The createMetadata() method introduced in this PR does this.

mikelehen · 2018-07-10T15:32:16Z

This may be worth discussing in person, but I find the gRPC design to be inconvenient and harmful to us. We never want to send any data whatsoever (credentials or otherwise) to the production backend over an unencrypted channel. Fortunately the production backend won't accept non-ssl traffic and so we are reasonably safe in that regard. Nobody can use ssl: false in production and have it work. That said, for testing / development, it is entirely reasonable to use ssl: false against local emulators, etc. whether or not there's some sort of "credential" involved. Making ssl: false actually work fully is quite useful to us (in addition to Ryan's goal, this will make the node client work against our hexa env).

I think at the point anybody (us or a customer going way off the beaten path) uses ssl: false, all security bets are off. And this PR seems to be a straightforward way to make ssl: false work (albeit by circumventing gRPC's design goals, apparently). If we wanted to re-add some minor guardrails we could try to make sure they've also configured to a non-production host or something.

mikelehen · 2018-07-10T15:58:31Z

After talking to Gil, I think I just re-hashed what you guys already discussed and so my latest comments can likely be ignored. Thanks/sorry!

wilhuff · 2018-07-10T15:59:21Z

Just to clarify: @ryanpbrewster and I discussed this yesterday and concluded that my earlier comments were unworkable. I'm on board with the way we're doing this now.

wilhuff

LGTM

@davideast

* Add @davideast as a CODEOWNER (#996) * Embed metadata directly into the RPC call (#979) * Embed metadata directly into the RPC call * [AUTOMATED]: Prettier Code Styling * Use ...args * [AUTOMATED]: Prettier Code Styling * Minimize diff * Add the OAuth assertion back in * Added missing type for optional database url. (#1001) * RxFire Realtime Database (#997) * initial database code * test setup * database tests * auditTrail and database tests * [AUTOMATED]: Prettier Code Styling * [AUTOMATED]: License Headers * Josh's comments. Database docs * [AUTOMATED]: Prettier Code Styling * Firestore docs * auth docs * declaration fixes * switch to peerDeps * [AUTOMATED]: Prettier Code Styling * test config * Expose array transforms and array contains queries. (#1004) Also remove test code that was combining multiple array contains queries since those were disallowed in 04c9c3a. * Move fieldFilter (free function) to Filter.create() (#988) This is a refactoring to unify filter creation across platforms. * Enable firestore sdk to talk to emulator (#1007) * Enable firestore sdk to talk to emulator * [AUTOMATED]: Prettier Code Styling * Revert firestore sdk changes * [AUTOMATED]: Prettier Code Styling * Revert credentials.ts * Cleanup * [AUTOMATED]: Prettier Code Styling * Set webSafe=false * Combine initializeTestApp and initializeFirestoreTestApp * [AUTOMATED]: Prettier Code Styling * Cleanup * [AUTOMATED]: Prettier Code Styling * Update major version since this is a breaking change that will cause the testing sdk to no longer work with old versions of the RTDB emulator * Completely remove admin sdk * Change version back to 0.1.0 * Setting GarbageSource in SyncEngine's constructor (#1010) * b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. (#1014) This fixes an issue occurring when a limbo target receives a documentUpdate, then a global snapshot, and then a CURRENT. Because there was a global snapshot before the CURRENT, WatchChangeAggregator has no pending document updates and calls SyncEngine.targetContainsDocument() to see if we previously got any document from the backend for the target. See: https://github.com/firebase/firebase-js-sdk/blob/6905339235ad801291edc696dd75a08e80647f5b/packages/firestore/src/remote/watch_change.ts#L422 Prior to this change, targetContainsDocument() returned false because it relies on our Views to track the contents of the target, and we don't have Views for limbo targets. Thus WatchChangeAggregator incorrectly manufactures a NoDocument document update which deletes data from our cache. The fix is to have SyncEngine track the fact that we did indeed get a document for the limbo resolution and return true from targetContainsDocument(). * Updating yarn.lock * Add @firebase/util as a dep of @firebase/testing * Allow remote updates from watch to heal a cache with synthesized deletes in it (#1015) * Write a spec test for the busted cache * Modify spec test to demonstrate deletedDoc issue. (#1017) * Allow updates for targets where the document is modified * Fix getRemoteKeysForTarget() method name in comment. (#1020) While porting I noticed this was slightly wrong. targetContainsDocument() is the method in WatchChangeAggregator. The SyncEngine method I meant to reference is getRemoteKeysForTarget(). * Making sure we don't export 'experimental' (#1023) * Add a schema migration that drops the query cache (#1019) * Add a schema migration that drops the query cache This is a force fix for potential existence filter mismatches caused by firebase/firebase-ios-sdk#1548 The essential problem is that when resuming a query, the server is allowed to forget deletes. If the number of incorrectly synthesized deletes matches the number of server-forgotten deletes then the existence filter can give a false positive, preventing the cache from self healing. Dropping the query cache clears any client-side resume token which prevents a false positive existence filter mismatch. Note that the remote document cache and mutation queues are unaffected so any cached documents that do exist will still work while offline. * Implement review feedback * Add a release note for the fix to #1548 (#1024) * Ensure that we create an empty TargetGlobal row. (#1029) Ensure the v3 migration unconditionally creates the TargetGlobal row. Remove the no-longer-necessary v2 schema migration. * Remove unnecessary `any` (#1030) * Fix an errant any usage * [AUTOMATED]: Prettier Code Styling * Publish [email protected] * Unify local.QueryData with the other platforms (#1027) This makes it line up with it's own docs, and also the other platforms. * Fix to #1027 to allow SnapshotVersion == 0 (#1033) * Add iat to fake access token payload (#1022) * Add iat to fake access token payload * [AUTOMATED]: Prettier Code Styling * Simpler tests * [AUTOMATED]: Prettier Code Styling * Do not clobber iat * catch server error RESET_PASSWORD_EXCEED_LIMIT (#1037) * Merging Master into Multi-Tab (#1038)

ryanpbrewster added 2 commits July 3, 2018 15:38

Embed metadata directly into the RPC call

98f7c0f

[AUTOMATED]: Prettier Code Styling

c1fe2a1

ryanpbrewster requested a review from mikelehen July 3, 2018 22:42

ryanpbrewster requested review from schmidt-sebastian and wilhuff as code owners July 3, 2018 22:42

google-oss-bot added the needs-triage label Jul 3, 2018

ryanpbrewster commented Jul 3, 2018

View reviewed changes

mikelehen reviewed Jul 3, 2018

View reviewed changes

ryanpbrewster added 3 commits July 3, 2018 16:06

Use ...args

6d7bf23

[AUTOMATED]: Prettier Code Styling

014696e

Minimize diff

63957d2

ryanpbrewster assigned wilhuff Jul 3, 2018

Add the OAuth assertion back in

c6cddf5

ryanpbrewster commented Jul 3, 2018

View reviewed changes

wilhuff reviewed Jul 4, 2018

View reviewed changes

wilhuff approved these changes Jul 10, 2018

View reviewed changes

Merge branch 'master' into rpb/direct-grpc-metadata

224ff71

ryanpbrewster merged commit 81cd260 into master Jul 10, 2018

ryanpbrewster deleted the rpb/direct-grpc-metadata branch July 10, 2018 17:35

firebase locked and limited conversation to collaborators Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embed metadata directly into the RPC call #979

Embed metadata directly into the RPC call #979

ryanpbrewster commented Jul 3, 2018

ryanpbrewster Jul 3, 2018

mikelehen left a comment

mikelehen Jul 3, 2018

ryanpbrewster Jul 3, 2018

mikelehen Jul 3, 2018

ryanpbrewster Jul 3, 2018

ryanpbrewster Jul 3, 2018

wilhuff left a comment

ryanpbrewster commented Jul 9, 2018

mikelehen commented Jul 10, 2018

mikelehen commented Jul 10, 2018

wilhuff commented Jul 10, 2018

wilhuff left a comment

Embed metadata directly into the RPC call #979

Embed metadata directly into the RPC call #979

Conversation

ryanpbrewster commented Jul 3, 2018

ryanpbrewster Jul 3, 2018

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

mikelehen Jul 3, 2018

Choose a reason for hiding this comment

ryanpbrewster Jul 3, 2018

Choose a reason for hiding this comment

mikelehen Jul 3, 2018

Choose a reason for hiding this comment

ryanpbrewster Jul 3, 2018

Choose a reason for hiding this comment

ryanpbrewster Jul 3, 2018

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

ryanpbrewster commented Jul 9, 2018

mikelehen commented Jul 10, 2018

mikelehen commented Jul 10, 2018

wilhuff commented Jul 10, 2018

wilhuff left a comment

Choose a reason for hiding this comment