Repository navigation
feat: add organization invitations - #23
Merged
Merged
Conversation
Invite by email only, owner-only sending, 5-hour single-use tokens delivered through MailX's own outbound pipeline (same pattern as password reset). A single combined accept endpoint handles both an already-logged-in invitee and a no-account-yet invitee (signup folded into acceptance, always using the invitation's own email). Org logo/inviter avatar are plain nullable URL fields shown in the invite email when set. Dedicated per-human rate limit on sending. Migration 000030 adds org_invitations, tenants.logo_url, humans.avatar_url.
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. So far, Strix has surfaced 1 security issue across this workspace. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- escape owner/inviter-supplied names in the invitation HTML email - refuse to send an invite when the mailer is configured but the dashboard base URL is not, matching password reset's guard - rate-limit the public invite-accept endpoint (bcrypt per call) - recheck expiry atomically at token-consume time, not just when the request started - invalidate a prior pending invite when re-inviting the same address instead of stacking duplicate valid links - validate the invitee email before it reaches the database
The previous dedup fix invalidated the old invitation and committed the new one before attempting delivery, so a failed send left the invitee with neither a working old link nor a delivered new one. Invalidation now happens only after SendSystemEmail succeeds.
Two concurrent invites to the same address could each supersede the other after both had already sent, since exclusion was only "not this row's ID". Excluding by "created strictly before this row" instead makes the operation commutative: the newest invitation can never be invalidated by an older one racing behind it.
| // which request's UPDATE happens to run last. | ||
| func (db *DB) SupersedeOtherPendingOrgInvitations(ctx context.Context, tenantID, email, keepID string, keepCreatedAt, now time.Time) error { | ||
| _, err := db.pool.Exec(ctx, | ||
| `UPDATE org_invitations SET expires_at = $5 WHERE tenant_id = $1 AND normalized_email = $2 AND id != $3 AND created_at < $4 AND accepted_at IS NULL AND expires_at > $5`, |
There was a problem hiding this comment.
created_at value, neither satisfies the new strict < comparison, so both links remain usable for the same organization and address. PostgreSQL timestamps can share a microsecond, and the pending-invitation index is not unique. This leaves the one-live-link policy unenforced in that case; use a deterministic tie-breaker.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/database/org_invitations.go
Line: 85
Comment:
**Equal timestamps leave duplicate links** If concurrent invitations receive the same `created_at` value, neither satisfies the new strict `<` comparison, so both links remain usable for the same organization and address. PostgreSQL timestamps can share a microsecond, and the pending-invitation index is not unique. This leaves the one-live-link policy unenforced in that case; use a deterministic tie-breaker.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+405
to
+409
| for i := 0; i < n; i++ { | ||
| go func() { | ||
| done <- svc.InviteToOrganization(ctx, owner.Human.ID, tenant.ID, "invitee@example.com") | ||
| }() | ||
| } |
There was a problem hiding this comment.
fakeMailer, but its SendSystemEmail method appends to calls without synchronization. Concurrent sends can lose recorded emails, making the link-count assertion unreliable, and go test -race can fail. Synchronize the shared test mailer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/humanauth/org_invitation_test.go
Line: 405-409
Comment:
**Concurrent sends race in test** These five goroutines share one `fakeMailer`, but its `SendSystemEmail` method appends to `calls` without synchronization. Concurrent sends can lose recorded emails, making the link-count assertion unreliable, and `go test -race` can fail. Synchronize the shared test mailer.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Two invitations sharing the same microsecond-precision created_at had no defined winner under the strict-less-than comparison; now compares (created_at, id) as a tuple for a genuine total order. Also fixes an unsynchronized slice append in the test-only fakeMailer that a concurrent regression test exposed under -race.
There was a problem hiding this comment.
Ferousco-dev has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
org_invitations,tenants.logo_url,humans.avatar_urlTest plan
gofmt/go vet/go buildcleango test ./...andgo test -race ./...clean against real Postgres+Redisinternal/humanauth/org_invitation_test.goThe PR is not ready to merge while the concurrent-resend test can race; equal invitation timestamps also need a deterministic ordering.
Fix with agent prompt
Summary
The PR adds owner-issued organization invitations, email delivery, combined signup or existing-account acceptance, database-backed single-use tokens, and rate limits. The latest changes order concurrent resends by creation time and add a concurrency regression test.
Reviews (4) · Last reviewed commit: "fix: supersede org invitations by creati..."