Include profile name in OAuth token cache filename hash#1298
Include profile name in OAuth token cache filename hash#1298hectorcast-db merged 3 commits intomainfrom
Conversation
751da6d to
eb51e6c
Compare
simonfaltum
left a comment
There was a problem hiding this comment.
Approving to unblock, mostly smaller nits. Not sure if I am scope-creeping too much
| client_secret=client_secret, | ||
| redirect_url=redirect_url, | ||
| scopes=scopes, | ||
| profile=cfg.profile if cfg.profile else None, |
There was a problem hiding this comment.
nit: this conditional is a no-op because cfg.profile is already Optional[str], and TokenCache handles None fine via self._profile or "". We could simplify to: profile=cfg.profile, but its probably not a big deal - maybe it guards against other mistakes in the future
There was a problem hiding this comment.
also no existing test asserts that external_browser actually passes cfg.profile through to the TokenCache constructor as far as I can see
databricks/sdk/oauth.py
Outdated
| def filename(self) -> str: | ||
| # Include host, client_id, and scopes in the cache filename to make it unique. | ||
| # Include host, client_id, scopes, and profile in the cache filename to make it unique. | ||
| hash = hashlib.sha256() |
There was a problem hiding this comment.
nit: hash shadows the Python builtin. Since you're already touching this property, maybe rename to h or hasher? Pre-existing, not a blocker.
databricks/sdk/oauth.py
Outdated
| for chunk in [ | ||
| self._host, | ||
| self._client_id, | ||
| ",".join(self._scopes), | ||
| self._profile or "", | ||
| ]: |
There was a problem hiding this comment.
I think hashlib.update() is equivalent to concatenating the raw bytes, so there are no delimiters between fields. This means collisions are possible across field boundaries.
I think theoretically scopes=["foo","bar"], profile="baz" collides with scopes=["foo"], profile=",barbaz" because the concatenated bytes are identical.
This is a pre-existing design issue (host/client_id had the same theoretical problem), but adding profile makes it more practical since commas are valid in configparser section names.
I don't know if we want to fix it while we are here but I think the way to do it could be a null-byte separator between fields :
for chunk in [self._host, self._client_id, ",".join(self._scopes), self._profile or ""]:
h.update(chunk.encode("utf-8"))
h.update(b"\x00")
That said, this would change the hash for all existing users (one-time reauth). Up to you whether that's worth it in this PR, a follow-up or leave as-is.
| def test_token_cache_unique_filename_by_profile(): | ||
| common_args = dict( | ||
| host="http://localhost:", | ||
| client_id="abc", | ||
| redirect_url="http://localhost:8020", | ||
| oidc_endpoints=OidcEndpoints("http://localhost:1234", "http://localhost:1234"), | ||
| ) | ||
| assert TokenCache(profile="dev", **common_args).filename != TokenCache(profile="prod", **common_args).filename | ||
|
|
||
|
|
||
| def test_token_cache_filename_no_profile_matches_empty_profile(): | ||
| common_args = dict( | ||
| host="http://localhost:", | ||
| client_id="abc", | ||
| redirect_url="http://localhost:8020", | ||
| oidc_endpoints=OidcEndpoints("http://localhost:1234", "http://localhost:1234"), | ||
| ) | ||
| assert TokenCache(**common_args).filename == TokenCache(profile=None, **common_args).filename |
There was a problem hiding this comment.
The tests look good and follow the existing pattern.
Two suggestions:
-
Consider adding a delimiter-collision test case, e.g. asserting that scopes=["a,b"], profile="c" differs from scopes=["a"], profile="b,c" — this would catch the framing issue above if it's ever fixed.
-
The no_profile_matches_empty_profile test asserts TokenCache(**args).filename == TokenCache(profile=None, **args).filename
This is trivially true since both calls pass None. A more interesting backward-compat test would pin the actual expected hash value, so a future refactor can't accidentally change the no-profile filename.
f142a91 to
9b00d48
Compare
When a config profile is set, include it in the SHA256 hash used to derive the token cache filename. This ensures that different profiles targeting the same host with the same client ID get separate cache entries, preventing credential cross-contamination between profiles. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Simplify profile=cfg.profile (drop no-op conditional) - Rename hash -> h to avoid shadowing Python builtin - Add null-byte field separators to prevent hash collisions across field boundaries - Update changelog: all users must reauthenticate once after upgrading - Add test asserting external_browser passes cfg.profile to TokenCache - Add delimiter-collision test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
9b00d48 to
b4ce377
Compare
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Changes
The
TokenCacheclass computes a SHA256 hash ofhost,client_id, andscopesto derive a unique cache filename. This PR adds the config profile name to that hash.Why
Without this, two profiles that share the same host and client ID would read/write the same cache file and overwrite each other's tokens. Including the profile name in the hash gives each profile its own isolated cache entry.
Impact
Users with a named config profile will get a new cache filename after upgrading, so they will need to reauthenticate once. Users without a named profile (or using the
DEFAULTprofile implicitly) are unaffected —profile=Nonehashes the same as before since it contributes an empty string.Test plan
test_token_cache_unique_filename_by_profile— different profile names produce different filenamestest_token_cache_filename_no_profile_matches_empty_profile— omittingprofileand passingprofile=Noneare equivalent🤖 Generated with Claude Code