Add solution for Challenge 15 by nzamulov #644
Merged
Conversation
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
WalkthroughA new Go file introduces a complete in-memory OAuth2 authorization server implementation with client registration, PKCE support, authorization code flow, token exchange, token validation, and revocation capabilities with thread-safe state management. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthServer as Authorization Server
participant ResourceServer as Resource Server
rect rgb(200, 220, 255)
note over Client,AuthServer: Authorization Code Flow (with PKCE)
Client->>AuthServer: GET /authorize?client_id=...&code_challenge=...
AuthServer->>AuthServer: Validate client & scopes
AuthServer-->>Client: Redirect with authorization_code & state
end
rect rgb(220, 200, 255)
note over Client,AuthServer: Token Exchange
Client->>AuthServer: POST /token<br/>(grant_type=authorization_code,<br/>code_verifier=...)
AuthServer->>AuthServer: Verify code_challenge & credentials
AuthServer-->>Client: {access_token, refresh_token, expires_in}
end
rect rgb(200, 255, 220)
note over Client,ResourceServer: Resource Access
Client->>ResourceServer: GET /resource<br/>Authorization: Bearer access_token
ResourceServer->>AuthServer: ValidateToken(access_token)
AuthServer-->>ResourceServer: {ClientID, UserID, Scopes, ExpiresAt}
ResourceServer-->>Client: Protected Resource
end
rect rgb(255, 220, 200)
note over Client,AuthServer: Token Revocation
Client->>AuthServer: POST /revoke<br/>(token=..., is_refresh_token=bool)
AuthServer->>AuthServer: Remove token from storage
AuthServer-->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 8
🧹 Nitpick comments (5)
challenge-15/submissions/nzamulov/solution-template.go (5)
276-284: Reduce lock scope in HandleToken to avoid holding write lock across I/O and CPU work.You lock at Line 276 and defer until function return, spanning random generation, JSON encoding, and network writes. Narrow to only map reads/writes using RLock/Lock blocks.
Example pattern:
// read-only s.mu.RLock() client, found := s.clients[clientID] s.mu.RUnlock() // later mutations s.mu.Lock() s.tokens[accessToken] = aToken s.refreshTokens[refreshToken] = rToken s.mu.Unlock()Also applies to: 285-289
139-154: GenerateRandomString: compute minimal byte length deterministically; remove top-up path.Cleaner and avoids over-reading from crypto/rand.
- // Over-generate bytes to ensure encoded output >= length, then trim. - byteLen := length // good enough with base64 expansion - buf := make([]byte, byteLen) + // Compute minimal bytes so base64 (no padding) yields >= length chars. + // ceil(length * 3 / 4) + byteLen := (length*3 + 3) / 4 + buf := make([]byte, byteLen) if _, err := rand.Read(buf); err != nil { return "", err } s := base64.RawURLEncoding.EncodeToString(buf) - if len(s) < length { - // Extremely unlikely; top up once. - extra := make([]byte, length) - if _, err := rand.Read(extra); err != nil { - return "", err - } - s += base64.RawURLEncoding.EncodeToString(extra) - } return s[:length], nil
110-116: Plaintext password in seeded user (ok for demo, risky beyond).If this code is ever reused beyond tests, store password hashes (bcrypt/scrypt/argon2). No change needed if strictly for challenge.
421-440: Token revocation should be indistinguishable (RFC 7009).If exposed via an endpoint, returning not found leaks token validity. Consider always returning success.
95-98: Standardize OAuth2 error responses and include WWW-Authenticate for 401.WriteError is good; for 401 from token endpoint include WWW-Authenticate: Bearer error="invalid_client|invalid_grant".
Also applies to: 459-472
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-15/submissions/nzamulov/solution-template.go(1 hunks)
🔇 Additional comments (1)
challenge-15/submissions/nzamulov/solution-template.go (1)
1-481: Overall: solid in-memory OAuth2/PKCE skeleton.Clear data model, consistent mutex usage, and helpful helpers. With the fixes above, this will be robust and spec-aligned.
Please run the token and PKCE e2e tests once you apply the diffs to confirm redirects and public client flows still pass.
Comment on lines
+163
to
+169
| for _, param := range authorizeParams { | ||
| if val := q.Get(param); len(val) == 0 { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Consistent error body for missing authorize params.
Return invalid_request with which param is missing.
- if val := q.Get(param); len(val) == 0 {
- w.WriteHeader(http.StatusBadRequest)
- return
- }
+ if val := q.Get(param); len(val) == 0 {
+ WriteError(w, http.StatusBadRequest, "invalid_request", fmt.Sprintf("missing %s", param))
+ return
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In challenge-15/submissions/nzamulov/solution-template.go around lines 163 to
169, the handler currently returns a bare 400 when authorize params are missing;
change it to respond with a consistent OAuth2 error JSON body indicating
"error":"invalid_request" and an "error_description" naming the specific missing
param. For each missing param set Content-Type to application/json, write
http.StatusBadRequest and a JSON object like
{"error":"invalid_request","error_description":"missing <param>"} (ensure you
replace <param> with the actual param name) and then return.
Comment on lines
+189
to
+191
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
Return structured error for redirect_uri not registered.
Avoid bare 400; use invalid_request for clarity.
- w.WriteHeader(http.StatusBadRequest)
- return
+ WriteError(w, http.StatusBadRequest, "invalid_request", "redirect_uri not registered for client")
+ returnCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In challenge-15/submissions/nzamulov/solution-template.go around lines 189 to
191, the handler returns a bare HTTP 400 when the redirect_uri is not
registered; replace this with a proper OAuth2-style structured error response.
Instead of w.WriteHeader(http.StatusBadRequest) and return, write a JSON body
with {"error":"invalid_request","error_description":"redirect_uri not
registered"} (set Content-Type: application/json) and send an appropriate status
(400), ensuring the response is encoded and written before returning.
Comment on lines
+240
to
+242
| w.Header().Set("Location", fmt.Sprintf("%s?code=%s&state=%s", redirectURI, codeStr, q.Get("state"))) | ||
| w.WriteHeader(http.StatusFound) | ||
| } |
There was a problem hiding this comment.
Fix redirects: URL-encode params and preserve existing query strings.
fmt.Sprintf builds broken URLs when redirect_uri already has query/fragment and fails to encode state. Use net/url to merge/encode.
@@
- if q.Get("response_type") != "code" {
- w.Header().Set("Location", fmt.Sprintf("%s?error=%s&state=%s", redirectURI, "unsupported_response_type", q.Get("state")))
- w.WriteHeader(http.StatusFound)
- return
- }
+ if q.Get("response_type") != "code" {
+ u, err := url.Parse(redirectURI)
+ if err != nil {
+ WriteError(w, http.StatusBadRequest, "invalid_request", "invalid redirect_uri")
+ return
+ }
+ qs := u.Query()
+ qs.Set("error", "unsupported_response_type")
+ if st := q.Get("state"); st != "" {
+ qs.Set("state", st)
+ }
+ u.RawQuery = qs.Encode()
+ w.Header().Set("Location", u.String())
+ w.WriteHeader(http.StatusFound)
+ return
+ }
@@
- w.Header().Set("Location", fmt.Sprintf("%s?code=%s&state=%s", redirectURI, codeStr, q.Get("state")))
- w.WriteHeader(http.StatusFound)
+ u, err := url.Parse(redirectURI)
+ if err != nil {
+ WriteError(w, http.StatusBadRequest, "invalid_request", "invalid redirect_uri")
+ return
+ }
+ qs := u.Query()
+ qs.Set("code", codeStr)
+ if st := q.Get("state"); st != "" {
+ qs.Set("state", st)
+ }
+ u.RawQuery = qs.Encode()
+ w.Header().Set("Location", u.String())
+ w.WriteHeader(http.StatusFound)Add import:
@@
-import (
+import (
"crypto/rand"
"fmt"
+ "net/url"Also applies to: 207-211
🤖 Prompt for AI Agents
In challenge-15/submissions/nzamulov/solution-template.go around lines 240-242
(and also apply the same change to lines 207-211): the redirect builds the URL
with fmt.Sprintf and doesn't URL-encode parameters or preserve existing
query/fragment in redirect_uri. Replace the fmt.Sprintf approach by parsing
redirectURI with net/url, merge the existing query parameters with the code and
state params using url.Values (ensuring both code and state are url-encoded),
set the parsed URL's RawQuery to the encoded values, and use url.String() for
the Location header; also add the required net/url import.
Comment on lines
+244
to
+246
| var accessTokenParams = []string{"grant_type", "code", "redirect_uri", "client_id", "client_secret"} | ||
| var refreshTokenParams = []string{"grant_type", "refresh_token", "client_id", "client_secret"} | ||
|
|
There was a problem hiding this comment.
Don’t require client_secret for public (PKCE) clients.
Make client_secret optional; require it only for confidential clients.
-var accessTokenParams = []string{"grant_type", "code", "redirect_uri", "client_id", "client_secret"}
-var refreshTokenParams = []string{"grant_type", "refresh_token", "client_id", "client_secret"}
+var accessTokenParams = []string{"grant_type", "code", "redirect_uri", "client_id"}
+var refreshTokenParams = []string{"grant_type", "refresh_token", "client_id"}Client auth check (already at Lines 285-289) will still enforce secret when configured. No behavior change for confidential clients; unblocks public PKCE.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var accessTokenParams = []string{"grant_type", "code", "redirect_uri", "client_id", "client_secret"} | |
| var refreshTokenParams = []string{"grant_type", "refresh_token", "client_id", "client_secret"} | |
| var accessTokenParams = []string{"grant_type", "code", "redirect_uri", "client_id"} | |
| var refreshTokenParams = []string{"grant_type", "refresh_token", "client_id"} |
🤖 Prompt for AI Agents
In challenge-15/submissions/nzamulov/solution-template.go around lines 244 to
246, the arrays listing required token request parameters currently include
"client_secret", which wrongly forces public (PKCE) clients to send a secret;
remove "client_secret" from both accessTokenParams and refreshTokenParams so
client_secret is not universally required, and add a short comment noting that
client_secret is optional for public clients and confidential clients are still
enforced by the existing client auth check at lines ~285-289.
Comment on lines
+251
to
+256
| func (s *OAuth2Server) HandleToken(w http.ResponseWriter, r *http.Request) { | ||
| if err := r.ParseForm(); err != nil { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Enforce POST and form content-type on token endpoint.
Spec-compliant and prevents CSRF via GET.
func (s *OAuth2Server) HandleToken(w http.ResponseWriter, r *http.Request) {
+ if r.Method != http.MethodPost {
+ w.Header().Set("Allow", http.MethodPost)
+ WriteError(w, http.StatusMethodNotAllowed, "invalid_request", "POST required")
+ return
+ }
+ if ct := r.Header.Get("Content-Type"); !strings.HasPrefix(ct, "application/x-www-form-urlencoded") {
+ WriteError(w, http.StatusBadRequest, "invalid_request", "Content-Type must be application/x-www-form-urlencoded")
+ return
+ }
if err := r.ParseForm(); err != nil {
w.WriteHeader(http.StatusBadRequest)
return
}🤖 Prompt for AI Agents
In challenge-15/submissions/nzamulov/solution-template.go around lines 251 to
256, the token endpoint accepts any HTTP method and content-type; update the
handler to enforce HTTP POST and require Content-Type
application/x-www-form-urlencoded: if r.Method != "POST" respond with 405 Method
Not Allowed and set header "Allow: POST"; check r.Header.Get("Content-Type")
starts with "application/x-www-form-urlencoded" (or return 415 Unsupported Media
Type); only then call r.ParseForm() and proceed with the existing logic.
Comment on lines
+293
to
+307
| if grantType == refreshTokenGrantType { | ||
| refreshTokenStr := r.FormValue("refresh_token") | ||
| refreshToken, found := s.refreshTokens[refreshTokenStr] | ||
| if !found { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| tokenOrCode.ClientID = refreshToken.ClientID | ||
| tokenOrCode.UserID = refreshToken.UserID | ||
| tokenOrCode.Scopes = refreshToken.Scopes | ||
| tokenOrCode.ExpiresAt = refreshToken.ExpiresAt | ||
|
|
||
| delete(s.refreshTokens, refreshTokenStr) | ||
| } |
There was a problem hiding this comment.
Reorder refresh-token rotation to avoid stranding clients on mid-flight errors.
Deleting the old refresh token before minting/storing the new pair can leave clients without a usable token if generation fails. Delete after successful issuance.
@@
- refreshTokenStr := r.FormValue("refresh_token")
- refreshToken, found := s.refreshTokens[refreshTokenStr]
+ refreshTokenStr := r.FormValue("refresh_token")
+ refreshToken, found := s.refreshTokens[refreshTokenStr]
if !found {
w.WriteHeader(http.StatusBadRequest)
return
}
@@
- delete(s.refreshTokens, refreshTokenStr)
+ // defer deletion of old refresh token until new pair is stored
+ oldRefresh := refreshTokenStrAnd after storing new tokens:
@@
- s.tokens[accessToken] = aToken
- s.refreshTokens[refreshToken] = rToken
+ s.tokens[accessToken] = aToken
+ s.refreshTokens[refreshToken] = rToken
+ if grantType == refreshTokenGrantType {
+ delete(s.refreshTokens, oldRefresh)
+ }Also applies to: 383-385
🤖 Prompt for AI Agents
In challenge-15/submissions/nzamulov/solution-template.go around lines 293 to
307 (and also review lines 383-385), the refresh-token rotation currently
deletes the old refresh token before successfully minting/storing the new token
pair; change the flow to defer deleting the old refresh token until after the
new access/refresh tokens have been created and persisted: first generate and
store the new tokens (ensure any storage writes succeed), then delete the old
refresh token from s.refreshTokens; apply the same reorder at the other
occurrence on lines 383-385 so deletion happens only after successful issuance
and storage.
Comment on lines
+327
to
+337
| codeVerifierStr := r.FormValue("code_verifier") | ||
| if authCode.CodeChallenge != "" { | ||
| if codeVerifierStr == "" { | ||
| WriteError(w, http.StatusBadRequest, "invalid_grant", "code_verifier is required for PKCE") | ||
| return | ||
| } | ||
| if !VerifyCodeChallenge(codeVerifierStr, authCode.CodeChallenge, authCode.CodeChallengeMethod) { | ||
| WriteError(w, http.StatusBadRequest, "invalid_grant", "code_verifier does not match code_challenge") | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate PKCE code_verifier length and charset per RFC 7636.
Enforce 43–128 chars and allowed characters before hashing.
- codeVerifierStr := r.FormValue("code_verifier")
+ codeVerifierStr := r.FormValue("code_verifier")
if authCode.CodeChallenge != "" {
if codeVerifierStr == "" {
WriteError(w, http.StatusBadRequest, "invalid_grant", "code_verifier is required for PKCE")
return
}
+ if !validPKCEVerifier(codeVerifierStr) {
+ WriteError(w, http.StatusBadRequest, "invalid_grant", "invalid code_verifier format")
+ return
+ }
if !VerifyCodeChallenge(codeVerifierStr, authCode.CodeChallenge, authCode.CodeChallengeMethod) {
WriteError(w, http.StatusBadRequest, "invalid_grant", "code_verifier does not match code_challenge")
return
}
}Add helper:
// place near VerifyCodeChallenge
func validPKCEVerifier(v string) bool {
if len(v) < 43 || len(v) > 128 {
return false
}
for i := 0; i < len(v); i++ {
c := v[i]
if (c >= 'A' && c <= 'Z') ||
(c >= 'a' && c <= 'z') ||
(c >= '0' && c <= '9') ||
c == '-' || c == '.' || c == '_' || c == '~' {
continue
}
return false
}
return true
}🤖 Prompt for AI Agents
In challenge-15/submissions/nzamulov/solution-template.go around lines 327 to
337, the PKCE code_verifier is accepted without validating length and allowed
characters; add a helper function validPKCEVerifier(v string) (place it near
VerifyCodeChallenge) that enforces 43–128 characters and only ALPHA / DIGIT /
"-" / "." / "_" / "~" per RFC 7636, and call it before VerifyCodeChallenge; if
validPKCEVerifier returns false respond with WriteError(w,
http.StatusBadRequest, "invalid_request", "code_verifier must be 43-128
characters and contain only A-Z a-z 0-9 - . _ ~") and return, otherwise proceed
to VerifyCodeChallenge as before.
Comment on lines
+352
to
+355
| if tokenOrCode.ExpiresAt.Before(time.Now()) { | ||
| w.WriteHeader(http.StatusUnauthorized) | ||
| return | ||
| } |
There was a problem hiding this comment.
Return OAuth2 JSON error for expired/invalid code or refresh token.
Bare 401 lacks error body; use invalid_grant per RFC 6749.
- w.WriteHeader(http.StatusUnauthorized)
+ WriteError(w, http.StatusUnauthorized, "invalid_grant", "authorization code or refresh token expired")
returnCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In challenge-15/submissions/nzamulov/solution-template.go around lines 352 to
355, the handler currently returns a bare 401 with no body when a token/code is
expired; replace that with an OAuth2-compliant JSON error response using
error="invalid_grant" (and a short error_description), set Content-Type:
application/json, and write the JSON body while returning the appropriate status
(use 400 Bad Request per RFC 6749 token endpoint error for invalid_grant).
Ensure the response is written instead of only setting the header so clients
receive the error payload.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Challenge 15 Solution
Submitted by: @nzamulov
Challenge: Challenge 15
Description
This PR contains my solution for Challenge 15.
Changes
challenge-15/submissions/nzamulov/solution-template.goTesting
Thank you for reviewing my submission! 🚀