Repository navigation
chore: sync main into release-candidate-v0.47.0 - #7046
Merged
Shivam-nagar23 merged 21 commits intoSep 21, 2026
Merged
Conversation
GetAllApiTokensForWebhook checked whether each STORED api token had trigger permission on the requested project/env, but never checked whether the CALLING user was permitted to query that project/env at all. As a result any authenticated user could request an arbitrary projectName/environmentName/appName and receive the plaintext token values of every stored api token (including super-admin tokens, whose Casbin policy is "*" and therefore always passes the per-token check). Add the same enforcement pattern used by the sibling handlers in this file, but scoped to the resource being queried: verify the requesting user's own token has trigger permission on the requested project and every requested environment before the service call is made, so no token data is looked up or returned for a project/env the caller cannot access. Fixes #7013
Inline the unnecessary "ok" variable in the caller-RBAC check added in the previous commit, per SonarCloud's "remove this unnecessary variable declaration and use the expression directly" finding on PR #7016.
…allers Checking that a caller has trigger permission on the requested project/env was not sufficient: a super-admin api-token's Casbin policy is "*", so it always satisfies that same per-object check regardless of how narrow the actual project/env being queried is. That meant any caller with legitimate, narrow trigger access to some app/env could still have super-admin (or other more-privileged) tokens returned alongside their own in the same response. Add callerDominatesToken, which excludes a stored token from the response whenever it is a super-admin token and the caller is not, reusing the same super-admin check already used by the other handlers in this file. The per-object check now runs against each stored token before this dominance check, matching the existing per-token-per-object evaluation the service already performs. The Enforcer interface here only exposes single-tuple Enforce/ EnforceInBatch calls, not policy or role enumeration, so a fully generic "is token's permission a strict subset of the caller's" comparison across every possible object isn't available without extending that interface. Super-admin dominance is the well-defined, checkable case that matches this issue (leaking admin token values), so it's what this fix targets; a more general scope comparison would need broader RBAC changes outside this endpoint's blast radius.
Group consecutive same-type parameters (storedToken/projObj/envObj and callerToken/storedToken) per SonarCloud's finding on the code added in the previous two commits.
Per thread decision: GetAllApiTokensForWebhook's only real caller is the dashboard's webhook config modal, which is already super-admin-only in the UI. Rather than layering a per-project/env caller check plus a token-dominance check on top of the existing per-stored-token filter, just gate the whole endpoint on the caller being a super-admin - the same Enforce(token, ResourceGlobal, ActionUpdate, "*") check already used by every other handler in this file (GetAllApiTokens, CreateApiToken, UpdateApiToken, DeleteApiToken). This removes the per-object caller check and the callerDominatesToken logic added in the previous two commits; the existing per-stored-token CheckAuthorizationForWebhook callback into the service is left as-is since it still does useful filtering (only tokens with trigger permission on the requested project/env are returned), it's just no longer load-bearing for access control now that the endpoint itself requires a super-admin caller. Net effect vs main: an 8-line super-admin check added right after GetLoggedInUser, matching the pattern already used by every sibling handler in this file.
Inline the unnecessary "ok" variable in the super-admin check added to GetAllApiTokensForWebhook, per SonarCloud's "remove this unnecessary variable declaration" finding on PR #7016.
…-7013 fix(security): enforce caller RBAC in API token webhook endpoint
… performance - Introduce `createApp`/`deleteApp` actions to segregate application lifecycle control from pipeline/workflow/config operations. - Update RBAC enforcement to reflect the new `createApp`/`deleteApp` actions for better granularity. - Add `idx_ci_artifact`, `idx_ci_workflow`, and other indexes to improve CI query execution and resolve PG_READ_TIMEOUT issues. - Introduce GIN index for `git_triggers` to optimize material/commit lookup in `validateBuildSequence`.
DELETE /orchestrator/user/bulk with a listingRequest filter (instead of
explicit ids) resolved the delete target list via the same query
builder the listing endpoint uses, but skipped the normalization the
listing endpoint applies first (SetDefaultValuesIfNotPresent,
setStatusFilterType). With no default page size and no status
filtering applied, a filter such as {"searchKey":"","status":["inactive"]}
resolved to every active user instead of the intended filtered subset,
causing a bulk delete of far more users than intended.
getUserIdsHonoringFilters now applies the same normalization the
listing path (GetAllWithFilters) applies before building the query,
and rejects a filter-based bulk delete that carries no concrete filter
criteria instead of letting it silently resolve to "every user". The
explicit ids-based delete path is unchanged.
Fixes #7020
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5DesTG5XvPdmPCDuL6izq
feat: decouple app lifecycle actions; add indexes to improve CI query…
fix: scope filter-based bulk user delete to match listing filters
When an external helm app is linked to the chart store, app_name holds the unique identifier (<releaseName>-<namespace>-<clusterId>) while display_name holds the release name. RBAC policies are always created against the release name, since that is what the listing APIs expose to the user. EnforcerUtil did not account for this: - GetHelmObjectByAppNameAndEnvId looked the app up by app_name only, so a release-name lookup returned ErrNoRows and the object collapsed to "//", making the app invisible to any non super-admin user. - Both it and GetHelmObject built the object from application.AppName, i.e. the unique identifier, which never matched the granted policy. - Neither substituted the "unassigned" project when team_id is 0, so an app linked without a project produced an empty project segment. EnforcerUtilHelm.GetAppRBACNameByInstalledAppId and GetAppRBACNameByInstalledAppIdAndTeamId had the same raw AppName problem. Resolve the app the same way getAppObject already does: reconstruct the unique identifier from the environment and try that first, falling back to the plain name for regular chart store apps, legacy rows and callers that already pass the identifier. Emit the display name via the new App.GetAppNameForRbac(), and fall back to the unassigned project when no project is assigned. Fixing the primitives covers all downstream call sites without touching the handlers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…re intentionally not sent for the plugin's internal steps.
feat: resolve RBAC inconsistencies for helm apps with unique identifiers
…-pipeline-level-oss misc: Conditions saved with the plugin definition (plugin_step_condition) a…
* chore: create a makefile to setup and clean resources for development * feat: add flag for frontend to show argo/flux tabs - add migration files - append flags in the /orchestrator/user/check/roles API to show/hide argo/flux tabs * feat: changes for dropdown API in user permissions page * fix: missing dependencies in constructor method * fix: improper filtering logic - earlier filter was only applied with the cluster ID - add changes to filter based on cluster ID's and namespaces * fix: prevent connecting to kubelink for failed clusters * wip: policy enforcements * wip: policy enforcements * chore: remove test folder * fix: code review fixes * fix: code review fixes * fix: code review fixes * chore: rename migration scripts to maintain sync with enterprise * chore: change migration script number * fix: roles missing while fetching user roles * fix: deleted roles being listed * fix: deleted roles being listed * fix: review fixes
|
Bito Review Skipped - No Changes Detected |
vivek-devtron
previously approved these changes
Sep 21, 2026
…bac idempotent
Two different migrations shared version 36504600 on the release branch:
add_ci_workflow_active_status_index and dev_admin_app_lifecycle. The
latter is already released on main at that number, so the CI index
migration (RC-only) is renumbered to 36904600, the next free slot.
Also adds ON CONFLICT ("entity","access_type","role") DO NOTHING to both
INSERTs in 36804600_argo_flux_rbac.up.sql. Both rbac_policy_data and
rbac_role_data declare UNIQUE (entity, access_type, role), so a re-run
previously failed on the unique constraint instead of being a no-op.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vivek-devtron
approved these changes
Sep 21, 2026
|
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
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.
Ref: https://github.com/devtron-labs/sprint-tasks/issues/2962
Prerequisite for #7045 (RC v0.47.0 -> main), which is BEHIND —
mainrequires branches be up to date before merging (strict status checks) and the RC was 19 commits behind.Two commits.
1. Sync
maininto the RC — duplicate migration removedThe raw merge re-introduced
scripts/sql/36604600_argo_flux_rbac.{up,down}.sqlfrommain. That is a problem twice over:36804600_argo_flux_rbacin misc: main sync develop 16sep #7039 (misc: main sync develop 16sep). Both files are byte-identical (upsha13cc1063011…,downsha147c3fc625b…), so the same DDL would be applied under two different versions.36604600is already taken on the RC by36604600_approval_declined_notification— a straight number collision.Resolution: dropped
main's36604600_argo_flux_rbac.{up,down}.sql, kept the RC's36804600_argo_flux_rbac. Apart from that removal the merge is content-neutral — it bringsmain's history across so the branch counts as up to date.2. Resolve the
36504600collision + make argo/flux rbac idempotentRenumber. Two different migrations shared version
36504600on the RC:36504600_add_ci_workflow_active_status_index36504600_dev_admin_app_lifecycledev_admin_app_lifecycleis already released onmainat36504600, so its number must not move.add_ci_workflow_active_status_indexexists only on the RC and is renumbered to36904600, the next free slot after36804600(verified unused onmain, the RC anddevelop).Idempotency.
36804600_argo_flux_rbac.up.sqlhad two bareINSERTs. Bothrbac_policy_dataandrbac_role_datadeclareUNIQUE ("entity", "access_type", "role"), so a re-run failed on the unique constraint rather than being a no-op. AddedON CONFLICT ("entity", "access_type", "role") DO NOTHINGto both statements.The rest of the tail of the sequence was already idempotent and is untouched:
36504600_dev_admin_app_lifecycleNOT ('createApp' = ANY (allowed_actions))36604600_approval_declined_notification36704600_variable_entity_mapping_indexIF NOT EXISTS36804600_argo_flux_rbac36904600_add_ci_workflow_active_status_indexCREATE INDEX IF NOT EXISTS/DROP INDEX IF EXISTSAfter this PR there are no duplicate migration numbers on the RC.
feat/dev-admin-pipeline-lifecyclebranch also uses36904600. It will need a bump before it merges.The identical migration changes are applied to devtron-enterprise in its own RC PR.
🤖 Generated with Claude Code