feat: add admin organization member invitations - #1029
italic-jinxin wants to merge 2 commits into
Conversation
GitHub Actions
Inactive
Review · Status⬛ StoppedIronLoop stopped because the pull request was closed while this Run was active. Run detailsAutomatic trigger · attempt 1 of 3 · stopped after 4m |
|
Review: admin organization member invitations Traced the whole path (route → Things that check out (worth stating, since they are the risky parts):
Issues 1. Ownerless orgs become unrepairable through this endpoint ( 2. if invitations.iter().any(|(_, role)| matches!(role, MemberRole::Owner)) {
return Err(OrganizationError::InvalidParams(...));
}A 50-entry batch with one 3. 4. Behaviour change on the existing (non-admin) endpoint (
Both are improvements, but they are response-semantics changes on a live endpoint — worth a changelog entry and a check that the dashboard does not key off the echoed email. 5. 6. Rolling-update window ( Nit Nothing here is data-corrupting, and the concurrency/migration handling is careful. (1) and (3) are the ones I would want resolved before merge, since they affect whether the endpoint does its job. |
GitHub Actions
Failure
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 6m 53s |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review · Summary
Found one authorization issue in the invitation email normalization.
Findings: 🔴 High 1
Code-specific findings are attached to the diff.
Validation
- ✅ Captured unit tests — The captured unit-test check passed.
- ❌ Captured E2E tests — The most recent captured E2E check failed with exit code 100; the supplied evidence does not identify the failing test.
Review details
- Run:
2e069fb0-ce3d-461e-9d34-8d5ee1636ac9 - Attempts: 1
| JOIN organizations o ON o.id = i.organization_id | ||
| LEFT JOIN users u ON u.id = i.invited_by_user_id | ||
| WHERE i.email = $1 AND i.status = $2 | ||
| WHERE LOWER(i.email) = LOWER($1) AND i.status = $2 |
There was a problem hiding this comment.
🔴 High · Case-folded invitation lookup crosses account boundaries
User emails are uniquely stored with exact-case semantics, and OAuth provisioning keys accounts by provider identity, so distinct active accounts can differ only by email case. This lookup now exposes a pending invitation for either account; the existing acceptance flow also compares case-folded emails and grants the invitation’s role (including owner). Enforce a single canonical email identity before case-insensitive matching, or retain exact matching for invitation visibility and acceptance.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 OpenCodeReview found 11 issue(s) in this PR.
- ✅ 10 posted as inline comment(s)
- 📝 1 posted as summary
📄 crates/api/tests/e2e_all/invitations.rs
Test coverage gap: the test verifies that list_by_email_with_details (used by /v1/users/me/invitations) matches emails case-insensitively, but does not exercise the case-insensitive behavior of the create() method itself. The key behavioral guarantee introduced by the repository change is that calling create() with "admin@test.com" will expire an existing pending invitation stored as "Admin@Test.com" (via the LOWER(email) = LOWER($2) UPDATE). Without a test for this path, a regression that reverts the expire-UPDATE to case-sensitive matching would go undetected. Consider adding a test that creates an invitation with one case variant via the API, then creates another with a different case variant for the same org, and asserts the first is expired.
| } else { | ||
| format!("Failed to create invitation: {e}") | ||
| }, |
There was a problem hiding this comment.
The non-conflict error branch formats the raw anyhow error e directly into the per-invitation error field: format!("Failed to create invitation: {e}"). This InvitationResult.error is passed through to the API consumer via services_invitation_result_to_api inside the 200 OK response body (see admin.rs line ~3028 and organization_members.rs line ~193). Unlike top-level service errors (which are mapped to generic messages like "Failed to create organization invitations"), per-invitation errors are NOT sanitized at the API boundary. The raw anyhow error may contain database connection strings, SQL fragments, or internal stack context, leading to information leakage to API consumers.
Suggestion:
| } else { | |
| format!("Failed to create invitation: {e}") | |
| }, | |
| } else { | |
| tracing::warn!( | |
| organization_id = %organization_id.0, | |
| "Failed to create invitation: {e}" | |
| ); | |
| "Failed to create invitation".to_string() | |
| }, |
| async fn has_owner(&self, id: Uuid) -> Result<bool, RepositoryError> { | ||
| let org = self.org.lock().unwrap(); | ||
| Ok(org.id.0 == id) | ||
| } |
There was a problem hiding this comment.
The test mock for has_owner returns Ok(org.id.0 == id), which is always true whenever the organization exists in the stub. This makes the "Organization has no owner; restore ownership before inviting members" guard (line 1014) untestable — there is no way for a test to exercise the false branch of this check. Consider making the stub configurable (e.g., adding a has_owner: bool or Mutex<bool> field) so that the no-owner scenario can be covered by tests.
Suggestion:
| async fn has_owner(&self, id: Uuid) -> Result<bool, RepositoryError> { | |
| let org = self.org.lock().unwrap(); | |
| Ok(org.id.0 == id) | |
| } | |
| async fn has_owner(&self, id: Uuid) -> Result<bool, RepositoryError> { | |
| Ok(self.org.lock().unwrap().id.0 == id && *self.has_owner_flag.lock().unwrap()) | |
| } |
| JOIN users user_account ON user_account.id = member.user_id | ||
| WHERE member.organization_id = $1 | ||
| AND user_account.is_active = true | ||
| AND LOWER(user_account.email) = LOWER($2) |
There was a problem hiding this comment.
The SQL uses LOWER(user_account.email) = LOWER($2) but does not apply TRIM(), while the Rust caller in create_invitations_with_role_impl normalizes the email with both trim() and to_lowercase() (line 1051). If a user's stored email in the users table contains leading/trailing whitespace, this membership check will fail to match, allowing an invitation to be created for someone who is already a member. The migration V0078 only creates an index on LOWER(email) for organization_invitations and does not normalize or trim existing users.email values. Consider adding TRIM() to both sides of the comparison, or ensuring emails are always trimmed at storage time.
Suggestion:
| AND LOWER(user_account.email) = LOWER($2) | |
| AND LOWER(TRIM(user_account.email)) = LOWER(TRIM($2)) |
| let organization_name = self | ||
| .repository | ||
| .get_active_name_by_id(invitation.organization_id.0) | ||
| .await | ||
| .map_err(Self::map_repository_error)? | ||
| .ok_or(OrganizationError::NotFound)?; |
There was a problem hiding this comment.
The resend_invitation_email_impl method now uses get_active_name_by_id (which filters on is_active = true) instead of the previous get_organization_impl to fetch the organization name. This introduces a behavior change: if an organization is deactivated after invitations were sent, resending the invitation email will now fail with NotFound instead of proceeding. This may be intentional (preventing emails for deactivated orgs), but it is a breaking change from the prior behavior. If resending for deactivated orgs should still work, consider using a repository method that does not filter on is_active, or document this as intentional behavior.
Suggestion:
| let organization_name = self | |
| .repository | |
| .get_active_name_by_id(invitation.organization_id.0) | |
| .await | |
| .map_err(Self::map_repository_error)? | |
| .ok_or(OrganizationError::NotFound)?; | |
| // If resending for deactivated orgs is intentional, keep the previous | |
| // get_organization_impl call which does not filter on is_active: | |
| // let org = self.get_organization_impl(invitation.organization_id.clone()).await?; | |
| // let organization_name = org.name; | |
| // If this is intentional, add a comment documenting the behavior change. |
| CREATE UNIQUE INDEX unique_pending_organization_invitation | ||
| ON organization_invitations (organization_id, LOWER(email)) | ||
| WHERE status = 'pending'; |
There was a problem hiding this comment.
Missing IF NOT EXISTS on CREATE UNIQUE INDEX. The project uses IF NOT EXISTS in other index-creation migrations (e.g., V0062). More importantly, without IF NOT EXISTS, the CONCURRENTLY pre-build workaround documented in V0062 cannot be used: if an operator pre-builds this unique index with CREATE UNIQUE INDEX CONCURRENTLY to avoid an ACCESS EXCLUSIVE lock on a large production table, this migration will then fail because the index already exists. Adding IF NOT EXISTS makes the migration a no-op if the index was pre-built, which is the pattern established in V0062.
Suggestion:
| CREATE UNIQUE INDEX unique_pending_organization_invitation | |
| ON organization_invitations (organization_id, LOWER(email)) | |
| WHERE status = 'pending'; | |
| CREATE UNIQUE INDEX IF NOT EXISTS unique_pending_organization_invitation | |
| ON organization_invitations (organization_id, LOWER(email)) | |
| WHERE status = 'pending'; |
| CREATE UNIQUE INDEX unique_pending_organization_invitation | ||
| ON organization_invitations (organization_id, LOWER(email)) | ||
| WHERE status = 'pending'; |
There was a problem hiding this comment.
No operational note about CONCURRENTLY for large production tables. V0062 includes a comment explaining that Refinery wraps migrations in a transaction (so CONCURRENTLY cannot be used inline) and recommends pre-building indexes manually with CONCURRENTLY before deployment. The same concern applies here: creating a unique index on a large organization_invitations table will hold an ACCESS EXCLUSIVE lock for the duration of the build. Consider adding a comment similar to V0062's note so operators know to pre-build the index on production databases.
| ALTER TABLE organization_invitations | ||
| DROP CONSTRAINT IF EXISTS organization_invitations_organization_id_email_status_key; |
There was a problem hiding this comment.
The old case-sensitive index idx_org_invitations_email ON organization_invitations(email) created in V0007 is now redundant: all email-based queries in the repository have been updated to use LOWER(email), which cannot use the raw email index. The only remaining raw-email references are the ILIKE '%' || $2 || '%' filters in list_email_deliveries, but leading-wildcard ILIKE cannot use a B-tree index regardless. Consider dropping idx_org_invitations_email in this migration to avoid unnecessary write-path index maintenance and reduce confusion about which email index is active.
Suggestion:
| ALTER TABLE organization_invitations | |
| DROP CONSTRAINT IF EXISTS organization_invitations_organization_id_email_status_key; | |
| ALTER TABLE organization_invitations | |
| DROP CONSTRAINT IF EXISTS organization_invitations_organization_id_email_status_key; | |
| DROP INDEX IF EXISTS idx_org_invitations_email; |
| CREATE INDEX idx_org_invitations_email_lower | ||
| ON organization_invitations (LOWER(email)); |
There was a problem hiding this comment.
Missing IF NOT EXISTS. Same reasoning as V0077: if an operator pre-builds this index with CREATE INDEX CONCURRENTLY on a large production table (following the pattern documented in V0062), this migration will fail because the index already exists. Using IF NOT EXISTS makes the migration a no-op when the index was pre-built.
Suggestion:
| CREATE INDEX idx_org_invitations_email_lower | |
| ON organization_invitations (LOWER(email)); | |
| CREATE INDEX IF NOT EXISTS idx_org_invitations_email_lower | |
| ON organization_invitations (LOWER(email)); |
| @@ -226,7 +228,10 @@ impl OrganizationInvitationRepository for PgOrganizationInvitationRepository { | |||
| ], | |||
| ) | |||
| .await | |||
| .map_err(map_db_error) | |||
| .map_err(map_db_error)?; | |||
|
|
|||
| transaction.commit().await.map_err(map_db_error)?; | |||
| Ok(row) | |||
There was a problem hiding this comment.
No explicit SET TRANSACTION ISOLATION LEVEL SERIALIZABLE. The retry_db macro only retries on TransactionConflict, ConnectionFailed, and PoolError, and TransactionConflict maps from T_R_SERIALIZATION_FAILURE and T_R_DEADLOCK_DETECTED. Without SERIALIZABLE isolation, the default (READ COMMITTED) will not produce serialization failures, so retry_db will never retry this transaction. The transaction does protect atomicity (expire + insert either both succeed or both roll back), but if the intent was to also gain retry-on-conflict behavior, the isolation level must be set. Note that the partial unique index (V0077) will produce a UNIQUE_VIOLATION (mapped to AlreadyExists, which is NOT retried) rather than a serialization conflict under any isolation level, so two concurrent create() calls for the same org+email will surface as AlreadyExists rather than one being retried. Worth verifying whether that error mapping is the intended behavior.
| WHERE LOWER(email) = LOWER($1) | ||
| ORDER BY created_at DESC", |
There was a problem hiding this comment.
Lower(email) expression index usage note: the list_by_email and list_by_email_with_details queries use WHERE LOWER(email) = LOWER($1), which is backed by the new idx_org_invitations_email_lower index from V0078 — good. However, the partial unique index from V0077 (organization_id, LOWER(email) WHERE status = 'pending') cannot serve the general LOWER(email) = LOWER($1) lookup because it is filtered to pending rows. This is fine as long as V0078 is always applied alongside V0077, which it is. Just flagging that the two migrations are interdependent: deploying only V0077 without V0078 would leave the general list_by_email queries without index support on LOWER(email) (they would fall back to sequential scan on large tables).
1 participant
Summary
Testing
cargo fmt --all -- --checkcargo check -p apicargo test -p services organization --lib— 25 passed