authd-oidc-brokers/broker: fix cache consolidation on content mismatch

[adombeck]

consolidateKnownCacheFiles() refused to merge the legacy username-keyed cache directory into the provider ID-keyed directory whenever a known file existed in both locations with differing content. For password and token.json, however, differing content is the normal case once both directories have been used for login: password is hashed with a fresh random salt on every write, and token.json is rewritten on every online login.

As a result, the merge failed permanently, the username directory was never replaced with a compatibility symlink, and subsequent logins could authenticate against whichever stale copy the session happened to select.

Since there is no reliable way to determine which copy is authoritative, keep whichever file has the most recent modification time and discard the other, assuming it reflects the most recent successful authentication. This allows consolidation to complete instead of leaving the cache permanently split.

Fixes #1787
UDENG-11233

[Copilot]

Pull request overview

Fixes cache consolidation when username- and provider ID-keyed credentials differ.

Changes:

• Selects the cache directory with the newest file mtime.
• Adds consolidation tests for differing and unexpected files.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
authd-oidc-brokers/internal/broker/broker.go	Revises credential-cache consolidation.
authd-oidc-brokers/internal/broker/broker_test.go	Tests mtime-based consolidation outcomes.

Suppressed comments (1)

authd-oidc-brokers/internal/broker/broker.go:490

• This validation now accepts every regular filename, although the PR and consolidateKnownCacheFiles are scoped to password and token.json. Consequently an unrelated file can determine which credential directory wins, be moved into the provider cache, or be silently deleted with the losing source directory; the new tests explicitly demonstrate that data loss. Restore the known-name check for both directories and reject consolidation without modifying either directory when any other entry exists.

		if !info.Mode().IsRegular() {
			return time.Time{}, fmt.Errorf("unexpected non-regular cache entry %q", entry.Name())
		}
		if info.ModTime().After(newest) {

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

authd-oidc-brokers/internal/broker/broker.go:495

• This validation now accepts every regular filename, so an unrelated or misspelled file can determine which directory wins and is then either promoted into the authoritative cache or silently deleted with the losing directory. The consolidation contract only recognizes password and token.json; reject all other names before considering mtimes, and change the new unexpected-entry cases to expect the cache to remain untouched.

		if !info.Mode().IsRegular() {
			return time.Time{}, fmt.Errorf("unexpected non-regular cache entry %q", entry.Name())
		}

authd-oidc-brokers/internal/broker/broker.go:481

• If this cleanup fails, the directory swap has already committed and sourceDir no longer exists, but returning the error makes the caller keep the session pointed at that old source path. finishAuth then recreates the username directory when caching the token, splitting the password and token again. Treat staging cleanup as post-commit best effort (log the failure) and return success so the caller creates the compatibility symlink and adopts targetDir.

	return os.RemoveAll(stagingDir)

authd-oidc-brokers/internal/broker/broker.go:474

• This fixed staging path has no crash recovery. A process exit after either rename (or a cleanup failure below) can leave a non-empty .staging directory, causing the next source-newer attempt to fail at os.Rename(targetDir, stagingDir) and leave the cache permanently split again. It can also collide with a valid provider-ID directory whose ID ends in .staging; use a unique sibling staging path and recover or clean abandoned staging directories.

This issue also appears in the following locations of the same file:

• line 481
• line 493

	stagingDir := targetDir + ".staging"
	if err := os.Rename(targetDir, stagingDir); err != nil {
		return err

[codecov]

Codecov Report

❌ Patch coverage is 70.58824% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.29%. Comparing base (48be1e6) to head (f80fa5c).
⚠️ Report is 37 commits behind head on main.

Files with missing lines	Patch %	Lines
authd-oidc-brokers/internal/broker/broker.go	70.58%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1789      +/-   ##
==========================================
- Coverage   88.19%   85.29%   -2.91%     
==========================================
  Files          96       26      -70     
  Lines        7092     1945    -5147     
  Branches      112        0     -112     
==========================================
- Hits         6255     1659    -4596     
+ Misses        781      286     -495     
+ Partials       56        0      -56     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nooreldeenmansour]

LGTM
But consider double-checking the Copilot suppressed comments if you haven't already done so. They sometimes have nice suggestions and are easy to miss. I looked at them and I think the second-suggestion is relevant

[adombeck]

consider double-checking the Copilot #1789 (review) if you haven't already done so. They sometimes have nice suggestions and are easy to miss. I looked at them and I think the second-suggestion is relevant

thanks! I agree that that suggestion is valid and pushed a commit to address it