docs: document missing features (RBAC, TOTP, magic links, email verification, password reset, maintenance)#34
Conversation
- Fix Go version requirement: 1.21+ → 1.26+ (matches go.mod) - Add maintenance package to packages table and add full section - Add RBAC section (auth.RoleChecker, StoreRoleChecker, CachingRoleChecker, RegisterRolePermissions, RBACUserStore) - Add MagicLinkStore, EmailVerificationStore, TOTPStore, PasswordResetStore to Store interfaces section - Add MagicLinkHandler, EmailVerificationHandler, PasswordResetHandler, and TOTPHandler sections in the handler package - Add auth.GenerateRandomBase64 to crypto utilities - Add full sentinel error reference table (ErrTOTPNotFound, ErrInvalidTOTPCode, ErrEmailNotVerified, ErrSessionRevoked, ErrOIDCSubjectAlreadyLinked) - Add TOTP replay protection, magic link, reset token, and email enumeration entries to Security notes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the README to document several previously implemented but undocumented authentication and maintenance features, and aligns the documented Go version requirement with go.mod.
Changes:
- Expands README coverage for RBAC, TOTP, magic links, email verification, password reset, and the
maintenancepackage. - Adds reference tables for sentinel errors, roles/permissions, and store interfaces.
- Updates the documented Go version requirement to
1.26+.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds/updates documentation for implemented auth flows (RBAC, TOTP, magic links, email verification, password reset) and maintenance cleanup, plus corrects the stated Go version. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
README.md:540
- The route comment for
RequestResetsays “always 200”, but the handler can return 429 when rate limited and 400 on missing/invalid email (and 500 on internal failures). Consider qualifying this as “returns 200 for well-formed requests regardless of whether the email is registered” and optionally calling out the 429 behavior when RateLimiter is set.
POST /password-reset/request → h.RequestReset // always 200; sends token if email is registered
README.md:525
SendVerificationis described as “always returns 200”, but the handler returns 400 for invalid input (e.g., missing email / invalid JSON). Consider qualifying this as “returns 200 for well-formed requests regardless of whether the email is registered” to keep the anti-enumeration guarantee accurate.
- `SendVerification` always returns 200 to avoid leaking account existence.
README.md:507
RequestMagicLinkis documented as “always returns 200”, but the handler returns non-200 responses for invalid input (e.g., missing email / invalid JSON) and for server misconfiguration or failures (e.g., Sender nil, store errors). Consider rephrasing to: it returns 200 for well-formed requests regardless of whether the email is registered (to prevent enumeration).
- `RequestMagicLink` always returns 200 to avoid leaking whether an address is registered.
- Files reviewed: 1/1 changed files
- Comments generated: 1
Resolves conflicts between branch documentation additions and main's improvements: uses main's handler ordering (TOTPHandler before MagicLink/EmailVerification/PasswordReset), adopts main's prose-over-inline-comments style for store interfaces, combines the best content from both branches (SHA-256 notes, Secret field encoding, multi-instance TOTP cache guidance, Password reset security note), and removes the duplicate maintenance section.
Both occurrences used (ctx, userID string) which incorrectly typed ctx as string. Updated to ctx context.Context to match the actual interface.
MagicLinkHandler.Sender, EmailVerificationHandler.SendEmail, and PasswordResetHandler.SendResetEmail snippets used grouped-name shorthand that incorrectly typed ctx as string. Updated to ctx context.Context to match the actual field types.
| | `auth.ErrExpiredToken` | Token has passed its `exp` claim | | ||
| | `auth.ErrEmailExists` | `CreateUser` called with an already-registered email | | ||
| | `auth.ErrEmailNotVerified` | A flow requires a verified email but the account's `EmailVerified` is false | | ||
| | `auth.ErrSessionRevoked` | Middleware finds the JWT `jti` in the store but the session is revoked | |
There was a problem hiding this comment.
ErrSessionRevoked is never returned by the middleware
ErrSessionRevoked is defined in auth/types.go but is never referenced anywhere else in the library. The middleware (auth/middleware.go) only checks for ErrNotFound from the session store; any other error (including ErrSessionRevoked) falls to the else branch and produces an HTTP 500 response. A SessionStore implementor who reads this table and returns ErrSessionRevoked for revoked sessions based on this documentation would receive 500 errors instead of 401s.
The "When returned" description should either reflect the actual behaviour (this error is currently unused by the library, and store implementations should return ErrNotFound for missing/revoked sessions) or the middleware should be updated to explicitly handle ErrSessionRevoked.
Prompt To Fix With AI
This is a comment left during a code review.
Path: README.md
Line: 108
Comment:
**`ErrSessionRevoked` is never returned by the middleware**
`ErrSessionRevoked` is defined in `auth/types.go` but is never referenced anywhere else in the library. The middleware (`auth/middleware.go`) only checks for `ErrNotFound` from the session store; any other error (including `ErrSessionRevoked`) falls to the `else` branch and produces an HTTP 500 response. A `SessionStore` implementor who reads this table and returns `ErrSessionRevoked` for revoked sessions based on this documentation would receive 500 errors instead of 401s.
The "When returned" description should either reflect the actual behaviour (this error is currently unused by the library, and store implementations should return `ErrNotFound` for missing/revoked sessions) or the middleware should be updated to explicitly handle `ErrSessionRevoked`.
How can I resolve this? If you propose a fix, please make it concise.
Summary
The README was missing documentation for several complete, implemented features. This PR fills all identified gaps.
Changes
Corrections
1.21+→1.26+(matchesgo 1.26.1ingo.mod)New sections
maintenancepackage entryauth– Sentinel errorsErrTOTPNotFound,ErrInvalidTOTPCode,ErrEmailNotVerified,ErrSessionRevoked,ErrOIDCSubjectAlreadyLinkedauth– RBACRegisterRolePermissions,StoreRoleChecker,NewCachingRoleChecker, built-in roles/permissions table,RBACUserStoreinterfaceauth– Cryptoauth.GenerateRandomBase64(used by magic links and password reset)auth– Store interfacesMagicLinkStore,EmailVerificationStore,TOTPStore,PasswordResetStorehandlerMagicLinkHandler,EmailVerificationHandler,PasswordResetHandler,TOTPHandler(full struct, routes, and behavior notes for each)maintenanceStartCleanupsection with example and behavioral notesTesting
No code was changed — only documentation. The build is unaffected.
Warning
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Greptile Summary
This PR documents several previously undocumented but fully-implemented features: RBAC, TOTP, magic links, email verification, password reset, and the
maintenancepackage. Prior review feedback (incorrectctxtypes in callback signatures andRBACUserStore) has been addressed.ErrSessionRevokeddocumentation is inaccurate: the table says it is returned by the middleware when a session is revoked, but this sentinel is never referenced anywhere in the library code. The middleware only handlesErrNotFound; returningErrSessionRevokedfrom aSessionStorewould produce HTTP 500 instead of 401.Confidence Score: 4/5
Safe to merge after fixing the ErrSessionRevoked description, which would mislead SessionStore implementors.
One P1 documentation inaccuracy remains: ErrSessionRevoked is documented as returned by the middleware but is never used by the library; a store implementor relying on this table would write incorrect code. All other changes are accurate and prior P1 findings are resolved.
README.md line 108 — ErrSessionRevoked sentinel error description
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Incoming Request] --> B{Has JWT?} B -- Yes --> C[Validate JWT signature & exp] C -- Invalid/Expired --> X1[401 Unauthorized] C -- Valid --> D{Sessions configured?} D -- No --> E[Inject UserID → handler] D -- Yes --> F[FindSessionByID jti] F -- ErrNotFound --> X2[401 session expired or revoked] F -- other error --> X3[500 Internal Server Error] F -- Found --> G{UserID match & not expired?} G -- No --> X2 G -- Yes --> E B -- No API key --> H[ValidateAPIKey hash] H -- Not found --> X1 H -- Found --> EComments Outside Diff (1)
README.md, line 588-624 (link)ctxtype in callback function literalsThree newly-documented callback fields use Go's grouped-name shorthand where
ctxis grouped withstringparameters, making all parametersstring. The actual field types requirecontext.Contextas the first argument, so these literals will not compile if copied verbatim.MagicLinkHandler.Sender):func(ctx, email, token string)— actual type:func(ctx context.Context, email, token string) errorEmailVerificationHandler.SendEmail):func(ctx, to, token string)— actual type:func(ctx context.Context, to, token string) errorPasswordResetHandler.SendResetEmail):func(ctx, toEmail, rawToken string)— actual type:func(ctx context.Context, toEmail, rawToken string) errorThis is the same class of bug as the
RBACUserStoresnippet that was already fixed in this PR (the previous(ctx, userID string)→(ctx context.Context, userID string)correction).Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "docs: fix ctx parameter type in callback..." | Re-trigger Greptile