feat(cli): improve auth#3466
Open
alkalescent wants to merge 7 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OAuth2 refresh-token support (exchange, endpoint resolution, TLS no-verify, client-id fallback), expiry/presence predicates, related errors, comprehensive refresh tests, and updates Cobra command Use-string generation with tests. ChangesAccess Token Refresh with Expiration Helpers
Command Documentation and Usage String Formatting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdfctl/pkg/auth/refresh_test.go`:
- Around line 111-115: The tests TestRefreshAccessTokenWrongAuthType and
TestRefreshAccessTokenEndpointError currently assert only require.Error; change
them to assert the specific expected sentinel errors (use require.ErrorIs or
require.ErrorContains). Update TestRefreshAccessTokenWrongAuthType to assert
ErrorIs(err, ErrInvalidAuthType) and update TestRefreshAccessTokenEndpointError
to assert the appropriate sentinel (or stable substring) for endpoint failures
to avoid false positives; ensure RefreshAccessToken in refresh.go wraps/returns
ErrInvalidAuthType (from errors.go) on the wrong-auth-type path so the test can
reliably match it.
- Around line 143-149: The test for RefreshAccessToken currently asserts updated
AccessToken.AccessToken and RefreshToken but omits expiry checks; update the
success-case in refresh_test.go after calling profile.GetAuthCredentials() to
also assert that creds.AccessToken.Expiration is in the future (e.g., compare to
time.Now().Unix() or time.Now()) so the refreshed token's expiration was
persisted (reference symbols: RefreshAccessToken, profile.GetAuthCredentials,
creds.AccessToken.Expiration).
In `@otdfctl/pkg/auth/refresh.go`:
- Around line 96-104: IsTokenExpired currently checks only access-token profiles
and uses time.Unix(...).After(...), which can be confusing when reused; update
it to either (A) keep the name but add/extend the godoc on IsTokenExpired to
explicitly state "returns false for non–access-token auth types because refresh
only applies to access tokens", and change the expiry check to use the
AccessToken.Expiry.Before(time.Now()) or token.Valid() semantics for clarity
(referencing profile.GetAuthCredentials(), profiles.AuthTypeAccessToken, and
creds.AccessToken), or (B) rename the function to ShouldRefresh and adjust the
semantics to reflect a refresh decision using
creds.AccessToken.Expiry.Before(time.Now())/token.Valid(); pick one approach and
update all call sites and comments accordingly so the behavior is unambiguous.
- Around line 59-74: The current logic constructs oldToken and then calls
oauth2Config.TokenSource(ctx, oldToken).Token(), but because oldToken.Valid()
can be true this will return the same token and skip an actual refresh; to force
a refresh clear oldToken.AccessToken (set to empty) and set oldToken.Expiry to a
past time (e.g., zero or time.Now().Add(-time.Hour)) before creating tokenSource
so the Token() call will use the RefreshToken to request a new access token;
keep the existing RefreshToken and the rest of the TLS/http client handling
intact (referencing oldToken, oauth2Config.TokenSource and tokenSource.Token()).
In `@otdfctl/pkg/man/man.go`:
- Around line 248-249: The current command construction sets Args to
cobra.ExactArgs when c.Args is present but then overwrites it with
cobra.ArbitraryArgs when c.ArbitraryArgs is also present, losing required-arg
enforcement; modify the logic in the place that builds the cobra command (where
Use: buildUseString(c.Name, c.Args, c.ArbitraryArgs) and Args: args are set) so
that when both c.Args and c.ArbitraryArgs are populated you set args to
cobra.MinimumNArgs(len(c.Args)) instead of replacing it with
cobra.ArbitraryArgs, leaving cobra.ExactArgs only for the pure-required case and
cobra.ArbitraryArgs only for the pure-arbitrary case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49ab52f8-9d6d-4611-9c7e-1ad5c616b507
📒 Files selected for processing (5)
otdfctl/pkg/auth/errors.gootdfctl/pkg/auth/refresh.gootdfctl/pkg/auth/refresh_test.gootdfctl/pkg/man/man.gootdfctl/pkg/man/man_test.go
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust mechanism for refreshing access tokens within the CLI, ensuring seamless authentication management. Additionally, it enhances the CLI's documentation generation by improving how command usage strings are formatted, providing better guidance for users regarding required and optional arguments. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The token fades, the session dies, We need a refresh, a new surprise. With code and test, the fix is clear, Authentication stays sincere. Footnotes
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements OAuth2 token refresh logic and expiration checks, alongside comprehensive unit tests. It also updates the manual page generation to format command usage strings with proper argument delimiters. The review feedback suggests enhancing the reliability of the new functions by adding nil pointer checks for the profile store and incorporating a safety buffer into the token expiration logic.
Contributor
alkalescent
force-pushed
the
DSPX-2905-auth
branch
from
4ac75b7 to
3163494
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdfctl/pkg/auth/refresh.go`:
- Around line 81-89: The stored Expiration currently uses newToken.Expiry.Unix()
which becomes a large negative number if newToken.Expiry is zero; update the
refresh logic in refresh.go where newCreds is built (the block creating
profiles.AuthCredentials) to check newToken.Expiry.IsZero() and set Expiration
to 0 in that case (otherwise use newToken.Expiry.Unix()). Also update
IsTokenExpired to treat Expiration == 0 as "no expiry / not expired" so a
missing expires_in round-trips correctly and freshly-refreshed tokens are not
treated as expired forever.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ebd5e65-0541-4622-b7c9-b7ff97783142
📒 Files selected for processing (5)
otdfctl/pkg/auth/errors.gootdfctl/pkg/auth/refresh.gootdfctl/pkg/auth/refresh_test.gootdfctl/pkg/man/man.gootdfctl/pkg/man/man_test.go
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
jakedoublev
previously approved these changes
May 13, 2026
jentfoo
reviewed
May 13, 2026
Contributor
jentfoo
left a comment
jentfoo
left a comment
There was a problem hiding this comment.
Generally looks reasonable, just the one minor bug spotted.
alkalescent
force-pushed
the
DSPX-2905-auth
branch
from
0ba950d to
ab9c483
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdfctl/pkg/auth/refresh_test.go`:
- Around line 251-291: The test TestRefreshAccessTokenTLSNoVerify currently uses
httptest.NewServer (HTTP) so it doesn't validate TLS-skip behavior; replace
httptest.NewServer with httptest.NewTLSServer to exercise TLS handshake against
a self-signed cert and keep the resolver function returning tokenServer.URL so
refreshAccessToken(profile, resolver) actually connects over HTTPS; ensure
TLSNoVerify remains true on the ProfileConfig so utils.NewHTTPClient receives
tlsNoVerify=true and the test asserts the refresh succeeds, and consider adding
a separate negative test that uses TLSNoVerify=false to confirm the same
TLSServer fails.
In `@otdfctl/pkg/auth/refresh.go`:
- Around line 77-86: The refresh flow can hang because oauth2 falls back to
http.DefaultClient (no timeout) when tlsNoVerify is false; update
utils.NewHTTPClient to set a sensible Timeout (e.g., 30*time.Second) and then
call utils.NewHTTPClient(tlsNoVerify) unconditionally in refresh.go (remove the
conditional) and set ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient)
before creating oauth2Config.TokenSource(ctx, oldToken) so the token fetch uses
the timed HTTP client.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad66f813-9c3d-4002-9ee3-91a1ac8023c6
📒 Files selected for processing (5)
otdfctl/pkg/auth/errors.gootdfctl/pkg/auth/refresh.gootdfctl/pkg/auth/refresh_test.gootdfctl/pkg/man/man.gootdfctl/pkg/man/man_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdfctl/pkg/auth/refresh_test.go`:
- Around line 129-137: The mock token handlers created via httptest.NewServer
(the http.HandlerFunc assigned to tokenServer and the other similar handlers at
the other locations) currently always return success and must be hardened to
assert the incoming request is a POST and that the expected form fields are
present; inside each handler assert r.Method == "POST", call r.ParseForm(), and
assert the form contains "grant_type" and "refresh_token", and where the test
simulates a client include an assertion for "client_id"; after these assertions
continue encoding the JSON response as before. Ensure you update each handler
instance (the tokenServer handler and the other two similar handlers referenced
in the comment) with the same request validations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34401108-1642-4aee-9510-74c1da1252af
📒 Files selected for processing (1)
otdfctl/pkg/auth/refresh_test.go
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
jakedoublev
previously approved these changes
May 21, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
alkalescent
force-pushed
the
DSPX-2905-auth
branch
from
038b6b7 to
f6e546a
Compare
policy-bot-opentdf
Bot
dismissed
jakedoublev’s stale review
Invalidated by push of f6e546a
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
otdfctl/pkg/man/man.go (1)
239-274:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix verification gaps for
otdfctl/pkg/man/man.go(lines 239-274)
gofumptcan’t be executed here (gofumpt: command not found), so formatting isn’t checkable.golangci-lint run otdfctl/pkg/man/man.gofails typecheck (undefined: DocFlag,undefined: styleDoc), whilego test ./otdfctl/pkg/man/...passes—lint needs to be rerun with a proper package-level invocation so it typechecks with the rest of the package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otdfctl/pkg/man/man.go` around lines 239 - 274, The linter/formatter failures are due to running tools against a single file (which causes undefined symbols like DocFlag and styleDoc) and gofumpt not being installed; rerun the checks at package level and ensure gofumpt is available: install gofumpt (e.g., go install mvdan.cc/gofumpt@latest) and add it to PATH, then run golangci-lint from the package or repo root (e.g., golangci-lint run ./otdfctl/pkg/man or golangci-lint run) so typechecking includes definitions for DocFlag and styleDoc and the formatter runs correctly; after that re-run gofumpt/golangci-lint to verify buildUseString/Doc usage and related symbols pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@otdfctl/pkg/man/man.go`:
- Around line 239-274: The linter/formatter failures are due to running tools
against a single file (which causes undefined symbols like DocFlag and styleDoc)
and gofumpt not being installed; rerun the checks at package level and ensure
gofumpt is available: install gofumpt (e.g., go install mvdan.cc/gofumpt@latest)
and add it to PATH, then run golangci-lint from the package or repo root (e.g.,
golangci-lint run ./otdfctl/pkg/man or golangci-lint run) so typechecking
includes definitions for DocFlag and styleDoc and the formatter runs correctly;
after that re-run gofumpt/golangci-lint to verify buildUseString/Doc usage and
related symbols pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c010a3ad-2306-4ed5-b121-52e19602a933
📒 Files selected for processing (5)
otdfctl/pkg/auth/errors.gootdfctl/pkg/auth/refresh.gootdfctl/pkg/auth/refresh_test.gootdfctl/pkg/man/man.gootdfctl/pkg/man/man_test.go
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
elizabethhealy
approved these changes
May 22, 2026
Contributor
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
4 participants
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.
Proposed Changes
auth client credentialsargument docChecklist
Testing Instructions
Summary by CodeRabbit
New Features
Bug Fixes
Tests