feat: Export GenerateJWT method to public #97
Conversation
|
chenchang.cc seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughReplaced a private JWT helper with a new public API: Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant JWTOAuthClient
participant Signer
Caller->>JWTOAuthClient: GetAccessToken(ctx)
JWTOAuthClient->>JWTOAuthClient: build GenerateJWTReq{TTL, SessionName, SessionContext}
JWTOAuthClient->>JWTOAuthClient: GenerateJWT(ctx, opts)
JWTOAuthClient->>Signer: create claims (iss,aud,iat,exp,jti[,session_*]) + header(kid,alg)
Signer-->>JWTOAuthClient: signed JWT string
JWTOAuthClient-->>Caller: return JWT
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
auth.go (2)
676-676: Add GoDoc for the newly exported method to satisfy lint and improve discoverabilityExported identifiers should have a comment starting with the identifier’s name. This also helps consumers understand the method’s purpose.
Apply:
-func (c *JWTOAuthClient) GenerateJWT(ttl int, sessionName *string, sessionContext *SessionContext) (string, error) { +// GenerateJWT builds and signs an RS256 JWT for the OAuth JWT-bearer grant. +// ttl is in seconds; sessionName and sessionContext are optional claims. +func (c *JWTOAuthClient) GenerateJWT(ttl int, sessionName *string, sessionContext *SessionContext) (string, error) {Optionally, for a smoother migration in case any in-package references remain, add a private shim (non-diff snippet shown since it’s outside this hunk):
// Deprecated: use GenerateJWT instead. Will be removed in a future release. func (c *JWTOAuthClient) generateJWT(ttl int, sessionName *string, sessionContext *SessionContext) (string, error) { return c.GenerateJWT(ttl, sessionName, sessionContext) }If you want, I can open a follow-up PR to add the doc comment and deprecation shim.
703-707: Nit: setting "alg" header is redundant; jwt library sets it from the signing method
jwt.NewWithClaims(jwt.SigningMethodRS256, ...)already setsalgto RS256. Manually setting it increases the chance of drift if the signing method ever changes.Apply:
token.Header["kid"] = c.publicKey token.Header["typ"] = "JWT" -token.Header["alg"] = "RS256"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
auth.go(2 hunks)
🔇 Additional comments (1)
auth.go (1)
656-656: GenerateJWT call-site updated — no remaining references to old privategenerateJWTQuick verification: searching the repo found no occurrences of
generateJWT(and shows the public usage/definition only.
- auth.go:656 — call site:
jwtCode, err := c.GenerateJWT(ttl, opts.SessionName, opts.SessionContext)- auth.go:676 — definition:
func (c *JWTOAuthClient) GenerateJWT(...)
haicoder
force-pushed
the
feat/gererate_jwt
branch
from
c75a608 to
cd1bd14
Compare
chyroc
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
auth.go (1)
688-731: Fix: avoid nil deref, prevent zero-expiry tokens, and resolve unused ctx compile error.
- Public
GenerateJWTdereferencesoptswithout a nil check, which will panic if callers passnil.- Using
opts.TTLdirectly allows 0 or negative TTL, yielding tokens that are immediately expired.- The
ctxparameter is unused, which is a compile-time error in Go.Apply this minimal, backward-compatible fix:
func (c *JWTOAuthClient) GenerateJWT(ctx context.Context, opts *GenerateJWTReq) (string, error) { - now := time.Now() + // Reserved for future tracing/logging; avoid unused param compile error. + _ = ctx + now := time.Now() jti, err := generateRandomString(16) if err != nil { return "", err } - ttl := opts.TTL - sessionName := opts.SessionName - sessionContext := opts.SessionContext + // Derive effective TTL and optional session info, with sensible defaults. + ttl := c.ttl + var sessionName *string + var sessionContext *SessionContext + if opts != nil { + if opts.TTL > 0 { + ttl = opts.TTL + } + sessionName = opts.SessionName + sessionContext = opts.SessionContext + } // Build claims claims := jwt.MapClaims{ "iss": c.clientID, "aud": c.hostName, "iat": now.Unix(), "exp": now.Add(time.Duration(ttl) * time.Second).Unix(), "jti": jti, }Optional follow-up: consider returning an error for invalid TTL values instead of silently falling back, if you prefer stricter input validation.
🧹 Nitpick comments (1)
auth.go (1)
681-687: Clarify TTL semantics in the new public request type.Since callers may omit or pass non-positive TTLs, document that such values fall back to the client default. This avoids confusion and better communicates intended behavior.
type GenerateJWTReq struct { - TTL int `json:"ttl,omitempty"` // Token validity period (in seconds) + TTL int `json:"ttl,omitempty"` // Token validity period in seconds; <= 0 uses client default SessionName *string `json:"session_name,omitempty"` // Session name SessionContext *SessionContext `json:"session_context,omitempty"` // SessionContext }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
auth.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test success
🔇 Additional comments (1)
auth.go (1)
656-662: Good refactor: centralize JWT generation via exported API.Calling the new exported
GenerateJWTwith a clearly constructedGenerateJWTReqkeepsGetAccessTokenlean and improves reuse. No issues here.
3 participants
Summary by CodeRabbit