Skip to content

fix: send exactly one accessJwt header per request - #295

Open
mlwelles wants to merge 1 commit into
mainfrom
fix/single-access-jwt-header
Open

fix: send exactly one accessJwt header per request#295
mlwelles wants to merge 1 commit into
mainfrom
fix/single-access-jwt-header

Conversation

@mlwelles

@mlwelles mlwelles commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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 duplicate accessJwt header. 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:

  1. grpc-java's ClientInterceptors.intercept makes the last-added interceptor outermost, and HeaderAttachingClientCall.start merges its own headers before delegating inward. The wire order is therefore refreshed-then-expired.
  2. Dgraph reads the first value — x/x.go: accessJwt := md.Get("accessJwt")return accessJwt[0], nil.

grpc-java's own Metadata.get is 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 with getStubWithJwt at send time, which is what lets a retry pick up a refreshed token in the first place:

  • The seven callable bodies in 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 login and retryLogin, which should not present the token they are replacing. They no longer do.

Tests

AccessJwtHeaderTest asserts what a server actually receives, using an in-process fake Dgraph and a recording ServerInterceptor. It needs no cluster. All three cases fail before this change:

Case Before
authenticatedRequestCarriesExactlyOneAccessJwt [access-1, access-1]
expiryRetryPresentsOnlyTheRefreshedToken [access-2, access-1] on the retried call
loginCarriesNoAccessJwt login presented the token it was replacing

The JWT-expiry retry had no coverage at all before this, which is why the duplicate survived. build.gradle declares an integrationTest source set pointing at src/integration-test/java, which does not exist, so these live in src/test alongside the rest.

No public API changes; anyClient() is private.

Checklist

  • Code compiles correctly and linting passes locally
  • For all code changes, an entry added to the CHANGELOG.md file describing and linking to
    this PR
  • Tests added for new functionality, or regression tests for bug fixes added as applicable

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

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.
@mlwelles
mlwelles requested a review from a team as a code owner August 6, 2026 13:43
@mlwelles

mlwelles commented Aug 6, 2026

Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant