Skip to content

Commit 89bd1f0

Browse files
committed
fix: make administrative user offboarding irreversible and complete (WI-1308)
Offboarding could be undone through pending invitations: user_invitations survived the anonymization and invitation acceptance activated any inactive user, resurrecting the retained row with an attacker-chosen password. The admin activate endpoint and the RECOVER_USER tool could also reactivate an offboarded account, and the cleanup missed several user-owned stores. - add an irreversible users.offboarded_at lifecycle state, set by OffboardUser and never cleared; every activation path rejects it (invitation verify/accept/generate, admin activate, password reset, RECOVER_USER) - extend the offboarding transaction to delete pending invitations, integration OAuth tokens, Todoist sync config and task links, integration OAuth state, calendar feed tokens, portal request drafts, team memberships, leave periods (releasing substitute slots), on-call layer memberships and overrides, and asset set roles - collect encrypted SCM and integration OAuth grant material before the rows are deleted and revoke the grants best-effort after commit where providers support it (GitHub OAuth Apps, Todoist); Gitea and Notion have no revocation API and expire unused - regression coverage: invitation reactivation, per-table cleanup, revocation collection, migration upgrade, and HTTP-level activation and password-reset denial contracts
1 parent de45c78 commit 89bd1f0

10 files changed

Lines changed: 443 additions & 59 deletions

File tree

internal/database/migrations.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -551,6 +551,14 @@ var Catalog = []Migration{
551551
ALTER TABLE notifications ADD COLUMN referenced_workspace_permission TEXT;
552552
`,
553553
},
554+
{
555+
Version: "20260911_users_offboarded_at",
556+
Name: "Add irreversible offboarded lifecycle state to users",
557+
CheckSQLite: sqliteColumnCheck("users", "offboarded_at"),
558+
CheckPostgres: pgColumnCheck("users", "offboarded_at"),
559+
SQLite: `ALTER TABLE users ADD COLUMN offboarded_at DATETIME`,
560+
Postgres: `ALTER TABLE users ADD COLUMN IF NOT EXISTS offboarded_at TIMESTAMPTZ`,
561+
},
554562
{
555563
Version: "20260905_notification_email_claims",
556564
Name: "Add recoverable notification email claims",

internal/database/schema/base_tables_postgres.sql

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ CREATE TABLE IF NOT EXISTS users (
2222
email_verification_expires TIMESTAMPTZ, -- Expiry time for verification token
2323
scim_external_id TEXT, -- SCIM externalId from identity provider
2424
scim_managed BOOLEAN DEFAULT false, -- If true, user is managed via SCIM
25+
offboarded_at TIMESTAMPTZ, -- Set when the account is administratively offboarded; never cleared, every activation path must reject it
2526
is_agent BOOLEAN DEFAULT false, -- If true, user is a non-human agent (API-only; cannot log in)
2627
agent_owner_user_id INTEGER REFERENCES users(id) ON DELETE CASCADE, -- NULL = service user (admin-provisioned); non-NULL = owned agent
2728
-- Distinguishes how an agent row got created. 'user' covers both the

internal/database/schema/users.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
email_verification_expires DATETIME, -- Expiry time for verification token
1616
scim_external_id TEXT, -- SCIM externalId from identity provider
1717
scim_managed BOOLEAN DEFAULT false, -- If true, user is managed via SCIM
18+
offboarded_at DATETIME, -- Set when the account is administratively offboarded; never cleared, every activation path must reject it
1819
is_agent BOOLEAN DEFAULT FALSE, -- If true, user is a non-human agent (API-only; cannot log in)
1920
agent_owner_user_id INTEGER REFERENCES users(id) ON DELETE CASCADE, -- NULL = service user (admin-provisioned); non-NULL = owned agent (inherits owner permissions)
2021
-- Distinguishes how an agent row got created. 'user' covers both the
@@ -164,4 +165,6 @@ CREATE INDEX IF NOT EXISTS idx_user_invitations_token ON user_invitations(token)
164165
CREATE INDEX IF NOT EXISTS idx_user_invitations_user_id ON user_invitations(user_id);
165166

166167

168+
-- migration: 20260911_users_offboarded_at
169+
167170
-- migration: 0014_users_is_agent

internal/handlers/users.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,13 @@ func (h *UserHandler) ResetPassword(w http.ResponseWriter, r *http.Request) {
527527
return
528528
}
529529

530+
// Password material on an offboarded account is meaningless state; refuse
531+
// instead of writing credentials onto a retired row.
532+
if target.Offboarded {
533+
respondConflict(w, r, "User has been offboarded and cannot be reactivated")
534+
return
535+
}
536+
530537
if err := h.repo.SetPassword(id, string(hashedBytes), requiresReset); err != nil {
531538
respondInternalError(w, r, err)
532539
return
@@ -618,6 +625,13 @@ func (h *UserHandler) ActivateUser(w http.ResponseWriter, r *http.Request) {
618625
return
619626
}
620627

628+
// Offboarding is irreversible; activation must never resurrect the
629+
// anonymized account.
630+
if target.Offboarded {
631+
respondConflict(w, r, "User has been offboarded and cannot be reactivated")
632+
return
633+
}
634+
621635
if err := h.repo.SetActive(id, true); err != nil {
622636
respondInternalError(w, r, err)
623637
return

internal/integrations/todoist/client.go

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
package todoist
33

44
import (
5+
"context"
56
"encoding/json"
67
"fmt"
78
"io"
@@ -15,7 +16,8 @@ import (
1516

1617
const (
1718
authorizeURL = "https://todoist.com/oauth/authorize"
18-
tokenURL = "https://todoist.com/oauth/access_token" // #nosec G101 -- OAuth token endpoint URL, not a credential
19+
tokenURL = "https://todoist.com/oauth/access_token" // #nosec G101 -- OAuth token endpoint URL, not a credential
20+
revokeURL = "https://api.todoist.com/sync/v9/access_tokens/revoke" // #nosec G101 -- OAuth revoke endpoint URL, not a credential
1921
requestTimeout = 10 * time.Second
2022
)
2123

@@ -73,3 +75,31 @@ func AuthorizeURL(clientID, scope, state string) string {
7375
}
7476
return authorizeURL + "?" + q.Encode()
7577
}
78+
79+
// RevokeToken invalidates a user's Todoist OAuth access token at the provider.
80+
// A 2xx response counts as revoked; Todoist returns 204 on success.
81+
func RevokeToken(ctx context.Context, clientID, clientSecret, accessToken string) error {
82+
form := url.Values{
83+
"client_id": {clientID},
84+
"client_secret": {clientSecret},
85+
"access_token": {accessToken},
86+
}
87+
req, err := http.NewRequestWithContext(ctx, http.MethodPost, revokeURL, strings.NewReader(form.Encode()))
88+
if err != nil {
89+
return fmt.Errorf("creating revoke request: %w", err)
90+
}
91+
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
92+
93+
client := utils.NewHTTPClient(requestTimeout)
94+
resp, err := client.Do(req)
95+
if err != nil {
96+
return fmt.Errorf("revoking token: %w", err)
97+
}
98+
defer func() { _ = resp.Body.Close() }()
99+
_, _ = io.Copy(io.Discard, resp.Body)
100+
101+
if resp.StatusCode < 200 || resp.StatusCode > 299 {
102+
return fmt.Errorf("todoist revoke error (status %d)", resp.StatusCode)
103+
}
104+
return nil
105+
}

internal/repository/user_repository.go

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -687,18 +687,19 @@ func (r *UserRepository) GetDeleteSnapshot(id int) (*DeleteSnapshot, error) {
687687

688688
// PasswordResetTarget is the small subset the password-reset audit needs.
689689
type PasswordResetTarget struct {
690-
Username string
691-
Email string
690+
Username string
691+
Email string
692+
Offboarded bool
692693
}
693694

694695
// GetPasswordResetTarget returns username+email for the reset audit.
695696
// Returns ErrNotFound when missing.
696697
func (r *UserRepository) GetPasswordResetTarget(id int) (*PasswordResetTarget, error) {
697698
var t PasswordResetTarget
698699
err := r.db.QueryRow(
699-
"SELECT username, email FROM users WHERE id = ?",
700+
"SELECT username, email, offboarded_at IS NOT NULL FROM users WHERE id = ?",
700701
id,
701-
).Scan(&t.Username, &t.Email)
702+
).Scan(&t.Username, &t.Email, &t.Offboarded)
702703
if err != nil {
703704
return nil, notFoundOrWrap(err, fmt.Sprintf("get user %d for password reset", id))
704705
}
@@ -716,21 +717,22 @@ func (r *UserRepository) SetPassword(id int, passwordHash string, requiresReset
716717
}
717718

718719
// ActivationTarget carries username/email/is_active for the activate/deactivate
719-
// audit + idempotence check.
720+
// audit + idempotence check, plus the irreversible offboarding state.
720721
type ActivationTarget struct {
721-
Username string
722-
Email string
723-
IsActive bool
722+
Username string
723+
Email string
724+
IsActive bool
725+
Offboarded bool
724726
}
725727

726728
// GetActivationTarget reads the activate/deactivate audit fields.
727729
// Returns ErrNotFound when missing.
728730
func (r *UserRepository) GetActivationTarget(id int) (*ActivationTarget, error) {
729731
var t ActivationTarget
730732
err := r.db.QueryRow(
731-
"SELECT username, email, is_active FROM users WHERE id = ?",
733+
"SELECT username, email, is_active, offboarded_at IS NOT NULL FROM users WHERE id = ?",
732734
id,
733-
).Scan(&t.Username, &t.Email, &t.IsActive)
735+
).Scan(&t.Username, &t.Email, &t.IsActive, &t.Offboarded)
734736
if err != nil {
735737
return nil, notFoundOrWrap(err, fmt.Sprintf("get user %d activation target", id))
736738
}

internal/server/server.go

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import (
5151
"windshift/internal/scm"
5252
"windshift/internal/services"
5353
"windshift/internal/smtp"
54+
"windshift/internal/sso"
5455
"windshift/internal/standardagent"
5556
"windshift/internal/utils"
5657
"windshift/internal/webauthn"
@@ -131,17 +132,21 @@ type Server struct {
131132
tokenTracker *services.TokenTracker
132133
webhookSender *webhook.WebhookSender
133134
scmSyncStopChan chan struct{}
134-
issueSyncStopChan chan struct{}
135-
magicLinkStopChan chan struct{}
136-
cleanupStopChan chan struct{}
137-
jiraHostStopChan chan struct{}
138-
cleanupTicker *time.Ticker
139-
pluginManager *plugins.Manager
140-
databaseDiagRepo *repository.DatabaseDiagnosticsRepository
141-
databasePoolMonitor *services.DatabasePoolMonitor
142-
channelService *services.ChannelService
143-
memoryBudget config.MemoryBudget
144-
metrics *appmetrics.Metrics
135+
// secretEncryption is the at-rest secret cipher shared by the SCM and
136+
// integration OAuth surfaces; set during wiring, used to revoke provider
137+
// grants when a user is offboarded.
138+
secretEncryption *sso.SecretEncryption
139+
issueSyncStopChan chan struct{}
140+
magicLinkStopChan chan struct{}
141+
cleanupStopChan chan struct{}
142+
jiraHostStopChan chan struct{}
143+
cleanupTicker *time.Ticker
144+
pluginManager *plugins.Manager
145+
databaseDiagRepo *repository.DatabaseDiagnosticsRepository
146+
databasePoolMonitor *services.DatabasePoolMonitor
147+
channelService *services.ChannelService
148+
memoryBudget config.MemoryBudget
149+
metrics *appmetrics.Metrics
145150

146151
loginRateLimiter *middleware.RateLimiter
147152
runnerRegisterLimiter *middleware.RateLimiter
@@ -577,9 +582,14 @@ func (s *Server) initialize() error {
577582
invitationService,
578583
services.NewUserReadService(s.db),
579584
func(id int) error {
580-
tokenIDs, err := services.OffboardUser(s.db, id, s.notificationService, authorizationCacheInvalidator)
581-
tokenManager.InvalidateTokens(tokenIDs)
585+
result, err := services.OffboardUser(s.db, id, s.notificationService, authorizationCacheInvalidator)
586+
if len(result.RevokedAPITokenIDs) > 0 {
587+
tokenManager.InvalidateTokens(result.RevokedAPITokenIDs)
588+
}
582589
sessionManager.InvalidateUserSessionValidation(id)
590+
if err == nil {
591+
s.revokeUserRemoteGrants(result.RemoteRevocations)
592+
}
583593
return err
584594
},
585595
userDeactivationService.DeactivateUser,
@@ -774,6 +784,7 @@ func (s *Server) initialize() error {
774784
ssoHandler := handlers.NewSSOHandler(s.db, sessionManager, permService, emailVerificationService, s.pluginManager, cfg.Auth.SessionSecret, baseURL, cfg.AllowedHosts, cfg.DisableCSRF, ipExtractor, cfg.UseProxy, additionalProxyList)
775785

776786
scmProviderHandler := handlers.NewSCMProviderHandler(s.db, cfg.Auth.SessionSecret, baseURL)
787+
s.secretEncryption = scmProviderHandler.GetEncryption()
777788
scmWorkspaceRepo := repository.NewSCMWorkspaceRepository(s.db)
778789
scmWorkspaceHandler := handlers.NewSCMWorkspaceHandler(scmWorkspaceRepo, scmProviderHandler.GetEncryption(), scmProviderHandler, scm.NewCredentialResolver(s.db, scmProviderHandler.GetEncryption()), permService, baseURL)
779790
scmItemLinksHandler := handlers.NewSCMItemLinksHandler(s.db, scmProviderHandler.GetEncryption(), permService)
@@ -1922,13 +1933,20 @@ func (s *Server) recoverUser(username string) {
19221933
var id int
19231934
var userEmail string
19241935
var isActive bool
1936+
var offboarded bool
19251937
err := s.db.QueryRow(
1926-
`SELECT id, email, is_active FROM users WHERE username = ?`, username,
1927-
).Scan(&id, &userEmail, &isActive)
1938+
`SELECT id, email, is_active, offboarded_at IS NOT NULL FROM users WHERE username = ?`, username,
1939+
).Scan(&id, &userEmail, &isActive, &offboarded)
19281940
if err != nil {
19291941
slog.Error("RECOVER_USER: user not found", "username", username)
19301942
return
19311943
}
1944+
if offboarded {
1945+
// Offboarding is irreversible; the recovery tool must not resurrect
1946+
// an anonymized account.
1947+
slog.Error("RECOVER_USER: refusing to re-enable offboarded user", "username", username, "id", id)
1948+
return
1949+
}
19321950
if isActive {
19331951
slog.Info("RECOVER_USER: user is already active, no action needed", "username", username, "email", userEmail)
19341952
return

0 commit comments

Comments
 (0)