Fix #5682: add IS_OWNER for permissions added by Python mutators#5821
Open
denik wants to merge 6 commits into
Open
Fix #5682: add IS_OWNER for permissions added by Python mutators#5821denik wants to merge 6 commits into
denik wants to merge 6 commits into
Conversation
Contributor
Approval status: pending
|
Collaborator
Integration test reportCommit: cc5aac3
19 interesting tests: 15 SKIP, 4 RECOVERED
Top 5 slowest tests (at least 2 minutes):
|
denik
changed the title
Permissions added to an existing resource by a PyDABs job/pipeline mutator go through NormalizeResources (UpdatedResources path), which does not run FixPermissions. The deploying user is therefore never added as IS_OWNER, and the direct engine PUTs an ownerless ACL that the Permissions API rejects with "The pipeline must have exactly one owner". The terraform provider re-injects the owner at PUT time, so it succeeds - hence the terraform-vs-direct divergence. Model the real backend rule (jobs and pipelines require exactly one owner; zero or two both fail) in the testserver, and add an acceptance test that deploys and records the per-engine outcome. Co-authored-by: Isaac
Convert the reproduction to a real deploy against a UC pipeline: notebook library, unique names via envsubst, a real grantee group on cloud, and cleanup. Add INVALID_PARAMETER_VALUE to the testserver's owner error to match the real Permissions API response shown in the issue. Co-authored-by: Isaac
Permissions added to an existing resource by a PyDABs job/pipeline mutator went through NormalizeResources, which ran only the normalize mutators and skipped FixPermissions. The deploying user was therefore never added as IS_OWNER, and the direct engine PUT an ownerless ACL that the Permissions API rejects with "must have exactly one owner". The terraform provider re-injected the owner at PUT time, hiding the bug for that engine. Run FixPermissions in NormalizeResources so Python-sourced permissions get the same owner treatment as YAML ones. FixPermissions is idempotent, so re-running it on resources that already have an owner is a no-op; ApplyBundlePermissions is not re-run because it is not idempotent and already ran in ProcessStaticResources. Guard FixPermissions against a nil CurrentUser, which is possible when it runs outside the initialize phase. Co-authored-by: Isaac
Co-authored-by: Isaac
The testserver now enforces exactly-one-owner for pipelines (like jobs), so the pipelines.permissions CRUD fixture must set IS_OWNER instead of CAN_MANAGE. Co-authored-by: Isaac
denik
force-pushed
the
denik/permissions-5682-one-owner
branch
from
796efdb to
1306a5d
Compare
Co-authored-by: Isaac
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
2 participants
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.
Permissions added to an existing job/pipeline by a PyDABs mutator went through
NormalizeResources, which skippedFixPermissions. The deploying user was never added asIS_OWNER, so the direct engine sent an ownerless permissions PUT and the API rejected it withThe <resource> must have exactly one owner(#5682). The terraform provider re-injects the owner at PUT time, which is why the same bundle worked on terraform but failed on the direct engine.Fix: run
FixPermissionsinNormalizeResourcesso permissions added by a Python mutator get the same owner treatment as permissions declared in YAML.FixPermissionsis idempotent, so it is a no-op for resources that already have an owner;ApplyBundlePermissionsis intentionally not re-run (not idempotent, already applied earlier).FixPermissionsalso now skips whenCurrentUseris unset.Includes an acceptance test that deploys a pipeline whose only permission is added by a Python mutator; verified against real AWS on both engines.
This pull request and its description were written by Isaac.