mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
fix(perms): own the server-scoped rule in HasServerPerm and fail closed on override-fetch errors (D13)
Closes audit finding A-2026-07-16, two defects in the same rule: - permissions.HasServerPerm (admin bypass OR all-of bit test) replaces the hand-rolled copies in api.RequirePermission (whose raw test was any-of for multi-bit masks) and ModerationService.requireBanPermission. RequirePermission's doc comment now states the scope contract: role bitfield only, channel overrides deliberately not consulted. - PermissionService.getOrPopulate and ChannelService.ListVisibleChannels no longer substitute an empty override map when GetAllChannelPermissionsForRole errors. That silently dropped every channel-level deny — and the permission cache then served the degraded snapshot for permCacheTTL (30s) across ~25 callers. Both fail closed now; admins skip the fetch entirely (they bypass channel checks). - PermissionService.HasChannelPerm delegates to Checker.HasChannelPermBatch and MessageService.GetAccessibleChannelIDs to VisibleChannelIDs — the missed fifth D9 site, making that closure true rather than aspirational. - AuthMiddleware rejects a dangling role_id (GetRoleByID returns nil, nil) with 401 instead of putting a nil role in the request context. Locked by failing-first tests: override-fetch-error denies (cached and uncached paths), admin-outage skip, multi-bit all-of, channel allow override must not grant a server-wide route, 403 locks on both RequirePermission routes, dangling-role 401. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -10,11 +10,12 @@ import (
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/config"
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/permissions"
|
||||
)
|
||||
|
||||
// setupDiagnosticsRouter creates a full router with an authenticated user for
|
||||
// diagnostics testing.
|
||||
func setupDiagnosticsRouter(t *testing.T) (http.Handler, string) {
|
||||
func setupDiagnosticsRouter(t *testing.T) (http.Handler, string, *db.DB) {
|
||||
t.Helper()
|
||||
|
||||
database, err := db.Open(":memory:")
|
||||
@@ -46,11 +47,11 @@ func setupDiagnosticsRouter(t *testing.T) (http.Handler, string) {
|
||||
uid, hash,
|
||||
)
|
||||
|
||||
return handler, token
|
||||
return handler, token, database
|
||||
}
|
||||
|
||||
func TestDiagnosticsConnectivity_ReturnsData(t *testing.T) {
|
||||
router, token := setupDiagnosticsRouter(t)
|
||||
router, token, _ := setupDiagnosticsRouter(t)
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/api/v1/diagnostics/connectivity", nil)
|
||||
req.Header.Set("Authorization", "Bearer "+token)
|
||||
@@ -82,7 +83,7 @@ func TestDiagnosticsConnectivity_ReturnsData(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestDiagnosticsConnectivity_Unauthenticated(t *testing.T) {
|
||||
router, _ := setupDiagnosticsRouter(t)
|
||||
router, _, _ := setupDiagnosticsRouter(t)
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/api/v1/diagnostics/connectivity", nil)
|
||||
req.RemoteAddr = "127.0.0.1:9999"
|
||||
@@ -94,6 +95,37 @@ func TestDiagnosticsConnectivity_Unauthenticated(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestDiagnosticsConnectivity_MemberForbidden locks the RequirePermission gate
|
||||
// on the route. Without it, only 200-for-owner and 401-unauthenticated were
|
||||
// covered, so deleting the ADMINISTRATOR gate broke no test while exposing the
|
||||
// server's network topology to every member.
|
||||
func TestDiagnosticsConnectivity_MemberForbidden(t *testing.T) {
|
||||
router, _, database := setupDiagnosticsRouter(t)
|
||||
|
||||
uid, err := database.CreateUser("diagmember", "$2a$12$fake", int(permissions.MemberRoleID))
|
||||
if err != nil {
|
||||
t.Fatalf("CreateUser: %v", err)
|
||||
}
|
||||
token := "diagtest-member-token"
|
||||
if _, err := database.Exec(
|
||||
`INSERT INTO sessions (user_id, token, device, ip_address, expires_at)
|
||||
VALUES (?, ?, 'test', '127.0.0.1', '2099-01-01T00:00:00Z')`,
|
||||
uid, auth.HashToken(token),
|
||||
); err != nil {
|
||||
t.Fatalf("insert session: %v", err)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/api/v1/diagnostics/connectivity", nil)
|
||||
req.Header.Set("Authorization", "Bearer "+token)
|
||||
req.RemoteAddr = "127.0.0.1:9999"
|
||||
rr := httptest.NewRecorder()
|
||||
router.ServeHTTP(rr, req)
|
||||
|
||||
if rr.Code != http.StatusForbidden {
|
||||
t.Errorf("status = %d, want 403; body: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// ─── isPrivateIP tests ──────────────────────────────────────────────────────
|
||||
|
||||
func TestIsPrivateIP(t *testing.T) {
|
||||
|
||||
@@ -10,6 +10,7 @@ import (
|
||||
"github.com/owncord/server/api"
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/permissions"
|
||||
"github.com/owncord/server/service"
|
||||
)
|
||||
|
||||
@@ -89,6 +90,40 @@ func TestCreateInvite_MemberForbidden(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestCreateInvite_ChannelAllowOverrideDoesNotGrant pins the scope boundary of
|
||||
// RequirePermission: it gates on SERVER-WIDE bits, so a per-channel allow must
|
||||
// never open it. The state is reachable — the admin channel-permission handler
|
||||
// masks override input with permissions.AllPerms, which includes ManageInvites.
|
||||
// This kills the plausible-looking "just route RequirePermission through
|
||||
// Checker.HasChannelPerm" refactor, which would pass naive review because
|
||||
// GetChannelPermissions returns (0, 0, nil) when no override row exists.
|
||||
func TestCreateInvite_ChannelAllowOverrideDoesNotGrant(t *testing.T) {
|
||||
database := newAuthTestDB(t)
|
||||
limiter := auth.NewRateLimiter()
|
||||
router := buildInviteRouter(database, limiter)
|
||||
|
||||
token := loginAndGetToken(t, router, database, "overrideuser", 4)
|
||||
|
||||
if _, err := database.Exec(
|
||||
`INSERT INTO channels (id, name, type) VALUES (1, 'general', 'text')`); err != nil {
|
||||
t.Fatalf("insert channel: %v", err)
|
||||
}
|
||||
if _, err := database.Exec(
|
||||
`INSERT INTO channel_overrides (channel_id, role_id, allow, deny) VALUES (1, 4, ?, 0)`,
|
||||
permissions.ManageInvites,
|
||||
); err != nil {
|
||||
t.Fatalf("insert channel override: %v", err)
|
||||
}
|
||||
|
||||
rr := postJSONWithToken(t, router, "/api/v1/invites", token, map[string]any{
|
||||
"max_uses": 1,
|
||||
})
|
||||
|
||||
if rr.Code != http.StatusForbidden {
|
||||
t.Errorf("CreateInvite with channel allow override status = %d, want 403", rr.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCreateInvite_Unlimited(t *testing.T) {
|
||||
database := newAuthTestDB(t)
|
||||
limiter := auth.NewRateLimiter()
|
||||
|
||||
+19
-11
@@ -84,8 +84,11 @@ func AuthMiddleware(database *db.DB) func(http.Handler) http.Handler {
|
||||
}
|
||||
|
||||
// Load role for permission checks.
|
||||
// A dangling role_id returns (nil, nil) from GetRoleByID, so the nil
|
||||
// check is load-bearing: without it a nil role reaches the context
|
||||
// and every downstream permission check has to re-guard it.
|
||||
role, err := database.GetRoleByID(user.RoleID)
|
||||
if err != nil {
|
||||
if err != nil || role == nil {
|
||||
writeJSON(w, http.StatusUnauthorized, errorResponse{
|
||||
Error: "UNAUTHORIZED",
|
||||
Message: "role not found",
|
||||
@@ -106,9 +109,20 @@ func AuthMiddleware(database *db.DB) func(http.Handler) http.Handler {
|
||||
}
|
||||
}
|
||||
|
||||
// RequirePermission returns middleware that checks the authenticated user's
|
||||
// role permissions. Returns 403 if the user lacks the required permission.
|
||||
// The ADMINISTRATOR bit (0x40000000) bypasses all checks.
|
||||
// RequirePermission returns middleware gating a route on SERVER-WIDE role
|
||||
// permissions. Returns 403 if the user lacks them.
|
||||
//
|
||||
// Scope contract — this is the whole reason the middleware and the service
|
||||
// layer look like two permission systems:
|
||||
// - It consults the role bitfield only. Channel overrides are NOT applied,
|
||||
// because a route reaching this middleware has no channel id to resolve
|
||||
// them against, and a per-channel allow must never open a server-wide gate.
|
||||
// - Anything channel-scoped belongs in the service layer behind
|
||||
// permissions.Checker (via svc.Permissions), which resolves overrides.
|
||||
// - ADMINISTRATOR bypasses; multi-bit masks require ALL bits.
|
||||
//
|
||||
// The rule itself lives in permissions.HasServerPerm so no call site can
|
||||
// re-derive it.
|
||||
func RequirePermission(perm int64) func(http.Handler) http.Handler {
|
||||
return func(next http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
@@ -121,13 +135,7 @@ func RequirePermission(perm int64) func(http.Handler) http.Handler {
|
||||
return
|
||||
}
|
||||
|
||||
// ADMINISTRATOR bypasses all permission checks.
|
||||
if permissions.HasAdmin(role.Permissions) {
|
||||
next.ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
|
||||
if role.Permissions&perm == 0 {
|
||||
if !permissions.HasServerPerm(role.Permissions, perm) {
|
||||
writeJSON(w, http.StatusForbidden, errorResponse{
|
||||
Error: "FORBIDDEN",
|
||||
Message: "insufficient permissions",
|
||||
|
||||
@@ -14,6 +14,7 @@ import (
|
||||
"github.com/owncord/server/api"
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/permissions"
|
||||
)
|
||||
|
||||
// ─── Helpers ─────────────────────────────────────────────────────────────────
|
||||
@@ -143,6 +144,51 @@ func TestAuthMiddleware_MalformedAuthHeader(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestAuthMiddleware_DanglingRoleUnauthorized pins the `role == nil` guard:
|
||||
// GetRoleByID returns (nil, nil) for a role_id with no roles row, so without
|
||||
// the guard a nil role reached the request context and the request only died
|
||||
// later, at RequirePermission's own nil check (403) — or not at all on routes
|
||||
// that have no RequirePermission.
|
||||
func TestAuthMiddleware_DanglingRoleUnauthorized(t *testing.T) {
|
||||
database := newAPITestDB(t)
|
||||
|
||||
// users.role_id has a FK to roles(id), so the dangling row can only be
|
||||
// created with FK enforcement momentarily off (db.Open pins the pool to a
|
||||
// single connection, so the pragma applies to the inserts that follow).
|
||||
if _, err := database.Exec(`PRAGMA foreign_keys=OFF`); err != nil {
|
||||
t.Fatalf("disable foreign keys: %v", err)
|
||||
}
|
||||
res, err := database.Exec(
|
||||
`INSERT INTO users (username, password, role_id) VALUES ('dangling', '$2a$12$fake', 999)`)
|
||||
if err != nil {
|
||||
t.Fatalf("insert dangling user: %v", err)
|
||||
}
|
||||
uid, _ := res.LastInsertId()
|
||||
if _, err := database.Exec(`PRAGMA foreign_keys=ON`); err != nil {
|
||||
t.Fatalf("re-enable foreign keys: %v", err)
|
||||
}
|
||||
|
||||
token, _ := auth.GenerateToken()
|
||||
if _, err := database.Exec(
|
||||
`INSERT INTO sessions (user_id, token, device, ip_address, expires_at)
|
||||
VALUES (?, ?, 'test', '127.0.0.1', '2099-01-01T00:00:00Z')`,
|
||||
uid, auth.HashToken(token),
|
||||
); err != nil {
|
||||
t.Fatalf("insert session: %v", err)
|
||||
}
|
||||
|
||||
h := api.AuthMiddleware(database)(http.HandlerFunc(ok))
|
||||
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
||||
withBearer(req, token)
|
||||
rr := httptest.NewRecorder()
|
||||
|
||||
h.ServeHTTP(rr, req)
|
||||
|
||||
if rr.Code != http.StatusUnauthorized {
|
||||
t.Errorf("AuthMiddleware dangling role status = %d, want 401", rr.Code)
|
||||
}
|
||||
}
|
||||
|
||||
// ─── RequirePermission tests ──────────────────────────────────────────────────
|
||||
|
||||
func TestRequirePermission_Allowed(t *testing.T) {
|
||||
@@ -152,9 +198,8 @@ func TestRequirePermission_Allowed(t *testing.T) {
|
||||
hash := auth.HashToken(token)
|
||||
_, _ = database.CreateSession(uid, hash, "test", "127.0.0.1")
|
||||
|
||||
// SEND_MESSAGES = 0x1 — Member role has this bit
|
||||
h := api.AuthMiddleware(database)(
|
||||
api.RequirePermission(0x1)(http.HandlerFunc(ok)),
|
||||
api.RequirePermission(permissions.SendMessages)(http.HandlerFunc(ok)),
|
||||
)
|
||||
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
||||
withBearer(req, token)
|
||||
@@ -174,9 +219,8 @@ func TestRequirePermission_Forbidden(t *testing.T) {
|
||||
hash := auth.HashToken(token)
|
||||
_, _ = database.CreateSession(uid, hash, "test", "127.0.0.1")
|
||||
|
||||
// MANAGE_ROLES = 0x1000000 — Member does not have this
|
||||
h := api.AuthMiddleware(database)(
|
||||
api.RequirePermission(0x1000000)(http.HandlerFunc(ok)),
|
||||
api.RequirePermission(permissions.ManageRoles)(http.HandlerFunc(ok)),
|
||||
)
|
||||
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
||||
withBearer(req, token)
|
||||
@@ -199,7 +243,7 @@ func TestRequirePermission_Administrator_Bypass(t *testing.T) {
|
||||
|
||||
// Any permission should pass for ADMINISTRATOR
|
||||
h := api.AuthMiddleware(database)(
|
||||
api.RequirePermission(0x1000000)(http.HandlerFunc(ok)),
|
||||
api.RequirePermission(permissions.ManageRoles)(http.HandlerFunc(ok)),
|
||||
)
|
||||
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
||||
withBearer(req, token)
|
||||
@@ -212,6 +256,31 @@ func TestRequirePermission_Administrator_Bypass(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestRequirePermission_MultiBitRequiresAllBits pins the one behaviour the
|
||||
// HasServerPerm consolidation changed: a multi-bit mask is ALL-of, not any-of.
|
||||
// The previous raw `role.Permissions&perm == 0` test returned 200 here because
|
||||
// Member holds SendMessages, which was enough to make the mask non-zero.
|
||||
func TestRequirePermission_MultiBitRequiresAllBits(t *testing.T) {
|
||||
database := newAPITestDB(t)
|
||||
uid, _ := database.CreateUser("multibit", "hash", 4) // Member role = 1635, has SendMessages, not ManageRoles
|
||||
token, _ := auth.GenerateToken()
|
||||
hash := auth.HashToken(token)
|
||||
_, _ = database.CreateSession(uid, hash, "test", "127.0.0.1")
|
||||
|
||||
h := api.AuthMiddleware(database)(
|
||||
api.RequirePermission(permissions.SendMessages | permissions.ManageRoles)(http.HandlerFunc(ok)),
|
||||
)
|
||||
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
||||
withBearer(req, token)
|
||||
rr := httptest.NewRecorder()
|
||||
|
||||
h.ServeHTTP(rr, req)
|
||||
|
||||
if rr.Code != http.StatusForbidden {
|
||||
t.Errorf("RequirePermission partial multi-bit mask status = %d, want 403", rr.Code)
|
||||
}
|
||||
}
|
||||
|
||||
// ─── RateLimitMiddleware tests ────────────────────────────────────────────────
|
||||
|
||||
func TestRateLimitMiddleware_UnderLimit(t *testing.T) {
|
||||
@@ -940,6 +1009,15 @@ CREATE TABLE IF NOT EXISTS channels (
|
||||
voice_max_video INTEGER NOT NULL DEFAULT 0
|
||||
);
|
||||
|
||||
CREATE TABLE IF NOT EXISTS channel_overrides (
|
||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
channel_id INTEGER NOT NULL REFERENCES channels(id) ON DELETE CASCADE,
|
||||
role_id INTEGER NOT NULL REFERENCES roles(id) ON DELETE CASCADE,
|
||||
allow INTEGER NOT NULL DEFAULT 0,
|
||||
deny INTEGER NOT NULL DEFAULT 0,
|
||||
UNIQUE(channel_id, role_id)
|
||||
);
|
||||
|
||||
CREATE TABLE IF NOT EXISTS messages (
|
||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
channel_id INTEGER NOT NULL REFERENCES channels(id) ON DELETE CASCADE,
|
||||
|
||||
@@ -71,6 +71,14 @@ func HasAdmin(rolePerms int64) bool {
|
||||
return rolePerms&Administrator != 0
|
||||
}
|
||||
|
||||
// HasServerPerm reports whether a role holds a SERVER-WIDE permission.
|
||||
// Administrator bypasses. Channel overrides are deliberately NOT consulted —
|
||||
// use Checker.HasChannelPerm/HasChannelPermBatch whenever a channel id exists.
|
||||
// Multi-bit masks are ALL-of (every bit must be present), matching HasPerm.
|
||||
func HasServerPerm(rolePerms, perm int64) bool {
|
||||
return HasAdmin(rolePerms) || HasPerm(rolePerms, perm)
|
||||
}
|
||||
|
||||
// EffectivePerms computes the resolved permission set for a channel override.
|
||||
// The formula matches Discord's channel override semantics:
|
||||
//
|
||||
|
||||
@@ -172,6 +172,38 @@ func TestHasAdmin_OwnerRolePermsHasBit(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// ─── HasServerPerm tests ──────────────────────────────────────────────────────
|
||||
|
||||
// TestHasServerPerm locks the contract api.RequirePermission inherits: admin
|
||||
// bypass, and ALL-of semantics for multi-bit masks (a raw `perms&mask != 0`
|
||||
// test would make them any-of).
|
||||
func TestHasServerPerm(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
rolePerms int64
|
||||
perm int64
|
||||
want bool
|
||||
}{
|
||||
{"admin bypasses a bit it lacks", permissions.Administrator, permissions.ManageInvites, true},
|
||||
// Deliberately UNLIKE HasPerm(Administrator, 0) == false: the admin
|
||||
// bypass short-circuits before the zero-mask guard.
|
||||
{"admin with zero mask", permissions.Administrator, 0, true},
|
||||
{"exact bit held", permissions.ManageInvites, permissions.ManageInvites, true},
|
||||
{"bit not held", permissions.SendMessages, permissions.ManageInvites, false},
|
||||
{"non-admin with zero mask", permissions.SendMessages, 0, false},
|
||||
{"multi-bit mask partially held", permissions.SendMessages, permissions.SendMessages | permissions.ManageRoles, false},
|
||||
{"multi-bit mask fully held", permissions.SendMessages | permissions.ManageRoles, permissions.SendMessages | permissions.ManageRoles, true},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
if got := permissions.HasServerPerm(tt.rolePerms, tt.perm); got != tt.want {
|
||||
t.Errorf("HasServerPerm() = %v, want %v", got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// ─── EffectivePerms tests ─────────────────────────────────────────────────────
|
||||
|
||||
// EffectivePerms(rolePerm, allow, deny) = (rolePerm & ^deny) | allow
|
||||
|
||||
@@ -57,7 +57,9 @@ func (s *ChannelService) ListVisibleChannels(ctx context.Context, userID int64)
|
||||
if !permissions.HasAdmin(role.Permissions) {
|
||||
overrides, err = s.st.GetAllChannelPermissionsForRole(role.ID)
|
||||
if err != nil {
|
||||
overrides = make(map[int64]db.ChannelOverride)
|
||||
// Fail closed — an empty map would return every denied channel.
|
||||
slog.Error("ChannelService.ListVisibleChannels GetAllChannelPermissionsForRole", "err", err, "user_id", userID, "role_id", role.ID)
|
||||
return nil, fmt.Errorf("%w: failed to fetch channel overrides", ErrInternal)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
package service
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"testing"
|
||||
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/permissions"
|
||||
)
|
||||
|
||||
// TestListVisibleChannels_OverrideFetchErrorFailsClosed is the uncached half of
|
||||
// the same fail-open bug as TestHasChannelPerm_OverrideFetchErrorDenies: an
|
||||
// empty override map here would list every channel the role is explicitly
|
||||
// denied. The listing must error instead of leaking the denied channel.
|
||||
func TestListVisibleChannels_OverrideFetchErrorFailsClosed(t *testing.T) {
|
||||
database := newTestDB(t)
|
||||
seedRole(t, database, &db.Role{
|
||||
ID: permissions.MemberRoleID,
|
||||
Name: "member",
|
||||
Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.AddReactions,
|
||||
Position: 1,
|
||||
})
|
||||
seedUserRole(t, database, 1, permissions.MemberRoleID)
|
||||
seedChannel(t, database, &db.Channel{ID: 10, Name: "secret", Type: "text"})
|
||||
seedChannelOverride(t, database, permissions.MemberRoleID, 10, 0, permissions.ReadMessages)
|
||||
|
||||
st := errOverrideStore{DB: database}
|
||||
permSvc := NewPermissionService(st, permissions.NewChecker(database))
|
||||
svc := NewChannelService(st, permSvc)
|
||||
|
||||
// Either failing path is acceptable and both are ErrInternal: the permission
|
||||
// cache may short-circuit on its own fail-closed nil, or ListVisibleChannels'
|
||||
// own override branch may error. What must never happen is a 200 listing.
|
||||
got, err := svc.ListVisibleChannels(context.Background(), 1)
|
||||
if !errors.Is(err, ErrInternal) {
|
||||
t.Fatalf("ListVisibleChannels err = %v, want ErrInternal", err)
|
||||
}
|
||||
if got != nil {
|
||||
t.Fatalf("ListVisibleChannels returned %d channels on override fetch failure, want none", len(got))
|
||||
}
|
||||
}
|
||||
@@ -636,31 +636,21 @@ func (s *MessageService) GetAccessibleChannelIDs(userID int64) ([]int64, error)
|
||||
return nil, fmt.Errorf("%w: failed to get role", ErrInternal)
|
||||
}
|
||||
|
||||
isAdmin := permissions.HasAdmin(role.Permissions)
|
||||
var overrides map[int64]db.ChannelOverride
|
||||
if !isAdmin {
|
||||
if !permissions.HasAdmin(role.Permissions) {
|
||||
var overrideErr error
|
||||
overrides, overrideErr = s.st.GetAllChannelPermissionsForRole(role.ID)
|
||||
if overrideErr != nil {
|
||||
return nil, fmt.Errorf("%w: failed to fetch channel overrides", ErrInternal)
|
||||
}
|
||||
if overrides == nil {
|
||||
overrides = make(map[int64]db.ChannelOverride)
|
||||
}
|
||||
}
|
||||
|
||||
// Single visibility predicate shared with REST ListVisibleChannels and the
|
||||
// ws ready payload, so no site can drift.
|
||||
visibleIDs := s.perms.Checker().VisibleChannelIDs(role.Permissions, channelRefs(channels), permOverrides(overrides))
|
||||
var ids []int64
|
||||
for i := range channels {
|
||||
if channels[i].Type == "dm" {
|
||||
continue
|
||||
}
|
||||
if isAdmin {
|
||||
ids = append(ids, channels[i].ID)
|
||||
continue
|
||||
}
|
||||
o := overrides[channels[i].ID]
|
||||
effective := permissions.EffectivePerms(role.Permissions, o.Allow, o.Deny)
|
||||
if effective&permissions.ReadMessages == permissions.ReadMessages {
|
||||
if visibleIDs[channels[i].ID] {
|
||||
ids = append(ids, channels[i].ID)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -35,8 +35,7 @@ func (s *ModerationService) requireBanPermission(actorID int64) error {
|
||||
if err != nil || actorRole == nil {
|
||||
return fmt.Errorf("%w: failed to load actor role", ErrForbidden)
|
||||
}
|
||||
if !permissions.HasAdmin(actorRole.Permissions) &&
|
||||
!permissions.HasPerm(actorRole.Permissions, permissions.BanMembers) {
|
||||
if !permissions.HasServerPerm(actorRole.Permissions, permissions.BanMembers) {
|
||||
return fmt.Errorf("%w: missing BAN_MEMBERS permission", ErrForbidden)
|
||||
}
|
||||
return nil
|
||||
|
||||
@@ -2,6 +2,7 @@ package service
|
||||
|
||||
import (
|
||||
"context"
|
||||
"log/slog"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
@@ -14,7 +15,7 @@ import (
|
||||
type cachedPerms struct {
|
||||
roleID int64
|
||||
rolePerms int64
|
||||
overrides map[int64]db.ChannelOverride
|
||||
overrides map[int64]permissions.ChannelOverride
|
||||
populatedAt time.Time
|
||||
}
|
||||
|
||||
@@ -62,12 +63,7 @@ func (s *PermissionService) HasChannelPerm(userID, channelID, perm int64) bool {
|
||||
if cp == nil {
|
||||
return false
|
||||
}
|
||||
if permissions.HasAdmin(cp.rolePerms) {
|
||||
return true
|
||||
}
|
||||
o := cp.overrides[channelID] // zero-value (0,0) when no override exists
|
||||
effective := permissions.EffectivePerms(cp.rolePerms, o.Allow, o.Deny)
|
||||
return effective&perm == perm
|
||||
return s.checker.HasChannelPermBatch(cp.rolePerms, cp.overrides, channelID, perm)
|
||||
}
|
||||
|
||||
// RequireChannelAccess checks whether the user can access the channel with
|
||||
@@ -150,10 +146,18 @@ func (s *PermissionService) getOrPopulate(userID int64) *cachedPerms {
|
||||
if err != nil || role == nil {
|
||||
return nil
|
||||
}
|
||||
overrides, err := s.st.GetAllChannelPermissionsForRole(role.ID)
|
||||
if err != nil {
|
||||
// Fall back to uncached if override fetch fails.
|
||||
overrides = make(map[int64]db.ChannelOverride)
|
||||
// Admins bypass every channel check, so skip the fetch entirely (mirrors
|
||||
// ChannelService.ListVisibleChannels and ws.buildReady).
|
||||
var overrides map[int64]permissions.ChannelOverride
|
||||
if !permissions.HasAdmin(role.Permissions) {
|
||||
raw, oErr := s.st.GetAllChannelPermissionsForRole(role.ID)
|
||||
if oErr != nil {
|
||||
// Fail closed: an empty map would silently drop every deny bit,
|
||||
// and caching it would keep doing so for permCacheTTL.
|
||||
slog.Error("PermissionService.getOrPopulate override fetch failed, denying", "err", oErr, "user_id", userID, "role_id", role.ID)
|
||||
return nil
|
||||
}
|
||||
overrides = permOverrides(raw)
|
||||
}
|
||||
|
||||
cp = &cachedPerms{
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package service
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
@@ -8,6 +9,41 @@ import (
|
||||
"github.com/owncord/server/permissions"
|
||||
)
|
||||
|
||||
// errOverrideStore wraps a real *db.DB but always fails the channel-override
|
||||
// fetch, so the fail-closed contract (A-2026-07-16) is testable. Embedding
|
||||
// *db.DB satisfies the service Store interface; only the one overridden method
|
||||
// diverges, every other call still hits the real database.
|
||||
type errOverrideStore struct {
|
||||
*db.DB
|
||||
}
|
||||
|
||||
func (errOverrideStore) GetAllChannelPermissionsForRole(int64) (map[int64]db.ChannelOverride, error) {
|
||||
return nil, errors.New("boom")
|
||||
}
|
||||
|
||||
// TestHasChannelPerm_OverrideFetchErrorDenies locks the fail-closed rule: when
|
||||
// the override fetch errors we must NOT substitute an empty map, because that
|
||||
// restores every bit a channel-level deny had stripped — and PermissionService
|
||||
// would then cache that degraded snapshot for permCacheTTL.
|
||||
func TestHasChannelPerm_OverrideFetchErrorDenies(t *testing.T) {
|
||||
database := newTestDB(t)
|
||||
seedRole(t, database, &db.Role{
|
||||
ID: permissions.MemberRoleID,
|
||||
Name: "member",
|
||||
Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.AddReactions,
|
||||
Position: 1,
|
||||
})
|
||||
seedUserRole(t, database, 1, permissions.MemberRoleID)
|
||||
seedChannel(t, database, &db.Channel{ID: 10, Name: "readonly", Type: "text"})
|
||||
seedChannelOverride(t, database, permissions.MemberRoleID, 10, 0, permissions.ReadMessages)
|
||||
|
||||
svc := NewPermissionService(errOverrideStore{DB: database}, permissions.NewChecker(database))
|
||||
|
||||
if svc.HasChannelPerm(1, 10, permissions.ReadMessages) {
|
||||
t.Fatal("override fetch failure must deny, not fall back to the base role bits")
|
||||
}
|
||||
}
|
||||
|
||||
// newTestPermService creates a PermissionService backed by a real in-memory DB
|
||||
// pre-populated with a single role and user.
|
||||
func newTestPermService(t *testing.T) (*PermissionService, *db.DB) {
|
||||
@@ -100,6 +136,28 @@ func TestHasChannelPerm_AdminBypass(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestHasChannelPerm_AdminSkipsOverrideFetch locks the admin skip in
|
||||
// getOrPopulate: fail-closed must not extend to admins, who bypass every
|
||||
// channel check anyway. Without the skip, an override-fetch outage would
|
||||
// deny admins everything instead of nothing.
|
||||
func TestHasChannelPerm_AdminSkipsOverrideFetch(t *testing.T) {
|
||||
database := newTestDB(t)
|
||||
seedRole(t, database, &db.Role{
|
||||
ID: permissions.AdminRoleID,
|
||||
Name: "admin",
|
||||
Permissions: permissions.Administrator,
|
||||
Position: 90,
|
||||
})
|
||||
seedUserRole(t, database, 1, permissions.AdminRoleID)
|
||||
seedChannel(t, database, &db.Channel{ID: 10, Name: "general", Type: "text"})
|
||||
|
||||
svc := NewPermissionService(errOverrideStore{DB: database}, permissions.NewChecker(database))
|
||||
|
||||
if !svc.HasChannelPerm(1, 10, permissions.ManageMessages) {
|
||||
t.Fatal("admin must not be denied by an override-fetch outage; the fetch is skipped for admins")
|
||||
}
|
||||
}
|
||||
|
||||
func TestInvalidateUser_ClearsCacheForUser(t *testing.T) {
|
||||
svc, database := newTestPermService(t)
|
||||
seedChannel(t, database, &db.Channel{ID: 10, Name: "general", Type: "text"})
|
||||
|
||||
Reference in New Issue
Block a user