Skip to content

fix(approval-policy): allow user added through groups#6929

Open
adilsitos wants to merge 3 commits into
mainfrom
fix/adilsitos/secrets-378
Open

fix(approval-policy): allow user added through groups#6929
adilsitos wants to merge 3 commits into
mainfrom
fix/adilsitos/secrets-378

Conversation

@adilsitos

Copy link
Copy Markdown
Contributor

Context

A user that is added into a project by a group cannot be added as a single approver on secrets and access policies. This was happening because the check only validated users, and not users by group memberships.

There was also another bug that was fixed on this PR. group membership were not being checked when creating an approval policy, which allowed the addition of groups as approvers that did not belong to a project.

Screenshots

Steps to verify the change

Bug 1: User added into a project through group

  • create an approval policy and select the approver as an user that belong to a project by a group
  • check that this can be created (it was causing an error earlier)

Bug 2: add a group that does not belong to a project as approver

  • (the ui does not show groups that don't belong to a project, but this can be done using API)
  • try to create an approval policy using an external org.
  • check that this fails

Type

  • Fix
  • Feature
  • Improvement
  • Breaking
  • Docs
  • Chore

Checklist

  • Title follows the conventional commit format: type(scope): short description (scope is optional, e.g., fix: prevent crash on sync or fix(api): handle null response).
  • Tested locally
  • [ ] Updated docs (if needed)
  • Updated CLAUDE.md files (if needed)
  • Read the contributing guide

@adilsitos adilsitos requested a review from akhilmhdh June 18, 2026 18:38
@infisical-review-police

Copy link
Copy Markdown

💬 Discussion in Slack: #pr-review-infisical-6929-fix-approval-policy-allow-user-added-through-groups

Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel.

@linear

linear Bot commented Jun 18, 2026

Copy link
Copy Markdown

SECRETS-378

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f9c273c0f6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines 126 to +129
const groupApprovers = approvers
?.filter((approver) => approver.type === ApproverType.Group)
.map((approver) => approver.id);
.map((approver) => approver.id)
.filter(Boolean) as string[];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject blank group approver IDs instead of dropping them

When a secret-approval request contains a group approver with id: "" (the v1/v2 route schemas use plain z.string() for group IDs), this filter removes the only approver before membership verification and insertion. On create this can persist a policy with approvals: 1 and no approver rows; on update the same pattern can delete existing approvers and reinsert none, because insertMany([]) is a no-op. Please validate/reject blank group IDs rather than filtering them out.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant