github/drone
5
0
Fork 0
mirror of https://github.com/harness/drone.git synced 2025-05-06 18:52:05 +08:00
Code Issues Packages Projects Releases Wiki Activity

[WIP] Move from space_owner to repo_admin for protection rules (#745)

Browse Source
This commit is contained in:
Johannes Batzill 2023-10-30 22:35:52 +00:00 committed by Harness
parent e0df722ce3
commit b0e519b571
20 changed files with 102 additions and 96 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
app/api/auth/repo.go
View File

@ -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

6
app/api/controller/githook/pre_receive.go
View File

@ -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)
@ -118,7 +118,7 @@ func (c *Controller) checkProtectionRules(
violations, err := protectionRules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{
Actor: &session.Principal,
IsSpaceOwner: isSpaceOwner,
IsRepoOwner: isRepoOwner,
Repo: repo,
RefAction: refAction,
RefType: refType,

6
app/api/controller/pullreq/merge.go
View File

@ -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,

4
app/api/controller/repo/commit.go
View File

@ -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
}
@ -79,7 +79,7 @@ func (c *Controller) CommitFiles(ctx context.Context,
violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{
Actor: &session.Principal,
IsSpaceOwner: isSpaceOwner,
IsRepoOwner: isRepoOwner,
Repo: repo,
RefAction: refAction,
RefType: protection.RefTypeBranch,

6
app/api/controller/repo/controller.go
View File

@ -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
}

4
app/api/controller/repo/create_branch.go
View File

@ -51,14 +51,14 @@ 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,
IsRepoOwner: isRepoOwner,
Repo: repo,
RefAction: protection.RefActionCreate,
RefType: protection.RefTypeBranch,

4
app/api/controller/repo/create_commit_tag.go
View File

@ -55,14 +55,14 @@ 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,
IsRepoOwner: isRepoOwner,
Repo: repo,
RefAction: protection.RefActionCreate,
RefType: protection.RefTypeTag,

4
app/api/controller/repo/delete_branch.go
View File

@ -46,14 +46,14 @@ 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,
IsRepoOwner: isRepoOwner,
Repo: repo,
RefAction: protection.RefActionDelete,
RefType: protection.RefTypeBranch,

4
app/api/controller/repo/delete_tag.go
View File

@ -37,14 +37,14 @@ 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,
IsRepoOwner: isRepoOwner,
Repo: repo,
RefAction: protection.RefActionDelete,
RefType: protection.RefTypeTag,

2
app/services/protection/bypass.go
View File

@ -20,7 +20,7 @@ import (
type DefBypass struct {
UserIDs []int64 `json:"user_ids,omitempty"`
SpaceOwners bool `json:"space_owners,omitempty"`
RepoOwners bool `json:"repo_owners,omitempty"`
}
func (v DefBypass) Sanitize() error {

8
app/services/protection/rule_branch.go
View File

@ -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))
}

16
app/services/protection/rule_branch_test.go
View File

@ -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,
},

2
app/services/protection/verify_lifecycle.go
View File

