Harden forward-auth, per-user isolation, and budget accuracy
Code-review fixes on the Authentik/budget/Resend work: - UpsertForwardUser: avoid 500 lockout on username UNIQUE collision and on the concurrent first-login race; normalize emails (lower+trim) so case variants don't create duplicate accounts. - Centralize email management in Authentik: when forward-auth mode is on, email is read-only in Veola for every user (not just forward rows); pure-local deployments keep editable email for Resend. - Fail closed on per-user isolation: ownedItem rejects uid==0/orphaned items; dashboard and global results error on a userless request. - Budget accuracy: count Apify usage only after a successful run, and count the billed Test Apify run. - Efficiency: resolve the deal-email recipient once per poll; build the settings page once in PostEmailPrefs.
This commit is contained in:
@@ -142,7 +142,16 @@ func (s *Store) GetUserByID(ctx context.Context, id int64) (*models.User, error)
|
||||
return u, err
|
||||
}
|
||||
|
||||
// normalizeEmail lowercases and trims an address so identity matching is
|
||||
// case-insensitive. The users.email unique index and the forward-auth match key
|
||||
// are otherwise byte-exact, which would let "Ada@x.com" and "ada@x.com" become
|
||||
// two accounts for one person.
|
||||
func normalizeEmail(email string) string {
|
||||
return strings.ToLower(strings.TrimSpace(email))
|
||||
}
|
||||
|
||||
func (s *Store) GetUserByEmail(ctx context.Context, email string) (*models.User, error) {
|
||||
email = normalizeEmail(email)
|
||||
if email == "" {
|
||||
return nil, nil
|
||||
}
|
||||
@@ -176,6 +185,7 @@ func (s *Store) ListUsers(ctx context.Context) ([]models.User, error) {
|
||||
// auth_source='forward' and carry an unusable password hash (forward-auth
|
||||
// users never log in with a local password).
|
||||
func (s *Store) UpsertForwardUser(ctx context.Context, email, username string, role models.Role) (*models.User, error) {
|
||||
email = normalizeEmail(email)
|
||||
if email == "" {
|
||||
return nil, errors.New("forward-auth requires an email")
|
||||
}
|
||||
@@ -184,11 +194,17 @@ func (s *Store) UpsertForwardUser(ctx context.Context, email, username string, r
|
||||
return nil, err
|
||||
}
|
||||
if existing != nil {
|
||||
if existing.Role != role || (username != "" && existing.Username != username) {
|
||||
newName := existing.Username
|
||||
if username != "" {
|
||||
newName = username
|
||||
newName := existing.Username
|
||||
if username != "" && username != existing.Username {
|
||||
// The IdP renamed the user. users.username is UNIQUE, so the
|
||||
// desired name may already belong to a different row; resolve a
|
||||
// free variant rather than letting the UPDATE fail with a 500.
|
||||
newName, err = s.uniqueUsername(ctx, username, existing.ID)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
}
|
||||
if existing.Role != role || newName != existing.Username {
|
||||
if _, err := s.DB.ExecContext(ctx,
|
||||
`UPDATE users SET role = ?, username = ? WHERE id = ?`,
|
||||
string(role), newName, existing.ID); err != nil {
|
||||
@@ -199,13 +215,27 @@ func (s *Store) UpsertForwardUser(ctx context.Context, email, username string, r
|
||||
}
|
||||
return existing, nil
|
||||
}
|
||||
if username == "" {
|
||||
username = email
|
||||
desired := username
|
||||
if desired == "" {
|
||||
desired = email
|
||||
}
|
||||
// users.username is UNIQUE; an Authentik username can collide with an
|
||||
// existing local/forward row. Pick a free variant so a new identity is
|
||||
// never locked out by a 500 on a username clash.
|
||||
uname, err := s.uniqueUsername(ctx, desired, 0)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
res, err := s.DB.ExecContext(ctx,
|
||||
`INSERT INTO users (username, password_hash, role, email, auth_source) VALUES (?, ?, ?, ?, 'forward')`,
|
||||
username, "!forward-auth", string(role), email)
|
||||
uname, "!forward-auth", string(role), email)
|
||||
if err != nil {
|
||||
// A concurrent first-login for the same email may have created the row
|
||||
// between GetUserByEmail and this INSERT; the email unique index then
|
||||
// rejects us. Re-resolve and let the race winner stand.
|
||||
if u, getErr := s.GetUserByEmail(ctx, email); getErr == nil && u != nil {
|
||||
return u, nil
|
||||
}
|
||||
return nil, err
|
||||
}
|
||||
id, err := res.LastInsertId()
|
||||
@@ -215,6 +245,24 @@ func (s *Store) UpsertForwardUser(ctx context.Context, email, username string, r
|
||||
return s.GetUserByID(ctx, id)
|
||||
}
|
||||
|
||||
// uniqueUsername returns desired if it is free (or already held by excludeID),
|
||||
// otherwise appends a numeric suffix until a free name is found. Bounded so a
|
||||
// pathological collision set can't loop forever.
|
||||
func (s *Store) uniqueUsername(ctx context.Context, desired string, excludeID int64) (string, error) {
|
||||
candidate := desired
|
||||
for i := 1; i <= 1000; i++ {
|
||||
ex, err := s.GetUserByUsername(ctx, candidate)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
if ex == nil || ex.ID == excludeID {
|
||||
return candidate, nil
|
||||
}
|
||||
candidate = fmt.Sprintf("%s-%d", desired, i)
|
||||
}
|
||||
return "", fmt.Errorf("no free username for %q", desired)
|
||||
}
|
||||
|
||||
func (s *Store) UpdateUserPassword(ctx context.Context, id int64, hash string) error {
|
||||
_, err := s.DB.ExecContext(ctx, `UPDATE users SET password_hash = ? WHERE id = ?`, hash, id)
|
||||
return err
|
||||
|
||||
Reference in New Issue
Block a user