fix: send exactly one accessJwt header per request - #295
Open
mlwelles wants to merge 1 commit into
Open
Conversation
anyClient() attached the access token to the stub it returned, and every call site attached it again before sending, so each authenticated request carried a duplicate accessJwt header. On a request retried after a JWT refresh the two values differed: the retried call presented both the refreshed token and the expired one. That kept working only by accident. grpc-java merges the outermost interceptor's headers first, so the wire order is refreshed-then-expired, and Dgraph reads accessJwt[0] (x/x.go). grpc-java's own Metadata.get returns the last value, so a Java server reading the same request would have honored the expired token. The invariant rested on an undocumented coupling between grpc-java's merge order, Dgraph's indexing, and no intermediary rewriting duplicate headers. Have anyClient() return the raw stub. Every consumer already wraps it with getStubWithJwt at send time -- the seven callable bodies, and AsyncTransaction at all three of its RPC sites -- which is what lets a retry pick up a refreshed token in the first place. The only consumers that used the stub directly are login and retryLogin, which should not present the token they replace. Add AccessJwtHeaderTest, which asserts what a server actually receives. It runs against an in-process fake Dgraph and needs no cluster. All three cases fail before this change; the retry case reports [access-2, access-1] where exactly one value is expected.
Contributor
Author
|
Note on ordering relative to #294 ("fix: make DgraphAsyncClient non-blocking to avoid ForkJoinPool.commonPool starvation"), which is open against the same file:
I found this while reviewing #294 and deliberately kept it out of that PR — mixing a credential-propagation change into a threading change would make both harder to review and neither revertable on its own. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
anyClient()attached the access token to the stub it returned, and then every call site attached it again before sending. Every authenticated request therefore carried a duplicateaccessJwtheader. On a request retried after a JWT refresh the two values differed, and the retried call presented both the refreshed token and the expired one.This kept working only by accident:
ClientInterceptors.interceptmakes the last-added interceptor outermost, andHeaderAttachingClientCall.startmerges its own headers before delegating inward. The wire order is therefore refreshed-then-expired.x/x.go:accessJwt := md.Get("accessJwt")…return accessJwt[0], nil.grpc-java's own
Metadata.getis documented to return the last value for a key, so a Java server reading the identical request would have honored the expired token. The invariant rested on an undocumented coupling between grpc-java's merge order, Dgraph's indexing, and no intermediary rewriting duplicate headers. Any one of those changing would break auth on the retry path, surfacing as a token-expired loop rather than a clean error.The change
anyClient()now returns the raw stub. Every consumer already wraps it withgetStubWithJwtat send time, which is what lets a retry pick up a refreshed token in the first place:DgraphAsyncClient(alter,checkVersion,runDQL,allocateIDs,createNamespace,dropNamespace,listNamespaces).AsyncTransaction, at all three of its RPC sites (doRequest,commit,discard). It never uses the bare stub.The only consumers that used the stub directly are
loginandretryLogin, which should not present the token they are replacing. They no longer do.Tests
AccessJwtHeaderTestasserts what a server actually receives, using an in-process fake Dgraph and a recordingServerInterceptor. It needs no cluster. All three cases fail before this change:authenticatedRequestCarriesExactlyOneAccessJwt[access-1, access-1]expiryRetryPresentsOnlyTheRefreshedToken[access-2, access-1]on the retried callloginCarriesNoAccessJwtThe JWT-expiry retry had no coverage at all before this, which is why the duplicate survived.
build.gradledeclares anintegrationTestsource set pointing atsrc/integration-test/java, which does not exist, so these live insrc/testalongside the rest.No public API changes;
anyClient()is private.Checklist
CHANGELOG.mdfile describing and linking tothis PR
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.