From f2370bd4fac0b57da1712e3a5bde58888fab5ff4 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Tue, 29 Sep 2026 18:53:25 +0100 Subject: [PATCH 1/2] fix(membership): end a member's sessions only in the organization that acted (#831) Changing a member's role or withdrawing their access in organization A called RevokeAllUserTokens, which deleted every refresh token the person held, so A's decision signed them out of organization B as well. The membership service now revokes through RevokeUserTokensInTenant, which deletes only the refresh tokens whose tenant_id is A. Sessions in other organizations are unaffected, and are still re-checked against their own membership on every refresh by the org session resolver. Password change and reset keep revoking everything. Signed-off-by: alex-dembele --- .../application/membership/members.go | 4 ++-- .../application/membership/service.go | 11 +++++++--- .../application/membership/service_test.go | 21 +++++++++++-------- backend/internal/auth/token.go | 13 ++++++++++++ 4 files changed, 35 insertions(+), 14 deletions(-) diff --git a/backend/internal/application/membership/members.go b/backend/internal/application/membership/members.go index 5ee7927e..e0fe3be4 100644 --- a/backend/internal/application/membership/members.go +++ b/backend/internal/application/membership/members.go @@ -157,7 +157,7 @@ func (s *Service) ChangeRole(ctx context.Context, tenantID uuid.UUID, in ChangeR // to re-derive claims, so a demotion actually takes effect rather than // waiting for whatever session they happen to hold to lapse on its own. if s.revoker != nil { - _ = s.revoker.RevokeAllUserTokens(ctx, m.UserID) + _ = s.revoker.RevokeUserTokensInTenant(ctx, m.UserID, tenantID) } v := toMemberView(m) @@ -233,7 +233,7 @@ func (s *Service) SetStatus(ctx context.Context, tenantID uuid.UUID, in SetStatu if !in.Status.GrantsAccess() && s.revoker != nil { // Best-effort: a revoker outage must not leave the membership half-changed // in the database. The membership itself already refuses new sessions. - _ = s.revoker.RevokeAllUserTokens(ctx, m.UserID) + _ = s.revoker.RevokeUserTokensInTenant(ctx, m.UserID, tenantID) } v := toMemberView(m) diff --git a/backend/internal/application/membership/service.go b/backend/internal/application/membership/service.go index a5a97923..90ff9141 100644 --- a/backend/internal/application/membership/service.go +++ b/backend/internal/application/membership/service.go @@ -79,10 +79,15 @@ type InvitationMail struct { SendersEmail string } -// SessionRevoker ends a member's sessions when their access is withdrawn. -// Satisfied by the auth TokenManager's RevokeAllUserTokens. +// SessionRevoker ends a member's sessions in one organization when that +// organization changes or withdraws their access. Satisfied by the auth +// TokenManager's RevokeUserTokensInTenant. +// +// It is deliberately scoped: a decision of organization A must not sign the +// person out of organization B (#831). Their sessions in B are re-checked +// against their B membership on every refresh anyway. type SessionRevoker interface { - RevokeAllUserTokens(ctx context.Context, userID uuid.UUID) error + RevokeUserTokensInTenant(ctx context.Context, userID, tenantID uuid.UUID) error } // PasswordHasher hashes the password an invitee chooses on acceptance. diff --git a/backend/internal/application/membership/service_test.go b/backend/internal/application/membership/service_test.go index 6cd11a2d..8129d48e 100644 --- a/backend/internal/application/membership/service_test.go +++ b/backend/internal/application/membership/service_test.go @@ -346,21 +346,24 @@ func (m *stubMailer) SendInvitation(_ context.Context, mail InvitationMail) erro type stubRevoker struct { mu sync.Mutex - revoked []uuid.UUID + revoked []revocation } -func (s *stubRevoker) RevokeAllUserTokens(_ context.Context, id uuid.UUID) error { +type revocation struct{ user, tenant uuid.UUID } + +func (s *stubRevoker) RevokeUserTokensInTenant(_ context.Context, id, tenant uuid.UUID) error { s.mu.Lock() defer s.mu.Unlock() - s.revoked = append(s.revoked, id) + s.revoked = append(s.revoked, revocation{id, tenant}) return nil } -func (s *stubRevoker) has(id uuid.UUID) bool { +// hasIn reports a revocation for the user in that tenant. +func (s *stubRevoker) hasIn(id, tenant uuid.UUID) bool { s.mu.Lock() defer s.mu.Unlock() for _, x := range s.revoked { - if x == id { + if x.user == id && x.tenant == tenant { return true } } @@ -553,8 +556,8 @@ func TestChangeRole_AppliesAndAudits(t *testing.T) { } // A demotion the member is still holding a token for has not taken effect // until their refresh lineage is gone. - if !h.revoker.has(target.UserID) { - t.Error("a role change must end the member's refresh lineage") + if !h.revoker.hasIn(target.UserID, h.tenantA) { + t.Error("a role change must end the member's refresh lineage in this organization") } } @@ -635,8 +638,8 @@ func TestSetStatus_DeactivateReactivateRevoke(t *testing.T) { t.Fatalf("deactivation did not take: %+v", v) } // Withdrawing access is not a UI state — the sessions have to go. - if !h.revoker.has(target.UserID) { - t.Error("deactivation must end the member's sessions") + if !h.revoker.hasIn(target.UserID, h.tenantA) { + t.Error("deactivation must end the member's sessions in this organization") } if ev := h.audit.find("organization_member", "update"); ev == nil || ev.After["reason"] != "leave of absence" { t.Errorf("deactivation must be audited with its reason: %+v", ev) diff --git a/backend/internal/auth/token.go b/backend/internal/auth/token.go index 2e4a1efa..6d784e7f 100644 --- a/backend/internal/auth/token.go +++ b/backend/internal/auth/token.go @@ -566,6 +566,19 @@ func (tm *TokenManager) RevokeAllUserTokens(ctx context.Context, userID uuid.UUI return nil } +// RevokeUserTokensInTenant revokes the user's refresh tokens for ONE +// organization. It is what an organization's own decision about a member (a +// role change, a deactivation) may end: that member's sessions in that +// organization, never their sessions elsewhere (#831). Account-level events +// (password change or reset) use RevokeAllUserTokens instead. +func (tm *TokenManager) RevokeUserTokensInTenant(ctx context.Context, userID, tenantID uuid.UUID) error { + result := tm.db.WithContext(ctx).Where("user_id = ? AND tenant_id = ?", userID, tenantID).Delete(&RefreshToken{}) + if result.Error != nil { + return fmt.Errorf("failed to revoke user tokens in tenant: %w", result.Error) + } + return nil +} + // generateRefreshToken generates a cryptographically secure random refresh token func generateRefreshToken() (string, error) { bytes := make([]byte, 32) From 08ecc467913ab106d6eecff933e6f57070b36a05 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Tue, 29 Sep 2026 18:53:25 +0100 Subject: [PATCH 2/2] test(membership): prove an action in one tenant leaves sessions in the others alive (#831) Signed-off-by: alex-dembele --- .../handler/membership_session_scope_test.go | 138 ++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 backend/internal/handler/membership_session_scope_test.go diff --git a/backend/internal/handler/membership_session_scope_test.go b/backend/internal/handler/membership_session_scope_test.go new file mode 100644 index 00000000..80d8f5f1 --- /dev/null +++ b/backend/internal/handler/membership_session_scope_test.go @@ -0,0 +1,138 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package handler_test + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "errors" + "net/http" + "testing" + + "github.com/google/uuid" + + coreauth "github.com/opendefender/openrisk/internal/auth" + "github.com/opendefender/openrisk/internal/domain" + authpkg "github.com/opendefender/openrisk/pkg/auth" +) + +// #831 — an organization's decision about a member ends that member's sessions +// in that organization only. The membership route is driven for real, with the +// real TokenManager as the revoker, against live refresh tokens in A and B. + +func TestMembershipSessions_WithdrawingAccessInAKeepsTheSessionInB(t *testing.T) { + for _, tc := range []struct { + name string + path string + body map[string]any + reason string + }{ + {"deactivate", "/status", map[string]any{"status": string(domain.MembershipDeactivated)}, "access withdrawn"}, + {"revoke", "/status", map[string]any{"status": string(domain.MembershipRevoked)}, "access withdrawn"}, + {"role change", "/role", map[string]any{"role": string(domain.RoleAdmin)}, "claims must be re-derived"}, + } { + t.Run(tc.name, func(t *testing.T) { + f := newOrgFixture(t) + tm := newSessionManager(t, f) + f.svc.WithSessionRevoker(tm) + + user, inA, _ := seedTwoOrgMember(t, f) + ctx := context.Background() + pairA, err := tm.GenerateTokenPair(ctx, user.ID, f.tenantA, nil, nil, nil, coreauth.DeviceContext{}) + if err != nil { + t.Fatalf("session in A: %v", err) + } + pairB, err := tm.GenerateTokenPair(ctx, user.ID, f.tenantB, nil, nil, nil, coreauth.DeviceContext{}) + if err != nil { + t.Fatalf("session in B: %v", err) + } + + f.mustCall(t, "admin@a.io", http.MethodPut, + "/api/v1/organization/members/"+inA.ID.String()+tc.path, tc.body, http.StatusOK) + + if _, err := tm.RefreshTokenPair(ctx, pairA.RefreshToken, coreauth.DeviceContext{}); err == nil { + t.Fatalf("the A session must end after %s (%s)", tc.name, tc.reason) + } + next, err := tm.RefreshTokenPair(ctx, pairB.RefreshToken, coreauth.DeviceContext{}) + if err != nil { + t.Fatalf("the B session must survive a %s in A, refresh failed: %v", tc.name, err) + } + t.Logf("after %s in A: A refresh refused, B refresh ok (new token issued: %v)", tc.name, next.AccessToken != "") + }) + } +} + +// Criterion 3: an account-level event still ends every session. Password +// change and reset call RevokeAllUserTokens (change_password.go, +// password_reset.go); this pins what that call does across organizations. +func TestMembershipSessions_AccountLevelRevocationStillEndsEverySession(t *testing.T) { + f := newOrgFixture(t) + tm := newSessionManager(t, f) + user, _, _ := seedTwoOrgMember(t, f) + ctx := context.Background() + + pairA, _ := tm.GenerateTokenPair(ctx, user.ID, f.tenantA, nil, nil, nil, coreauth.DeviceContext{}) + pairB, _ := tm.GenerateTokenPair(ctx, user.ID, f.tenantB, nil, nil, nil, coreauth.DeviceContext{}) + + if err := tm.RevokeAllUserTokens(ctx, user.ID); err != nil { + t.Fatalf("revoke all: %v", err) + } + for name, p := range map[string]*coreauth.TokenPair{"A": pairA, "B": pairB} { + if _, err := tm.RefreshTokenPair(ctx, p.RefreshToken, coreauth.DeviceContext{}); err == nil { + t.Errorf("an account-level revocation must end the %s session too", name) + } + } +} + +// newSessionManager builds a TokenManager over the fixture's database. Its org +// resolver re-checks the membership the way the composition root's +// resolveSessionForOrg does: an inactive or missing membership refuses. +func newSessionManager(t *testing.T, f *orgFixture) *coreauth.TokenManager { + t.Helper() + if err := f.db.Exec(`CREATE TABLE refresh_tokens ( + id TEXT PRIMARY KEY, user_id TEXT NOT NULL, tenant_id TEXT NOT NULL, + family_id TEXT NOT NULL, token_hash TEXT NOT NULL UNIQUE, + device_fingerprint TEXT, ip_address TEXT, user_agent TEXT, + expires_at DATETIME NOT NULL, rotated_at DATETIME, last_used_at DATETIME, + created_at DATETIME, updated_at DATETIME)`).Error; err != nil { + t.Fatalf("create refresh_tokens: %v", err) + } + priv, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("rsa: %v", err) + } + tm := coreauth.NewTokenManager(f.db, &authpkg.RSAKeys{PrivateKey: priv, PublicKey: &priv.PublicKey}) + tm.SetOrgSessionResolver(func(ctx context.Context, userID, orgID uuid.UUID) (*coreauth.SessionClaims, error) { + var m domain.OrganizationMember + if err := f.db.WithContext(ctx).Where("user_id = ? AND organization_id = ? AND is_active = ?", userID, orgID, true).First(&m).Error; err != nil { + return nil, errors.New("no active membership") + } + return &coreauth.SessionClaims{TenantID: orgID, OrgRoles: map[uuid.UUID]string{orgID: string(m.Role)}}, nil + }) + return tm +} + +// seedTwoOrgMember creates one person with an active membership in A and in B. +func seedTwoOrgMember(t *testing.T, f *orgFixture) (*domain.User, *domain.OrganizationMember, *domain.OrganizationMember) { + t.Helper() + user := &domain.User{ID: uuid.New(), Email: "dual@both.io", Username: "dual", FullName: "Dual Member", IsActive: true} + if err := f.db.Create(user).Error; err != nil { + t.Fatalf("seed user: %v", err) + } + mk := func(org uuid.UUID, role domain.MemberRole) *domain.OrganizationMember { + m := &domain.OrganizationMember{ + ID: uuid.New(), OrganizationID: org, UserID: user.ID, Role: role, + Status: domain.MembershipActive, IsActive: true, + JoinedAt: f.now, CreatedAt: f.now, UpdatedAt: f.now, + } + if err := f.db.Create(m).Error; err != nil { + t.Fatalf("seed member: %v", err) + } + return m + } + return user, mk(f.tenantA, domain.RoleUser), mk(f.tenantB, domain.RoleAdmin) +}