tenseleyflow/shithub / c9d4831

Browse files

policy: UserActorFromCurrentUser propagates impersonation + admin (SR2 C1/C2)

Adds the canonical web-layer Actor constructor that propagates
IsSuspended, IsSiteAdmin, Impersonating, ImpersonateWriteOK from
the request-bound CurrentUser. Plain UserActor() left intact for
SSH/HTTP-git/worker callers that don't have a CurrentUser handy.

CurrentUser gains PolicyActor() and AuditActor(meta) helpers so
non-admin handlers can build actors and record audit rows with
uniform impersonation handling (SR2 H2).

actor_test pins:
- IsSuspended propagates (regression for SR1 C1)
- IsSiteAdmin propagates (regression for SR2 C2)
- Impersonating fires when ImpersonatedUserID != 0 (SR2 C1)
- ImpersonateWriteOK is honored (read-only-by-default guarantee)
- IsSiteAdmin stays false on the impersonated identity
Authored by espadonne
SHA
c9d48312f053cd2af8f42a0a66b5d9979b6388dd
Parents
0fc23b6
Tree
f245206

3 changed files

StatusFile+-
M internal/auth/policy/actor.go 41 0
A internal/auth/policy/actor_test.go 115 0
M internal/web/middleware/auth.go 41 0
internal/auth/policy/actor.gomodified
@@ -34,6 +34,12 @@ func AnonymousActor() Actor {
3434
 // UserActor wraps a logged-in user. Suspension and site-admin flags
3535
 // must be loaded from the DB by the caller — the policy package does
3636
 // not query users on its own to keep the decision pure.
37
+//
38
+// New web-layer code should prefer UserActorFromCurrentUser, which
39
+// also propagates the impersonation pair so admin viewing-as flows
40
+// don't silently leak write permission. UserActor is preserved for
41
+// callers that don't have a CurrentUser handy (SSH/HTTP git, worker
42
+// jobs, tests).
3743
 func UserActor(userID int64, username string, suspended, siteAdmin bool) Actor {
3844
 	return Actor{
3945
 		UserID:      userID,
@@ -42,3 +48,38 @@ func UserActor(userID int64, username string, suspended, siteAdmin bool) Actor {
4248
 		IsSiteAdmin: siteAdmin,
4349
 	}
4450
 }
51
+
52
+// CurrentUserView is the minimal view of the request-bound user that
53
+// UserActorFromCurrentUser needs. The web's middleware.CurrentUser
54
+// satisfies this implicitly — keeping the type local avoids a
55
+// policy → middleware import cycle.
56
+type CurrentUserView struct {
57
+	ID                 int64
58
+	Username           string
59
+	IsSuspended        bool
60
+	IsSiteAdmin        bool
61
+	ImpersonatedUserID int64
62
+	RealActorID        int64
63
+	ImpersonateWriteOK bool
64
+}
65
+
66
+// UserActorFromCurrentUser is the canonical web-layer constructor.
67
+// Beyond UserActor, it propagates the impersonation pair so the
68
+// "read-only by default" promise on policy.go's impersonation gate
69
+// is actually enforced — and so audit rows can be tagged with the
70
+// real actor ID when impersonating.
71
+//
72
+// Convention: every handler that can run for an impersonating admin
73
+// should construct the actor through this constructor. Any callsite
74
+// still on plain UserActor with IsSiteAdmin=false hardcoded is a
75
+// silent permission leak (audit 2026-05-10 C1/C2).
76
+func UserActorFromCurrentUser(v CurrentUserView) Actor {
77
+	return Actor{
78
+		UserID:             v.ID,
79
+		Username:           v.Username,
80
+		IsSuspended:        v.IsSuspended,
81
+		IsSiteAdmin:        v.IsSiteAdmin,
82
+		Impersonating:      v.ImpersonatedUserID != 0,
83
+		ImpersonateWriteOK: v.ImpersonateWriteOK,
84
+	}
85
+}
internal/auth/policy/actor_test.goadded
@@ -0,0 +1,115 @@
1
+// SPDX-License-Identifier: AGPL-3.0-or-later
2
+
3
+package policy
4
+
5
+import "testing"
6
+
7
+// TestUserActorFromCurrentUser_PropagatesAllFlags pins the SR2 C1+C2
8
+// fix: the constructor used by the web layer must carry IsSuspended,
9
+// IsSiteAdmin, Impersonating, and ImpersonateWriteOK into the Actor.
10
+//
11
+// Before SR2, every web mutation handler used UserActor(...) with
12
+// `false` literals for site-admin/impersonation, so:
13
+//   - C1: an impersonating admin could write as the target user
14
+//     because Impersonating was never true at the actor level.
15
+//   - C2: a site admin could not read private repos they didn't
16
+//     collaborate on because IsSiteAdmin never propagated to Actor.
17
+//
18
+// This test fails LOUD on regressions of either path.
19
+func TestUserActorFromCurrentUser_PropagatesAllFlags(t *testing.T) {
20
+	t.Parallel()
21
+
22
+	cases := []struct {
23
+		name string
24
+		view CurrentUserView
25
+		want Actor
26
+	}{
27
+		{
28
+			name: "plain logged-in user",
29
+			view: CurrentUserView{ID: 7, Username: "alice"},
30
+			want: Actor{UserID: 7, Username: "alice"},
31
+		},
32
+		{
33
+			name: "suspended user",
34
+			view: CurrentUserView{ID: 7, Username: "alice", IsSuspended: true},
35
+			want: Actor{UserID: 7, Username: "alice", IsSuspended: true},
36
+		},
37
+		{
38
+			name: "site admin (not impersonating)",
39
+			view: CurrentUserView{ID: 7, Username: "alice", IsSiteAdmin: true},
40
+			want: Actor{UserID: 7, Username: "alice", IsSiteAdmin: true},
41
+		},
42
+		{
43
+			name: "impersonating admin, read-only-by-default",
44
+			view: CurrentUserView{
45
+				ID:                 99, // viewer.ID is the impersonated user
46
+				Username:           "target",
47
+				ImpersonatedUserID: 99,
48
+				RealActorID:        1,
49
+				ImpersonateWriteOK: false,
50
+			},
51
+			want: Actor{
52
+				UserID:        99,
53
+				Username:      "target",
54
+				Impersonating: true,
55
+				// ImpersonateWriteOK stays false — the policy gate on
56
+				// policy.go for write actions denies. This is the
57
+				// guarantee C1 was leaking.
58
+			},
59
+		},
60
+		{
61
+			name: "impersonating admin with write-mode opted in",
62
+			view: CurrentUserView{
63
+				ID:                 99,
64
+				Username:           "target",
65
+				ImpersonatedUserID: 99,
66
+				RealActorID:        1,
67
+				ImpersonateWriteOK: true,
68
+			},
69
+			want: Actor{
70
+				UserID:             99,
71
+				Username:           "target",
72
+				Impersonating:      true,
73
+				ImpersonateWriteOK: true,
74
+			},
75
+		},
76
+	}
77
+
78
+	for _, tc := range cases {
79
+		t.Run(tc.name, func(t *testing.T) {
80
+			t.Parallel()
81
+			got := UserActorFromCurrentUser(tc.view)
82
+			if got != tc.want {
83
+				t.Fatalf("UserActorFromCurrentUser:\n got=%+v\nwant=%+v", got, tc.want)
84
+			}
85
+		})
86
+	}
87
+}
88
+
89
+// TestUserActorFromCurrentUser_SiteAdminReadsPrivateRepo pins C2:
90
+// before SR2, IsSiteAdmin never reached the Actor on the read path,
91
+// so policy.go's "site admin reads anything" branch was unreachable
92
+// from the web layer. With the constructor migrated, the override
93
+// works as documented.
94
+func TestUserActorFromCurrentUser_ImpersonatingDoesNotEscalateAdmin(t *testing.T) {
95
+	t.Parallel()
96
+
97
+	// While impersonating, the wrapping middleware forces IsSiteAdmin
98
+	// to false on the impersonated identity. The constructor must
99
+	// honor that — never silently re-grant admin powers to the
100
+	// impersonated session.
101
+	view := CurrentUserView{
102
+		ID:                 99,
103
+		Username:           "target",
104
+		IsSiteAdmin:        false, // middleware enforced
105
+		ImpersonatedUserID: 99,
106
+		RealActorID:        1,
107
+	}
108
+	got := UserActorFromCurrentUser(view)
109
+	if got.IsSiteAdmin {
110
+		t.Fatal("UserActorFromCurrentUser must not set IsSiteAdmin while impersonating; got true")
111
+	}
112
+	if !got.Impersonating {
113
+		t.Fatal("UserActorFromCurrentUser must set Impersonating when ImpersonatedUserID is non-zero")
114
+	}
115
+}
internal/web/middleware/auth.gomodified
@@ -6,6 +6,8 @@ import (
66
 	"context"
77
 	"net/http"
88
 	"net/url"
9
+
10
+	"github.com/tenseleyFlow/shithub/internal/auth/policy"
911
 )
1012
1113
 var currentUserKey = ctxKey{name: "current_user"}
@@ -34,6 +36,45 @@ type CurrentUser struct {
3436
 // IsAnonymous reports whether this is an unauthenticated request.
3537
 func (u CurrentUser) IsAnonymous() bool { return u.ID == 0 }
3638
39
+// PolicyActor builds a policy.Actor that propagates suspension,
40
+// site-admin, and the impersonation pair. Use this everywhere the
41
+// web layer needs an actor — the alternative (plain
42
+// policy.UserActor) silently drops impersonation/site-admin and
43
+// re-introduces the C1/C2 leaks the SR2 sprint fixed.
44
+func (u CurrentUser) PolicyActor() policy.Actor {
45
+	return policy.UserActorFromCurrentUser(policy.CurrentUserView{
46
+		ID:                 u.ID,
47
+		Username:           u.Username,
48
+		IsSuspended:        u.IsSuspended,
49
+		IsSiteAdmin:        u.IsSiteAdmin,
50
+		ImpersonatedUserID: u.ImpersonatedUserID,
51
+		RealActorID:        u.RealActorID,
52
+		ImpersonateWriteOK: u.ImpersonateWriteOK,
53
+	})
54
+}
55
+
56
+// AuditActor returns the (actor_id, augmented meta) pair to record
57
+// for an audit row. During an impersonated session, the real admin
58
+// is the actor and the impersonated user_id is stashed in meta.
59
+// Outside impersonation, returns u.ID and meta unchanged.
60
+//
61
+// Use this anywhere a handler builds an audit row so impersonation
62
+// trails are uniform across admin and non-admin surfaces (SR2 H2).
63
+//
64
+// Most callers want the higher-level RecordAudit helper instead;
65
+// AuditActor is exposed for the rare case where the recorder/db
66
+// pair isn't readily threaded through.
67
+func (u CurrentUser) AuditActor(meta map[string]any) (int64, map[string]any) {
68
+	if u.RealActorID == 0 {
69
+		return u.ID, meta
70
+	}
71
+	if meta == nil {
72
+		meta = map[string]any{}
73
+	}
74
+	meta["impersonated_user_id"] = u.ImpersonatedUserID
75
+	return u.RealActorID, meta
76
+}
77
+
3778
 // UserLookupResult is what UserLookup returns. It's a struct so future
3879
 // fields (e.g. is_admin once S34 lands) don't keep widening the signature
3980
 // and forcing every callsite to update.