feat(parity): wire API sessions into access control, fix authz level bug#279
Merged
Conversation
Completes the session feature end-to-end and closes more gaps vs the
original etherpad:
- SessionManager: implement the findAuthorID/doesSessionExist stubs.
API-session storage (create/get/delete/list) now lives here; the
lib/api/session endpoints delegate to it. SecurityManager.CheckAccess
therefore honours sessionID cookies: private group pads are reachable
with a valid, unexpired session of the pad's group (quoted and
comma-separated cookies handled like upstream #3819).
- webaccess: NormalizeAuthzLevel dropped "readOnly" and "modify" through
the switch (missing fallthrough in the port) and denied them; `true`
now normalizes to "create" like the original. UserCanModify no longer
panics on a nil PadAuthorizations map - it denies.
- new GET /admin/api/stats endpoint (totalPads, totalSessions,
totalActivePads) mirroring the original getStats API.
- new POST /admin/api/pads/{padId}/sendClientsMessage endpoint backed by
PadMessageHandler.SendCustomMessageToPad, mirroring the original
handleCustomMessage broadcast.
- document the not-yet-ported preAuthorize plugin hook in webaccess
instead of a misleading FIXME.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Code Review by Qodo
1. Session bypasses token validation
|
PR Summary by QodoWire API sessions into access control, fix authz level normalization, add stats/message APIs WalkthroughsDescription• Implement API-session storage/lookup in SessionManager and enforce it in access control. • Fix authz normalization and nil-map handling to prevent incorrect denies and panics. • Add /admin/api/stats and sendClientsMessage endpoints with regression and broadcast tests. Diagramgraph TD
A["Admin HTTP API"] --> B["Session endpoints"] --> C["pad.SessionManager"] --> D[("DataStore KV")]
E["SecurityManager.CheckAccess"] --> C
F["Stats endpoint"] --> G["WS SessionStore"]
H["sendClientsMessage"] --> I["PadMessageHandler"] --> J{{"Pad clients"}}
subgraph Legend
direction LR
_api["API/Service"] ~~~ _db[("Database/KV")]] ~~~ _ext{{"External"}}
end
High-Level AssessmentThe following are alternative approaches to this PR: 1. Keep API-session storage in lib/api/session
2. Use a dedicated DB table for sessions
Recommendation: The chosen approach (centralizing API-session persistence + lookup in pad.SessionManager and having both HTTP endpoints and CheckAccess delegate to it) is the best fit for parity and correctness: it eliminates duplicated logic and ensures sessionID cookies actually affect authorization. A dedicated table could be revisited later if operational needs (cleanup/reporting) outgrow the KV-backed model. File ChangesEnhancement (6)
Bug fix (1)
Refactor (1)
Tests (4)
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+149
to
+159
| sessionIDs := strings.Split(cookieQuoteTrimmer.ReplaceAllString(*sessionCookie, ""), ",") | ||
| now := time.Now().Unix() | ||
| for _, sessionID := range sessionIDs { | ||
| info, err := sm.GetSessionInfo(strings.TrimSpace(sessionID)) | ||
| if err != nil || info == nil { | ||
| continue | ||
| } | ||
| if info.GroupID == groupID && now < info.ValidUntil { | ||
| return &info.AuthorID | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Session bypasses token validation 🐞 Bug ⛨ Security
Once SessionManager.FindAuthorID returns a non-nil author for a valid session cookie, SecurityManager.CheckAccess skips utils.IsValidAuthorToken() and still calls AuthorManager.GetAuthorId() with an unvalidated token. Because GetAuthorId() auto-creates an author for unknown tokens, a valid session can be combined with arbitrary token strings (including empty / non-"t." tokens) to mint authors and bypass token-format enforcement.
Agent Prompt
## Issue description
With real session lookups enabled, `SecurityManager.CheckAccess` now conditionally skips `utils.IsValidAuthorToken()` when `sessionAuthorID != nil`. This allows unvalidated token strings to reach `AuthorManager.GetAuthorId()`, which auto-creates authors for unknown tokens.
## Issue Context
This becomes reachable only after this PR because `SessionManager.FindAuthorID()` now returns non-nil values for valid sessions.
## Fix Focus Areas
- lib/pad/SecurityManager.go[108-130]
- lib/author/authorManager.go[36-56]
- lib/pad/SessionManager.go[140-160]
## Suggested fix
1. In `SecurityManager.CheckAccess`, validate `token` format regardless of `sessionAuthorID` (and guard against `token == nil` before dereferencing).
2. If the intent is to allow session-only auth, avoid calling `AuthorManager.GetAuthorId()` with arbitrary tokens in the session-authenticated path; otherwise, keep token validation mandatory even when a session is present.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+156
to
+158
| if info.GroupID == groupID && now < info.ValidUntil { | ||
| return &info.AuthorID | ||
| } |
There was a problem hiding this comment.
2. Session author not enforced 🐞 Bug ≡ Correctness
SecurityManager.CheckAccess uses the session cookie only as a gate (for private group pad access) but never enforces that the token-derived author matches the session’s stored AuthorID. This breaks the author-binding semantics of API sessions and allows a session cookie for group G to be combined with any other valid author token to access G’s private pads as that other author.
Agent Prompt
## Issue description
API sessions are stored with an `AuthorID`, but `SecurityManager.CheckAccess` does not use `sessionAuthorID` to determine the granted author identity, nor does it verify the token-derived author matches the session author.
## Issue Context
`SessionManager.FindAuthorID` returns the author of the first valid, unexpired session for the pad's group. `CheckAccess` computes this but always sets `GrantedAccess.AuthorId` from `AuthorManager.GetAuthorId(*token)`.
## Fix Focus Areas
- lib/pad/SecurityManager.go[108-149]
- lib/pad/SessionManager.go[26-31]
- lib/pad/SessionManager.go[140-160]
## Suggested fix
Choose one consistent model:
- **Strict binding (recommended for API sessions):** if `sessionAuthorID != nil`, require `retrievedAuthorFromToken.Id == *sessionAuthorID`, otherwise deny.
- **Session-driven identity:** if `sessionAuthorID != nil`, set `GrantedAccess.AuthorId = *sessionAuthorID` (and avoid creating/mapping authors via arbitrary tokens in this path).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Summary
Follow-up to #276 — completes the API-session feature end-to-end and fixes two more porting bugs found along the way:
Session access control (the missing half of the session API)
SessionManager.findAuthorID/doesSessionExistwere stubs (alwaysnil/false), soSecurityManager.CheckAccessnever honoured sessionID cookies. The API-session storage now lives inpad.SessionManager(thelib/api/sessionendpoints delegate to it, same storage keys), andCheckAccessgrants access to private group pads for valid, unexpired sessions of the pad's group. Quoted and comma-separated cookie values are handled like the original (upstream #3819).Porting bugs fixed
NormalizeAuthzLeveldeniedreadOnlyandmodify: the JSswitchfallthrough was ported as empty Go cases, so onlycreatepassed — users with readOnly/modify pad authorizations were rejected.truenow also normalizes to"create"like the original.UserCanModifypanicked on a nilPadAuthorizationsmap (panic("This should not happen")) — now denies instead of crashing the request.New endpoints (original API parity)
GET /admin/api/stats→{totalPads, totalSessions, totalActivePads}(originalgetStats)POST /admin/api/pads/{padId}/sendClientsMessage→ broadcasts a custom COLLABROOM message type to all pad clients (originalsendClientsMessage/handleCustomMessage)Also replaces the misleading
//FIXME this needs to be setinwebaccess.gowith documentation of the not-yet-ported preAuthorize plugin hook.Test plan
go test -p 1 ./...— all 27 packages green (Memory/SQLite/Postgres/MySQL),go vet ./...clean🤖 Generated with Claude Code