diff --git a/app/api/auth/repo.go b/app/api/auth/repo.go index 73406bfac..f9fdb429c 100644 --- a/app/api/auth/repo.go +++ b/app/api/auth/repo.go @@ -57,15 +57,16 @@ func CheckRepo( return Check(ctx, authorizer, session, scope, resource, permission) } -func IsSpaceAdmin( +func IsRepoOwner( ctx context.Context, authorizer authz.Authorizer, session *auth.Session, repo *types.Repository, ) (bool, error) { - err := CheckRepo(ctx, authorizer, session, repo, enum.PermissionSpaceCreate, false) + // for now we use repoedit as permission to verify if someone is a SpaceOwner and hence a RepoOwner. + err := CheckRepo(ctx, authorizer, session, repo, enum.PermissionRepoEdit, false) if err != nil && !errors.Is(err, ErrNotAuthorized) { - return false, fmt.Errorf("failed to check access to find if the user is space admin: %w", err) + return false, fmt.Errorf("failed to check access user access: %w", err) } return err == nil, nil diff --git a/app/api/controller/githook/pre_receive.go b/app/api/controller/githook/pre_receive.go index 5c8a1481a..e3c93a62a 100644 --- a/app/api/controller/githook/pre_receive.go +++ b/app/api/controller/githook/pre_receive.go @@ -98,9 +98,9 @@ func (c *Controller) checkProtectionRules( refUpdates changedRefs, output *githook.Output, ) error { - isSpaceOwner, err := apiauth.IsSpaceAdmin(ctx, c.authorizer, session, repo) + isRepoOwner, err := apiauth.IsRepoOwner(ctx, c.authorizer, session, repo) if err != nil { - return err + return fmt.Errorf("failed to determine if user is repo owner: %w", err) } protectionRules, err := c.protectionManager.ForRepository(ctx, repo.ID) @@ -117,12 +117,12 @@ func (c *Controller) checkProtectionRules( } violations, err := protectionRules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ - Actor: &session.Principal, - IsSpaceOwner: isSpaceOwner, - Repo: repo, - RefAction: refAction, - RefType: refType, - RefNames: names, + Actor: &session.Principal, + IsRepoOwner: isRepoOwner, + Repo: repo, + RefAction: refAction, + RefType: refType, + RefNames: names, }) if err != nil { errCheckAction = fmt.Errorf("failed to verify protection rules for git push: %w", err) diff --git a/app/api/controller/pullreq/merge.go b/app/api/controller/pullreq/merge.go index 221800f54..ffd5ac2a2 100644 --- a/app/api/controller/pullreq/merge.go +++ b/app/api/controller/pullreq/merge.go @@ -129,9 +129,9 @@ func (c *Controller) Merge( } } - isSpaceOwner, err := apiauth.IsSpaceAdmin(ctx, c.authorizer, session, targetRepo) + isRepoOwner, err := apiauth.IsRepoOwner(ctx, c.authorizer, session, targetRepo) if err != nil { - return nil, nil, fmt.Errorf("failed to determine if the user is space admin: %w", err) + return nil, nil, fmt.Errorf("failed to determine if user is repo owner: %w", err) } checkResults, err := c.checkStore.ListResults(ctx, targetRepo.ID, pr.SourceSHA) @@ -152,7 +152,7 @@ func (c *Controller) Merge( ruleOut, violations, err := protectionRules.MergeVerify(ctx, protection.MergeVerifyInput{ Actor: &session.Principal, - IsSpaceOwner: isSpaceOwner, + IsRepoOwner: isRepoOwner, TargetRepo: targetRepo, SourceRepo: sourceRepo, PullReq: pr, diff --git a/app/api/controller/repo/commit.go b/app/api/controller/repo/commit.go index 0ee39b7ab..5b611bbb7 100644 --- a/app/api/controller/repo/commit.go +++ b/app/api/controller/repo/commit.go @@ -62,7 +62,7 @@ func (c *Controller) CommitFiles(ctx context.Context, return types.CommitFilesResponse{}, nil, err } - rules, isSpaceOwner, err := c.fetchRules(ctx, session, repo) + rules, isRepoOwner, err := c.fetchRules(ctx, session, repo) if err != nil { return types.CommitFilesResponse{}, nil, err } @@ -78,12 +78,12 @@ func (c *Controller) CommitFiles(ctx context.Context, } violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ - Actor: &session.Principal, - IsSpaceOwner: isSpaceOwner, - Repo: repo, - RefAction: refAction, - RefType: protection.RefTypeBranch, - RefNames: []string{branchName}, + Actor: &session.Principal, + IsRepoOwner: isRepoOwner, + Repo: repo, + RefAction: refAction, + RefType: protection.RefTypeBranch, + RefNames: []string{branchName}, }) if err != nil { return types.CommitFilesResponse{}, nil, fmt.Errorf("failed to verify protection rules: %w", err) diff --git a/app/api/controller/repo/controller.go b/app/api/controller/repo/controller.go index d79bc9836..6043f5725 100644 --- a/app/api/controller/repo/controller.go +++ b/app/api/controller/repo/controller.go @@ -130,9 +130,9 @@ func (c *Controller) fetchRules( session *auth.Session, repo *types.Repository, ) (protection.Protection, bool, error) { - isSpaceOwner, err := apiauth.IsSpaceAdmin(ctx, c.authorizer, session, repo) + isRepoOwner, err := apiauth.IsRepoOwner(ctx, c.authorizer, session, repo) if err != nil { - return nil, false, fmt.Errorf("failed to determine space ownership: %w", err) + return nil, false, fmt.Errorf("failed to determine if user is repo owner: %w", err) } protectionRules, err := c.protectionManager.ForRepository(ctx, repo.ID) @@ -140,5 +140,5 @@ func (c *Controller) fetchRules( return nil, false, fmt.Errorf("failed to fetch protection rules for the repository: %w", err) } - return protectionRules, isSpaceOwner, nil + return protectionRules, isRepoOwner, nil } diff --git a/app/api/controller/repo/create_branch.go b/app/api/controller/repo/create_branch.go index 5823cba67..aec8e3fb7 100644 --- a/app/api/controller/repo/create_branch.go +++ b/app/api/controller/repo/create_branch.go @@ -51,18 +51,18 @@ func (c *Controller) CreateBranch(ctx context.Context, in.Target = repo.DefaultBranch } - rules, isSpaceOwner, err := c.fetchRules(ctx, session, repo) + rules, isRepoOwner, err := c.fetchRules(ctx, session, repo) if err != nil { return nil, nil, err } violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ - Actor: &session.Principal, - IsSpaceOwner: isSpaceOwner, - Repo: repo, - RefAction: protection.RefActionCreate, - RefType: protection.RefTypeBranch, - RefNames: []string{in.Name}, + Actor: &session.Principal, + IsRepoOwner: isRepoOwner, + Repo: repo, + RefAction: protection.RefActionCreate, + RefType: protection.RefTypeBranch, + RefNames: []string{in.Name}, }) if err != nil { return nil, nil, fmt.Errorf("failed to verify protection rules: %w", err) diff --git a/app/api/controller/repo/create_commit_tag.go b/app/api/controller/repo/create_commit_tag.go index f555f7e22..4964f448d 100644 --- a/app/api/controller/repo/create_commit_tag.go +++ b/app/api/controller/repo/create_commit_tag.go @@ -55,18 +55,18 @@ func (c *Controller) CreateCommitTag(ctx context.Context, in.Target = repo.DefaultBranch } - rules, isSpaceOwner, err := c.fetchRules(ctx, session, repo) + rules, isRepoOwner, err := c.fetchRules(ctx, session, repo) if err != nil { return nil, nil, err } violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ - Actor: &session.Principal, - IsSpaceOwner: isSpaceOwner, - Repo: repo, - RefAction: protection.RefActionCreate, - RefType: protection.RefTypeTag, - RefNames: []string{in.Name}, + Actor: &session.Principal, + IsRepoOwner: isRepoOwner, + Repo: repo, + RefAction: protection.RefActionCreate, + RefType: protection.RefTypeTag, + RefNames: []string{in.Name}, }) if err != nil { return nil, nil, fmt.Errorf("failed to verify protection rules: %w", err) diff --git a/app/api/controller/repo/delete_branch.go b/app/api/controller/repo/delete_branch.go index ed0fcb286..3f4755c5a 100644 --- a/app/api/controller/repo/delete_branch.go +++ b/app/api/controller/repo/delete_branch.go @@ -46,18 +46,18 @@ func (c *Controller) DeleteBranch(ctx context.Context, return nil, usererror.ErrDefaultBranchCantBeDeleted } - rules, isSpaceOwner, err := c.fetchRules(ctx, session, repo) + rules, isRepoOwner, err := c.fetchRules(ctx, session, repo) if err != nil { return nil, err } violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ - Actor: &session.Principal, - IsSpaceOwner: isSpaceOwner, - Repo: repo, - RefAction: protection.RefActionDelete, - RefType: protection.RefTypeBranch, - RefNames: []string{branchName}, + Actor: &session.Principal, + IsRepoOwner: isRepoOwner, + Repo: repo, + RefAction: protection.RefActionDelete, + RefType: protection.RefTypeBranch, + RefNames: []string{branchName}, }) if err != nil { return nil, fmt.Errorf("failed to verify protection rules: %w", err) diff --git a/app/api/controller/repo/delete_tag.go b/app/api/controller/repo/delete_tag.go index dab43ffb5..1ab9fd47b 100644 --- a/app/api/controller/repo/delete_tag.go +++ b/app/api/controller/repo/delete_tag.go @@ -37,18 +37,18 @@ func (c *Controller) DeleteTag(ctx context.Context, return nil, err } - rules, isSpaceOwner, err := c.fetchRules(ctx, session, repo) + rules, isRepoOwner, err := c.fetchRules(ctx, session, repo) if err != nil { return nil, err } violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ - Actor: &session.Principal, - IsSpaceOwner: isSpaceOwner, - Repo: repo, - RefAction: protection.RefActionDelete, - RefType: protection.RefTypeTag, - RefNames: []string{tagName}, + Actor: &session.Principal, + IsRepoOwner: isRepoOwner, + Repo: repo, + RefAction: protection.RefActionDelete, + RefType: protection.RefTypeTag, + RefNames: []string{tagName}, }) if err != nil { return nil, fmt.Errorf("failed to verify protection rules: %w", err) diff --git a/app/services/protection/bypass.go b/app/services/protection/bypass.go index 7c66bd2b4..51a499528 100644 --- a/app/services/protection/bypass.go +++ b/app/services/protection/bypass.go @@ -19,8 +19,8 @@ import ( ) type DefBypass struct { - UserIDs []int64 `json:"user_ids,omitempty"` - SpaceOwners bool `json:"space_owners,omitempty"` + UserIDs []int64 `json:"user_ids,omitempty"` + RepoOwners bool `json:"repo_owners,omitempty"` } func (v DefBypass) Sanitize() error { diff --git a/app/services/protection/rule_branch.go b/app/services/protection/rule_branch.go index e70effc86..8da493df4 100644 --- a/app/services/protection/rule_branch.go +++ b/app/services/protection/rule_branch.go @@ -42,7 +42,7 @@ func (v *Branch) MergeVerify( ctx context.Context, in MergeVerifyInput, ) (MergeVerifyOutput, []types.RuleViolations, error) { - if v.isBypassed(in.Actor, in.IsSpaceOwner) { + if v.isBypassed(in.Actor, in.IsRepoOwner) { return MergeVerifyOutput{}, nil, nil } @@ -53,7 +53,7 @@ func (v *Branch) RefChangeVerify( ctx context.Context, in RefChangeVerifyInput, ) ([]types.RuleViolations, error) { - if v.isBypassed(in.Actor, in.IsSpaceOwner) || in.RefType != RefTypeBranch || len(in.RefNames) == 0 { + if v.isBypassed(in.Actor, in.IsRepoOwner) || in.RefType != RefTypeBranch || len(in.RefNames) == 0 { return nil, nil } @@ -76,9 +76,9 @@ func (v *Branch) Sanitize() error { return nil } -func (v *Branch) isBypassed(actor *types.Principal, isSpaceOwner bool) bool { +func (v *Branch) isBypassed(actor *types.Principal, isRepoOwner bool) bool { return actor != nil && (actor.Admin || - v.Bypass.SpaceOwners && isSpaceOwner || + v.Bypass.RepoOwners && isRepoOwner || slices.Contains(v.Bypass.UserIDs, actor.ID)) } diff --git a/app/services/protection/rule_branch_test.go b/app/services/protection/rule_branch_test.go index 3c8b07d6e..3299f4244 100644 --- a/app/services/protection/rule_branch_test.go +++ b/app/services/protection/rule_branch_test.go @@ -33,39 +33,39 @@ func TestBranch_isBypass(t *testing.T) { }{ { name: "empty", - bypass: DefBypass{UserIDs: nil, SpaceOwners: false}, + bypass: DefBypass{UserIDs: nil, RepoOwners: false}, actor: user, exp: false, }, { name: "admin", - bypass: DefBypass{UserIDs: nil, SpaceOwners: false}, + bypass: DefBypass{UserIDs: nil, RepoOwners: false}, actor: admin, exp: true, }, { - name: "space-owners-false", - bypass: DefBypass{UserIDs: nil, SpaceOwners: false}, + name: "repo-owners-false", + bypass: DefBypass{UserIDs: nil, RepoOwners: false}, actor: user, owner: true, exp: false, }, { - name: "space-owners-true", - bypass: DefBypass{UserIDs: nil, SpaceOwners: true}, + name: "repo-owners-true", + bypass: DefBypass{UserIDs: nil, RepoOwners: true}, actor: user, owner: true, exp: true, }, { name: "selected-false", - bypass: DefBypass{UserIDs: []int64{1, 66}, SpaceOwners: false}, + bypass: DefBypass{UserIDs: []int64{1, 66}, RepoOwners: false}, actor: user, exp: false, }, { name: "selected-true", - bypass: DefBypass{UserIDs: []int64{1, 42, 66}, SpaceOwners: false}, + bypass: DefBypass{UserIDs: []int64{1, 42, 66}, RepoOwners: false}, actor: user, exp: true, }, diff --git a/app/services/protection/verify_lifecycle.go b/app/services/protection/verify_lifecycle.go index ba14b6373..b9ca7f4a9 100644 --- a/app/services/protection/verify_lifecycle.go +++ b/app/services/protection/verify_lifecycle.go @@ -26,12 +26,12 @@ type ( } RefChangeVerifyInput struct { - Actor *types.Principal - IsSpaceOwner bool - Repo *types.Repository - RefAction RefAction - RefType RefType - RefNames []string + Actor *types.Principal + IsRepoOwner bool + Repo *types.Repository + RefAction RefAction + RefType RefType + RefNames []string } RefType int diff --git a/app/services/protection/verify_pullreq.go b/app/services/protection/verify_pullreq.go index abeed3fe7..4f1046ccc 100644 --- a/app/services/protection/verify_pullreq.go +++ b/app/services/protection/verify_pullreq.go @@ -33,7 +33,7 @@ type ( MergeVerifyInput struct { Actor *types.Principal - IsSpaceOwner bool + IsRepoOwner bool Membership *types.Membership TargetRepo *types.Repository SourceRepo *types.Repository diff --git a/web/src/components/BranchProtection/BranchProtectionForm/BranchProtectionForm.tsx b/web/src/components/BranchProtection/BranchProtectionForm/BranchProtectionForm.tsx index a7aefd7e4..ddd6d8e43 100644 --- a/web/src/components/BranchProtection/BranchProtectionForm/BranchProtectionForm.tsx +++ b/web/src/components/BranchProtection/BranchProtectionForm/BranchProtectionForm.tsx @@ -152,8 +152,7 @@ const BranchProtectionForm = (props: { const excludeArr = excludeList?.map((arr: string) => ['exclude', arr]) const finalArray = [...includeArr, ...excludeArr] const idsArray = (rule?.definition as ProtectionBranch)?.bypass?.user_ids - const filteredArray = users?.filter(user => idsArray?.includes(user.id as number)) - const resultArray = filteredArray?.map(user => `${user.id} ${user.display_name}`) + const bypassList = users?.filter(user => idsArray?.includes(user.id as number))?.map(user => `${user.id} ${user.display_name}`) return { name: rule?.uid, desc: rule.description, @@ -161,8 +160,8 @@ const BranchProtectionForm = (props: { target: '', targetDefault: (rule?.pattern as ProtectionPattern)?.default, targetList: finalArray, - allProjectOwners: (rule.definition as ProtectionBranch)?.bypass?.space_owners, - projectOwners: resultArray, + allRepoOwners: (rule.definition as ProtectionBranch)?.bypass?.repo_owners, + bypassList: bypassList, requireMinReviewers: minReviewerCheck, minReviewers: minReviewerCheck ? (rule.definition as ProtectionBranch)?.pullreq?.approvals?.require_minimum_count @@ -202,7 +201,7 @@ const BranchProtectionForm = (props: { const excludeArray = formData?.targetList?.filter(([type]) => type === 'exclude').map(([, value]) => value) ?? [] - const intArray = formData?.projectOwners?.map(item => parseInt(item.split(' ')[0])) + const bypassList = formData?.bypassList?.map(item => parseInt(item.split(' ')[0])) const payload: OpenapiRule = { uid: formData.name, type: 'branch', @@ -215,8 +214,8 @@ const BranchProtectionForm = (props: { }, definition: { bypass: { - user_ids: intArray, - space_owners: formData.allProjectOwners + user_ids: bypassList, + repo_owners: formData.allRepoOwners }, pullreq: { approvals: { @@ -253,7 +252,7 @@ const BranchProtectionForm = (props: { }}> {formik => { const targetList = formik.values.targetList - const bypassList = formik.values.projectOwners + const bypassList = formik.values.bypassList const minReviewers = formik.values.requireMinReviewers const statusChecks = formik.values.statusChecks const limitMergeStrats = formik.values.limitMergeStrategies @@ -394,22 +393,26 @@ const BranchProtectionForm = (props: { {getString('branchProtection.bypassList')} - + { - bypassList?.push(item.value as string) - const uniqueArr = Array.from(new Set(bypassList)) - formik.setFieldValue('projectOwners', uniqueArr) + const id = item.value?.toString().split(' ')[0] + const displayName = item.label + const bypassEntry = `${id} ${displayName}` - // formik.values.projectOwners.push(item.value as number) + bypassList?.push(bypassEntry) + const uniqueArr = Array.from(new Set(bypassList)) + formik.setFieldValue('bypassList', uniqueArr) }} - name={'projectOwners'}> + name={'bypassList'}> {bypassList?.map((owner, idx) => { - const name = owner.split(' ')[1] + const nameIdx = owner.indexOf(" ") + 1 + const name = owner.substring(nameIdx) + return ( @@ -423,7 +426,7 @@ const BranchProtectionForm = (props: { const filteredData = bypassList.filter( item => !(item[0] === owner[0] && item[1] === owner[1]) ) - formik.setFieldValue('projectOwners', filteredData) + formik.setFieldValue('bypassList', filteredData) }} /> @@ -444,7 +447,7 @@ const BranchProtectionForm = (props: {