Uh oh!
There was an error while loading. Please reload this page.
Add test recording permissions API behaviour - #3900
Merged
Merged
Conversation
denik
requested review from
andrewnester, anton-107, pietern and shreyas-goenka
as code ownersNovember 10, 2025 16:41
denikforce-pushed
the
denik/permissions-collapse
branch
from
November 10, 2025 17:02
a260d6f to
3ccae8cCompare| testdiff.PrepareReplacementsWorkspaceConfig(t, &repls, cfg) | ||
| cmd.Env = auth.ProcessEnv(cfg) | ||
| cmd.Env = append(cmd.Env, "USERNAME="+user.UserName) |
Contributor
There was a problem hiding this comment.
We already have this as CURRENT_USER_NAME. Maybe chanage the replacement?
ContributorAuthor
There was a problem hiding this comment.
thanks - missed that one; switched to that fd4aae8
Collaborator
6 failing tests:
Top 42 slowest tests (at least 2 minutes):
|
denikforce-pushed
the
denik/permissions-collapse
branch
from
November 10, 2025 20:07
fd4aae8 to
9c7375eCompareshreyas-goenka
approved these changes
Nov 11, 2025
Uh oh!
There was an error while loading. Please reload this page.
| jobs CAN_MANAGE,IS_OWNER => IS_OWNER | ||
| === Since we take the most recent level, IS_OWNER is lost, which results in the error due to lack of owner defined | ||
| jobs IS_OWNER,CAN_MANAGE => SET ERROR Error: The job must have exactly one owner. |
Contributor
There was a problem hiding this comment.
This fails because we try to define a second owner right? Not because we are unsetting the original owner?
ContributorAuthor
There was a problem hiding this comment.
No, this fails because there is no IS_OWNER in the request for a given principal according to backend logic (take last permission).
Co-authored-by: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com>
denik
enabled auto-merge
November 11, 2025 10:00
pietern
approved these changes
Nov 11, 2025
pietern
left a comment
Contributor
There was a problem hiding this comment.
This is great. The best test is against the real thing.
Uh oh!
There was an error while loading. Please reload this page.
| try: | ||
| return json.loads(result.stdout) | ||
| except Exception as ex: | ||
| raise Error(f"{cmd} returned non-json: {ex}\n{result.stdout}") |
Contributor
There was a problem hiding this comment.
Optional: function looks useful enough to put in a lib for acc testing.
Uh oh!
There was an error while loading. Please reload this page.
| def test_permissions(target_user, resource_type, resource_id, levels, expected): | ||
| acls = [] | ||
| if resource_type == "jobs" and target_user != os.environ["CURRENT_USER_NAME"]: | ||
| # make sure we keep IS_OWNER |
Contributor
There was a problem hiding this comment.
This is because "last entry wins"?
ContributorAuthor
There was a problem hiding this comment.
we need IS_OWNER since otherwise backend returns error and does not let set permissions.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
github-merge-queueBot
pushed a commit
that referenced
this pull request
Nov 12, 2025
## Changes When multiple permissions are present, select one per principal (highest of all available), ## Why Backend only stores one permission level per principal, whatever is latest in the request. So if users define multiple levels for the same principal, arbitrary level is going to be send to the backend, just whatever happens to be last in request. In some case, terraform will reject multiple levels and error. Why we select max level: The intent is usually to have some default permissions applied broadly (e.g., CAN_VIEW) and then grant higher permission (e.g. CAN_MANAGE) to selected principal, in which case principal has both CAN_VIEW and CAN_MANAGE which is the same as just CAN_MANAGE. Should fix#3864 ## Tests New acceptance test. Fix testserver to match real backend as tested by #3900
denik added a commit
that referenced
this pull request
May 20, 2026
## Changes Add test recording permissions API behaviour wrt multiple levels for the same principal. ## Why Document behaviour (important for permissions processing). I also plan to use this test to fix the testserver. --------- Co-authored-by: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com>
denik added a commit
that referenced
this pull request
May 20, 2026
## Changes When multiple permissions are present, select one per principal (highest of all available), ## Why Backend only stores one permission level per principal, whatever is latest in the request. So if users define multiple levels for the same principal, arbitrary level is going to be send to the backend, just whatever happens to be last in request. In some case, terraform will reject multiple levels and error. Why we select max level: The intent is usually to have some default permissions applied broadly (e.g., CAN_VIEW) and then grant higher permission (e.g. CAN_MANAGE) to selected principal, in which case principal has both CAN_VIEW and CAN_MANAGE which is the same as just CAN_MANAGE. Should fix#3864 ## Tests New acceptance test. Fix testserver to match real backend as tested by #3900
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Changes
Add test recording permissions API behaviour wrt multiple levels for the same principal.
Why
Document behaviour (important for permissions processing). I also plan to use this test to fix the testserver.