@ -27,7 +27,7 @@ type (
RefChangeVerifyInput struct {
Actor *types.Principal
IsSpaceOwner bool
IsRepoOwner bool
Repo *types.Repository
RefAction RefAction
RefType RefType

2
app/services/protection/verify_pullreq.go
View File

@ -33,7 +33,7 @@ type (
MergeVerifyInput struct {
Actor *types.Principal
IsSpaceOwner bool
IsRepoOwner bool
Membership *types.Membership
TargetRepo *types.Repository
SourceRepo *types.Repository

37
web/src/components/BranchProtection/BranchProtectionForm/BranchProtectionForm.tsx
View File

@ -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: {
<Text className={css.headingSize} padding={{ bottom: 'medium' }} font={{ variation: FontVariation.H4 }}>
{getString('branchProtection.bypassList')}
</Text>
<FormInput.CheckBox label={getString('branchProtection.allProjectOwners')} name={'allProjectOwners'} />
<FormInput.CheckBox label={getString('branchProtection.allRepoOwners')} name={'allRepoOwners'} />
<FormInput.Select
items={userOptions}
onQueryChange={setSearchTerm}
className={css.widthContainer}
onChange={item => {
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'}></FormInput.Select>
name={'bypassList'}></FormInput.Select>
<Container className={cx(css.widthContainer, css.bypassContainer)}>
{bypassList?.map((owner, idx) => {
const name = owner.split(' ')[1]
const nameIdx = owner.indexOf(" ") + 1
const name = owner.substring(nameIdx)
return (
<Layout.Horizontal key={`${name}-${idx}`} flex={{ align: 'center-center' }} padding={'small'}>
<Avatar hoverCard={false} size="small" name={name.toString()} />
@ -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)
}}
/>
</Layout.Horizontal>
@ -444,7 +447,7 @@ const BranchProtectionForm = (props: {
<Layout.Horizontal spacing="small">
<Button
type="submit"
text={editMode ? getString('branchProtection.editRule') : getString('branchProtection.createRule')}
text={editMode ? getString('branchProtection.saveRule') : getString('branchProtection.createRule')}
variation={ButtonVariation.PRIMARY}
disabled={false}
/>

3
web/src/framework/strings/stringTypes.ts
View File

@ -44,7 +44,7 @@ export interface StringsMap {
branchDoesNotHaveFile: string
branchName: string
branchNotFound: string
'branchProtection.allProjectOwners': string
'branchProtection.allRepoOwners': string
'branchProtection.autoDeleteText': string
'branchProtection.autoDeleteTitle': string
'branchProtection.blockBranchCreation': string
@ -86,6 +86,7 @@ export interface StringsMap {
'branchProtection.ruleDeleted': string
'branchProtection.ruleEmpty': string
'branchProtection.ruleUpdated': string
'branchProtection.saveRule': string
'branchProtection.statusCheck': string
'branchProtection.targetBranches': string
'branchProtection.targetPlaceholder': string

3
web/src/i18n/strings.en.yaml
View File

@ -859,7 +859,7 @@ branchProtection:
defaultBranch: Default branch
bypassList: Bypass List
newBranchProtectionRule: New Branch Protection Rule
allProjectOwners: All project owners
allRepoOwners: Allow users with edit permission on the repository to bypass
protectionSelectAll: 'Protections: Select all that apply'
requireMinReviewersTitle: Require a minimum number of reviewers
requireMinReviewersContent: Require approval on pull requests from a minimum number of reviewers
@ -883,6 +883,7 @@ branchProtection:
blockBranchDeletion: Block branch deletion
blockBranchDeletionText: Only allow users with bypass permission to delete matching branches
editRule: Edit Rule
saveRule: Save Rule
deleteRule: Delete Rule
ruleDeleted: Rule Deleted
ruleEmpty: There are no rules in your repo. Click the button below to create a rule.

2
web/src/services/code/index.tsx
View File

@ -470,7 +470,7 @@ export interface ProtectionDefApprovals {
}
export interface ProtectionDefBypass {
space_owners?: boolean
repo_owners?: boolean
user_ids?: number[]
}

2
web/src/services/code/swagger.yaml
View File

@ -7557,7 +7557,7 @@ components:
type: object
ProtectionDefBypass:
properties:
space_owners:
repo_owners:
type: boolean
user_ids:
items:

4
web/src/utils/Utils.ts
View File

@ -208,8 +208,8 @@ export const rulesFormInitialPayload = {
target: '',
targetDefault: true,
targetList: [] as string[][],
allProjectOwners: false,
projectOwners: [] as string[],
allRepoOwners: false,
bypassList: [] as string[],
requireMinReviewers: false,
minReviewers: '',
requireCodeOwner: false,