From 76309b51cb29b4d872b5b9023aef74b6e2a03f19 Mon Sep 17 00:00:00 2001 From: mediocregopher <> Date: Sat, 29 Feb 2020 13:02:25 -0700 Subject: [PATCH] Refactor access controls to support multiple branches message: |- Refactor access controls to support multiple branches This was a big lift. It implements a backwards incompatible change to overhaul access control patterns to also encompass which branch is being interacted with, not only which files. The `accessctl` package was significantly rewritten to support this, as well as some of the code modifying it. The INTRODUCTION and SPEC were also appropriately updated. The change to the SPEC is _technically_ backwards incompatible, but it won't effect anything. The `access_control` previously being used will just be ignored, and the changes to `accessctl` include the definition of fallback access controls which will automatically be applied if nothing else matches, so when verifying the older history of this repo those will be used. change_hash: AIfNYLmOLGpuyTiVodT3hDe9lF4E+5DbOTgSdkbjJONb credentials: - type: pgp_signature pub_key_id: 95C46FA6A41148AC body: iQIzBAABAgAdFiEEJ6tQKp6olvZKJ0lwlcRvpqQRSKwFAl5aw0sACgkQlcRvpqQRSKy7kw//UMyS/waV/tE1vntZrMbmEtFmiXPcMVNal76cjhdiF3He50qXoWG6m0qWz+arD1tbjoZml6pvU+Xt45y/Uc54DZizzz0E9azoFW0/uvZiLApftFRArZbT9GhbDs2afalyoTJx/xvQu+a2FD/zfljEWE8Zix+bwHCLojiYHHVA65HFLEt8RsH33jFyzWvS9a2yYqZXL0qrU9tdV68hazdIm1LCp+lyVV74TjwxPAZDOmNAE9l4EjIk1pgr2Qo4u2KwJqCGdVCvka8TiFFYiP7C6319ZhDMyj4m9yZsd1xGtBd9zABVBDgmzCEjt0LI3Tv35lPd2tpFBkjQy0WGcMAhwSHWSP7lxukQMCEB7og/SwtKaExiBJhf1HRO6H9MlhNSW4X1xwUgP+739ixKKUY/RcyXgZ4pkzt6sewAMVbUOcmzXdUvuyDJQ0nhDFowgicpSh1m8tTkN1aLUx18NfnGZRgkgBeE6EpT5/+NBfFwvpiQkXZ3bcMiNhNTU/UnWMyqjKlog+8Ca/7CqgswYppMaw4iPaC54H8P6JTH+XnqDlLKSkvh7PiJJa5nFDG07fqc8lYKm1KGv6virAhEsz/AYKLoNGIsqXt+mYUDLvQpjlRsiN52euxyn5e41LcrH0RidIGMVeaS+7re1pWbkCkMMMtYlnCbC5L6mfrBu6doN8o= account: mediocregopher --- .dehub/config.yml | 8 -- INTRODUCTION.md | 30 ++-- SPEC.md | 85 ++++++----- accessctl/access_control.go | 161 +++++++++++++++++---- accessctl/access_control_test.go | 232 +++++++++++++++++++------------ cmd/dehub/main.go | 21 ++- commit.go | 80 +++++------ commit_test.go | 23 +-- config.go | 4 +- repo.go | 53 ++++++- repo_test.go | 87 +++++++++++- 11 files changed, 541 insertions(+), 243 deletions(-) diff --git a/.dehub/config.yml b/.dehub/config.yml index d844302..a072e28 100644 --- a/.dehub/config.yml +++ b/.dehub/config.yml @@ -4,11 +4,3 @@ accounts: signifiers: - type: pgp_public_key_file path: ".dehub/mediocregopher.asc" - -access_controls: - - pattern: "**" - condition: - type: signature - account_ids: - - mediocregopher - count: 100% diff --git a/INTRODUCTION.md b/INTRODUCTION.md index 52ff2a7..d234a18 100644 --- a/INTRODUCTION.md +++ b/INTRODUCTION.md @@ -39,9 +39,9 @@ platforms into the git history itself, including dehub's own configuration. By doing this, the server-side git component can be reduced to a mere pre-receive hook (if anything at all). This opens the door for much more -lightweight and flexible hosting of git projects, and even more radical -solutions like hosting git projects on completely decentralized platforms like -IPFS. +lightweight and flexible hosting of git projects, as well as even more radical +solutions; dehub can enable hosting git projects on completely decentralized +platforms like IPFS. ### Example @@ -53,16 +53,17 @@ equivalent of the `master` branch). MyProject's repo would contain a ``` # ... access_controls: - - # matches all files, but could be used for more fine-grained control - - pattern: "**" - condition: - type: signature - account_ids: - - alice - - bob - - carol - count: 2 + - branch_pattern: trunk + change_access_controls: + # matches all files, but could be used for more fine-grained control + - file_path_pattern: "**" + condition: + type: signature + account_ids: + - alice + - bob + - carol + count: 2 ``` A commit in the `trunk` branch would have a message with the following form: @@ -102,8 +103,7 @@ needed to verify commits in `trunk` when they are pushed or pulled. ## dehub Thread Branches -The `trunk` branch is the project's source-of-truth; all commits in it must have -dehub encoded message bodies with acceptable credentials. Other branches, called +The `trunk` branch is the project's source-of-truth. Other branches, called threads, are used to coordinate new changes, and then coalesce those changes into a commit suitable for `trunk`. diff --git a/SPEC.md b/SPEC.md index d6b1ce7..8b71bf6 100644 --- a/SPEC.md +++ b/SPEC.md @@ -28,41 +28,60 @@ accounts: - type: "keybase" user: "some_keybase_user_id" -# access_controls defines under what conditions different files in the repo may -# be modified. For each file modified in a commit, all access control patterns -# are applied sequentially until one matches, and the associated access control -# conditions are checked. A commit is only allowed if the conditions of all -# modified files are met. +# access_controls define who may do what in the repo. The value is a list of +# access control objects, each applying to one or more potential branch names. access_controls: - # pattern is a glob pattern describing what files this access control - # applies to. Single star matches all characters except path separators, - # double star matches everything. - - pattern: ".dehub/**" - - # signature conditions indicate that a commit must be signed by one or - # more accounts to be allowed. - condition: - type: signature - - # account_ids lists all accounts whose signature will count towards - # meeting the condition - account_ids: - - some_user_id - - # count describes how many signatures are required. It can be either a - # contrete integer (e.g. 2, meaning any 2 accounts listed by - # account_ids) or a percent. - count: 100% - - # This catch-all pattern for the rest of the repo requires that changes to - # any files not under `.dehub/` are signed by at least one of the - # defined accounts. - - pattern: "**" - condition: - type: signature - any_account: true # indicates any account defined in accounts is valid - count: 1 + # branch_pattern is a glob pattern describing what branch names this access + # control applies to. The first matching branch_pattern for a branch name + # defines which access controls are applied. + - branch_pattern: trunk + + # change_access_controls is an array of possible access controls applied for + # files being changed in the branch + change_access_controls: + + # file_path_pattern is a glob pattern describing what files this access control + # applies to. Single star matches all characters except path separators, + # double star matches everything. The first matching file_path_pattern for a + # file path (relative to the repo root) defines which access controls are + # applied. + - file_path_pattern: ".dehub/**" + + # signature conditions indicate that a commit must be signed by one or + # more accounts in order to be allowed. + condition: + type: signature + + # account_ids lists all accounts whose signature will count towards + # meeting the condition + account_ids: + - some_user_id + + # count describes how many signatures are required. It can be either a + # contrete integer (e.g. 2, meaning any 2 accounts listed by + # account_ids) or a percent. + count: 100% # all accounts in account_ids must sign + + # This catch-all pattern for the rest of the repo requires that changes to + # any files not under `.dehub/` are signed by at least one of the + # defined accounts. + - file_path_pattern: "**" + condition: + type: signature + any_account: true # indicates any account defined in accounts is valid + count: 1 + + # If a branch is not matched by any access control object then the following + # default object is implied: + # + # branch_pattern: ** + # change_access_controls: + # - file_path_pattern: ** + # condition: + # type: signature + # any_account: true + # count: 1 ``` # Change Hash diff --git a/accessctl/access_control.go b/accessctl/access_control.go index 7d50428..0d1e2ed 100644 --- a/accessctl/access_control.go +++ b/accessctl/access_control.go @@ -2,52 +2,153 @@ package accessctl import ( "fmt" + "sort" "github.com/bmatcuk/doublestar" ) -// AccessControl represents an access control object being defined in the -// Config. -type AccessControl struct { - Pattern string `yaml:"pattern"` - Condition Condition `yaml:"condition"` +var ( + // DefaultChangeAccessControl represents the ChangeAccessControl which is + // applied when a changed file's path does not match any defined patterns + // within a BranchAccessControl. + DefaultChangeAccessControl = ChangeAccessControl{ + FilePathPattern: "**", + Condition: Condition{ + Signature: &ConditionSignature{ + AnyAccount: true, + Count: "1", + }, + }, + } + + // DefaultBranchAccessControls represents the BranchAccessControls which are + // applied when the name of a branch being interacted with does not match + // any defined patterns within the Config. + DefaultBranchAccessControls = []BranchAccessControl{ + // These are currently the same, but they will differ once things like + // comments start being implemented. + { + BranchPattern: "trunk", + ChangeAccessControls: []ChangeAccessControl{DefaultChangeAccessControl}, + }, + { + BranchPattern: "**", + ChangeAccessControls: []ChangeAccessControl{DefaultChangeAccessControl}, + }, + } +) + +// BranchAccessControl represents an access control object defined for the +// purpose of controlling who is able to perform what interactions with a +// branch. +type BranchAccessControl struct { + BranchPattern string `yaml:"branch_pattern"` + ChangeAccessControls []ChangeAccessControl `yaml:"change_access_controls"` +} + +// ChangeAccessControl represents an access control object being defined in the +// Config for the purpose of controlling who is able to change which files. +type ChangeAccessControl struct { + FilePathPattern string `yaml:"file_path_pattern"` + Condition Condition `yaml:"condition"` +} + +// MatchInteractions is used as an input to Match to describe all +// interactions which are being attempted on a particular Branch. +type MatchInteractions struct { + // Branch is the name of the branch the interactions are being attempted on. + // It is required. + Branch string + + // FilePathsChanged is the set of file paths (relative to the repo root) + // which have been modified in some way. + FilePathsChanged []string } -// ErrNoApplicableAccessControls is returned from ApplicableAccessControls when -// a changed path has no applicable AccessControls which match it. -type ErrNoApplicableAccessControls struct { - Path string +// MatchedChangeAccessControl contains information about a ChangeAccessControl +// which was matched in Match +type MatchedChangeAccessControl struct { + ChangeAccessControl ChangeAccessControl + + // FilePaths contains all FilePaths to which this access control was found + // to be applicable. + FilePaths []string } -func (err ErrNoApplicableAccessControls) Error() string { - return fmt.Sprintf("no AccessControls which apply to changed file %q", err.Path) +// MatchResult is the result returned from the Match method. +type MatchResult struct { + // BranchPattern indicates the BranchPattern field of the + // BranchAccessControl object which matched the inputs. + BranchPattern string + + // ChangeAccessControls indicates which ChangeAccessControl objects matched + // the files being changed. + ChangeAccessControls []MatchedChangeAccessControl } -// ApplicableAccessControls returns a subset of the given AccessControls which -// are applicable to the given file paths (ie those whose Conditions must be met -// in order for the changes to go through. -func ApplicableAccessControls(accessControls []AccessControl, filesChanged []string) ([]AccessControl, error) { - applicableSet := map[AccessControl]struct{}{} - for _, path := range filesChanged { - var any bool - for _, ac := range accessControls { - if ok, err := doublestar.PathMatch(ac.Pattern, path); err != nil { - return nil, fmt.Errorf("error matching path %q to patterrn %q: %w", - path, ac.Pattern, err) +// Match takes in a set of access controls and a set of interactions taking +// place, and returns a MatchResult describing the access controls which should +// be applied to the interactions. +func Match(accessControls []BranchAccessControl, interactions MatchInteractions) (MatchResult, error) { + var res MatchResult + + accessControls = append(accessControls, DefaultBranchAccessControls...) + + // find the applicable BranchAccessControl + var branchAC BranchAccessControl + { + var ok bool + var err error + for i := range accessControls { + ok, err = doublestar.Match(accessControls[i].BranchPattern, interactions.Branch) + if err != nil { + return res, fmt.Errorf("error matching branch %q to pattern %q: %w", + accessControls[i].BranchPattern, interactions.Branch, err) } else if ok { - applicableSet[ac] = struct{}{} - any = true + branchAC = accessControls[i] break } } - if !any { - return nil, ErrNoApplicableAccessControls{Path: path} + if !ok { + panic(fmt.Sprintf("no patterns matched branch %q, which shouldn't be possible", interactions.Branch)) } + res.BranchPattern = branchAC.BranchPattern } - applicable := make([]AccessControl, 0, len(applicableSet)) - for ac := range applicableSet { - applicable = append(applicable, ac) + // determine ChangeAccessControl for each path in FilesChanged + { + changeACs := append(branchAC.ChangeAccessControls, DefaultChangeAccessControl) + acToPaths := map[ChangeAccessControl][]string{} + for _, path := range interactions.FilePathsChanged { + var ok bool + var err error + for _, ac := range changeACs { + if ok, err = doublestar.PathMatch(ac.FilePathPattern, path); err != nil { + return res, fmt.Errorf("error matching path %q to patterrn %q: %w", + path, ac.FilePathPattern, err) + } else if ok { + acToPaths[ac] = append(acToPaths[ac], path) + break + } + } + if !ok { + panic(fmt.Sprintf("no patterns matched change path %q, which shouldn't be possible", path)) + } + } + for ac, paths := range acToPaths { + res.ChangeAccessControls = append(res.ChangeAccessControls, MatchedChangeAccessControl{ + ChangeAccessControl: ac, + FilePaths: paths, + }) + } + + // sort result for determinancy + sort.Slice(res.ChangeAccessControls, func(i, j int) bool { + pi := res.ChangeAccessControls[i].ChangeAccessControl.FilePathPattern + pj := res.ChangeAccessControls[j].ChangeAccessControl.FilePathPattern + return pi < pj + }) } - return applicable, nil + + return res, nil } diff --git a/accessctl/access_control_test.go b/accessctl/access_control_test.go index 07326ba..9f0fa85 100644 --- a/accessctl/access_control_test.go +++ b/accessctl/access_control_test.go @@ -1,117 +1,177 @@ package accessctl import ( - "errors" "reflect" - "sort" "testing" + + "github.com/davecgh/go-spew/spew" ) -func TestApplicableAccessControls(t *testing.T) { +func normalizeResult(res MatchResult) MatchResult { + if len(res.ChangeAccessControls) == 0 { + res.ChangeAccessControls = nil + } + return res +} + +func TestMatch(t *testing.T) { + secondCond := Condition{ + Signature: &ConditionSignature{ + AnyAccount: true, + Count: "2", + }, + } + tests := []struct { - descr string - patterns, filesChanged []string - exp []string - expErrPath string + descr string + + branchACs []BranchAccessControl + interactions MatchInteractions + result MatchResult }{ { - descr: "empty input empty output", - }, - { - descr: "empty patterns", - filesChanged: []string{"foo", "bar"}, - expErrPath: "foo", - }, - { - descr: "empty filesChanged", - patterns: []string{"patternA", "patternB"}, + descr: "empty input empty result", + result: MatchResult{ + BranchPattern: "**", + }, }, { - descr: "no applicable files", - filesChanged: []string{"foo"}, - patterns: []string{"bar"}, - expErrPath: "foo", + descr: "empty access controls", + interactions: MatchInteractions{ + Branch: "trunk", + FilePathsChanged: []string{"foo", "bar"}, + }, + result: MatchResult{ + BranchPattern: "trunk", + ChangeAccessControls: []MatchedChangeAccessControl{ + { + ChangeAccessControl: DefaultChangeAccessControl, + FilePaths: []string{"foo", "bar"}, + }, + }, + }, }, { - descr: "all applicable files", - filesChanged: []string{"foo", "bar"}, - patterns: []string{"**"}, - exp: []string{"**"}, + descr: "empty filesPathsChanged", + branchACs: DefaultBranchAccessControls, + interactions: MatchInteractions{Branch: "trunk"}, + result: MatchResult{BranchPattern: "trunk"}, }, { - descr: "pattern precedent", - filesChanged: []string{"foo"}, - patterns: []string{"foo", "**"}, - exp: []string{"foo"}, + descr: "no matching branch patterns", + branchACs: []BranchAccessControl{{ + BranchPattern: "dunk", + ChangeAccessControls: []ChangeAccessControl{{ + FilePathPattern: "**", + Condition: secondCond, + }}, + }}, + interactions: MatchInteractions{ + Branch: "crunk", + FilePathsChanged: []string{"foo"}, + }, + result: MatchResult{ + BranchPattern: "**", + ChangeAccessControls: []MatchedChangeAccessControl{{ + ChangeAccessControl: DefaultChangeAccessControl, + FilePaths: []string{"foo"}, + }}, + }, }, { - descr: "pattern precedent inv", - filesChanged: []string{"foo"}, - patterns: []string{"**", "foo"}, - exp: []string{"**"}, + descr: "no matching files", + branchACs: []BranchAccessControl{{ + BranchPattern: "trunk", + ChangeAccessControls: []ChangeAccessControl{{ + FilePathPattern: "boo", + Condition: secondCond, + }}, + }}, + interactions: MatchInteractions{ + Branch: "trunk", + FilePathsChanged: []string{"foo"}, + }, + result: MatchResult{ + BranchPattern: "trunk", + ChangeAccessControls: []MatchedChangeAccessControl{{ + ChangeAccessControl: DefaultChangeAccessControl, + FilePaths: []string{"foo"}, + }}, + }, }, { - descr: "individual matches", - filesChanged: []string{"foo", "bar/baz"}, - patterns: []string{"foo", "bar/baz"}, - exp: []string{"foo", "bar/baz"}, + descr: "branch pattern precedent", + branchACs: []BranchAccessControl{ + { + BranchPattern: "trunk", + ChangeAccessControls: []ChangeAccessControl{{ + FilePathPattern: "foo", + Condition: secondCond, + }}, + }, + { + BranchPattern: "**", + ChangeAccessControls: []ChangeAccessControl{ + DefaultChangeAccessControl, + }, + }, + }, + interactions: MatchInteractions{ + Branch: "trunk", + FilePathsChanged: []string{"foo"}, + }, + result: MatchResult{ + BranchPattern: "trunk", + ChangeAccessControls: []MatchedChangeAccessControl{{ + ChangeAccessControl: ChangeAccessControl{ + FilePathPattern: "foo", + Condition: secondCond, + }, + FilePaths: []string{"foo"}, + }}, + }, }, { - descr: "star match dir", - filesChanged: []string{"foo", "bar/baz"}, - patterns: []string{"foo", "bar/*"}, - exp: []string{"foo", "bar/*"}, - }, - { - descr: "star not match dir", - filesChanged: []string{"foo", "bar/baz/biz"}, - patterns: []string{"foo", "bar/*"}, - expErrPath: "bar/baz/biz", - }, - { - descr: "doublestar match dir", - filesChanged: []string{"foo", "bar/bar", "bar/baz/biz"}, - patterns: []string{"foo", "bar/**"}, - exp: []string{"foo", "bar/**"}, + descr: "multiple files matching FilePathPatterns", + branchACs: []BranchAccessControl{{ + BranchPattern: "trunk", + ChangeAccessControls: []ChangeAccessControl{{ + FilePathPattern: "foo*", + Condition: secondCond, + }}, + }}, + interactions: MatchInteractions{ + Branch: "trunk", + FilePathsChanged: []string{"foo_a", "bar", "foo_b"}, + }, + result: MatchResult{ + BranchPattern: "trunk", + ChangeAccessControls: []MatchedChangeAccessControl{ + { + ChangeAccessControl: DefaultChangeAccessControl, + FilePaths: []string{"bar"}, + }, + { + ChangeAccessControl: ChangeAccessControl{ + FilePathPattern: "foo*", + Condition: secondCond, + }, + FilePaths: []string{"foo_a", "foo_b"}, + }, + }, + }, }, } for _, test := range tests { t.Run(test.descr, func(t *testing.T) { - accessControls := make([]AccessControl, len(test.patterns)) - for i := range test.patterns { - accessControls[i] = AccessControl{Pattern: test.patterns[i]} - } - - out, err := ApplicableAccessControls(accessControls, test.filesChanged) - if err != nil && test.expErrPath == "" { - t.Fatalf("unexpected error: %v", err) - } else if test.expErrPath != "" { - if noAppErr := (ErrNoApplicableAccessControls{}); !errors.As(err, &noAppErr) { - t.Fatalf("expected ErrNoApplicableAccessControls for path %q, but got %v", test.expErrPath, err) - } else if test.expErrPath != noAppErr.Path { - t.Fatalf("expected ErrNoApplicableAccessControls for path %q, but got one for path %q", test.expErrPath, noAppErr.Path) - } - return + res, err := Match(test.branchACs, test.interactions) + if err != nil { + t.Fatalf("error matching: %v", err) } - - outPatterns := make([]string, len(out)) - for i := range out { - outPatterns[i] = out[i].Pattern - } - - clean := func(s []string) []string { - if len(s) == 0 { - return nil - } - sort.Strings(s) - return s - } - - outPatterns = clean(outPatterns) - test.exp = clean(test.exp) - if !reflect.DeepEqual(outPatterns, test.exp) { - t.Fatalf("expected: %+v\ngot: %+v", test.exp, outPatterns) + res, expRes := normalizeResult(res), normalizeResult(test.result) + if !reflect.DeepEqual(res, expRes) { + t.Fatalf("expected:%s\ngot: %s", spew.Sdump(expRes), spew.Sdump(res)) } }) } diff --git a/cmd/dehub/main.go b/cmd/dehub/main.go index 5b645c2..b133b62 100644 --- a/cmd/dehub/main.go +++ b/cmd/dehub/main.go @@ -174,6 +174,7 @@ var subCmds = []subCmd{ descr: "verifies one or more commits as having the proper credentials", body: func(sctx subCmdCtx) error { rev := sctx.flag.String("rev", "HEAD", "Revision of commit to verify") + branch := sctx.flag.String("branch", "", "Branch that the revision is on. If not given then the currently checked out branch is assumed") sctx.flagParse() h, err := sctx.repo().GitRepo.ResolveRevision(plumbing.Revision(*rev)) @@ -181,7 +182,16 @@ var subCmds = []subCmd{ return fmt.Errorf("could not resolve revision %q: %w", *rev, err) } - if err := sctx.repo().VerifyChangeCommit(*h); err != nil { + var branchName plumbing.ReferenceName + if *branch == "" { + if branchName, err = sctx.repo().CheckedOutBranch(); err != nil { + return fmt.Errorf("could not determined currently checked out branch: %w", err) + } + } else { + branchName = plumbing.NewBranchReferenceName(*branch) + } + + if err := sctx.repo().VerifyChangeCommit(branchName, *h); err != nil { return fmt.Errorf("could not verify commit at %q (%s): %w", *rev, *h, err) } @@ -209,15 +219,14 @@ var subCmds = []subCmd{ } else if err != nil { return fmt.Errorf("error reading next line from stdin: %w", err) } + fmt.Printf("Processing line %q\n", strings.TrimSpace(line)) lineParts := strings.Fields(line) if len(lineParts) < 3 { return fmt.Errorf("malformed pre-receive hook stdin line %q", line) } - if plumbing.ReferenceName(lineParts[2]) != dehub.Trunk { - return fmt.Errorf("only commits to the trunk branch are allowed at the moment (tried to push to %q)", lineParts[2]) - } + branchName := plumbing.ReferenceName(lineParts[2]) // the zeroRevision gets sent on the very first push const zeroRevision plumbing.Revision = "0000000000000000000000000000000000000000" @@ -269,8 +278,8 @@ var subCmds = []subCmd{ for i := len(hashesToCheck) - 1; i >= 0; i-- { hash := hashesToCheck[i] fmt.Printf("Verifying change commit %q\n", hash) - if err := sctx.repo().VerifyChangeCommit(hash); err != nil { - return fmt.Errorf("could not verify change commit %q", hash) + if err := sctx.repo().VerifyChangeCommit(branchName, hash); err != nil { + return fmt.Errorf("could not verify change commit %q: %w", hash, err) } } fmt.Println("All pushed commits have been verified, well done.") diff --git a/commit.go b/commit.go index e8f2548..b2a812f 100644 --- a/commit.go +++ b/commit.go @@ -116,8 +116,8 @@ func (r *Repo) HasStagedChanges() (bool, error) { return any, nil } -// NewChangeCommit constructs a ChangeCommit using the given SignifierInterface -// to create a Credential for it. +// NewChangeCommit constructs a ChangeCommit. If sig is given then it is used to +// create a Credential for the ChangeCommit. func (r *Repo) NewChangeCommit(msg, accountID string, sig sigcred.SignifierInterface) (ChangeCommit, error) { _, headTree, err := r.head() if errors.Is(err, plumbing.ErrReferenceNotFound) { @@ -131,49 +131,38 @@ func (r *Repo) NewChangeCommit(msg, accountID string, sig sigcred.SignifierInter return ChangeCommit{}, err } - // this is necessarily different than headTree for the case of there being - // no HEAD (ie it's the first commit). In that case we want headTree to be - // empty (because it's being used to generate the change hash), but we want - // the signifier to use the raw fs (because that's where the signifier's - // data might be). - sigFS, err := r.headOrRawFS() - if err != nil { - return ChangeCommit{}, err - } - - cfg, err := r.loadConfig(sigFS) - if err != nil { - return ChangeCommit{}, fmt.Errorf("could not load config: %w", err) - } - changeHash := genChangeHash(nil, msg, headTree, stagedTree) - cred, err := sig.Sign(sigFS, changeHash) - if err != nil { - return ChangeCommit{}, fmt.Errorf("failed to sign commit hash: %w", err) - } - cred.AccountID = accountID - // This isn't strictly necessary, but we want to save people the effort of - // creating an invalid commit, pushing it, having it be rejected, then - // having to reset on the commit. - err = r.assertAccessControls( - cfg.AccessControls, []sigcred.Credential{cred}, - headTree, stagedTree, - ) - if err != nil { - return ChangeCommit{}, fmt.Errorf("commit would not satisfy access controls: %w", err) + var creds []sigcred.Credential + if sig != nil { + // this is necessarily different than headTree for the case of there + // being no HEAD (ie it's the first commit). In that case we want + // headTree to be empty (because it's being used to generate the change + // hash), but we want the signifier to use the raw fs (because that's + // where the signifier's data might be). + sigFS, err := r.headOrRawFS() + if err != nil { + return ChangeCommit{}, err + } + + cred, err := sig.Sign(sigFS, changeHash) + if err != nil { + return ChangeCommit{}, fmt.Errorf("failed to sign commit hash: %w", err) + } + cred.AccountID = accountID + creds = append(creds, cred) } return ChangeCommit{ Message: msg, ChangeHash: changeHash, - Credentials: []sigcred.Credential{cred}, + Credentials: creds, }, nil } func (r *Repo) assertAccessControls( - accessCtls []accessctl.AccessControl, creds []sigcred.Credential, - from, to *object.Tree, + accessCtls []accessctl.BranchAccessControl, creds []sigcred.Credential, + branch plumbing.ReferenceName, from, to *object.Tree, ) error { filesChanged, err := calcDiff(from, to) if err != nil { @@ -185,18 +174,23 @@ func (r *Repo) assertAccessControls( pathsChanged[i] = filesChanged[i].path } - accessCtls, err = accessctl.ApplicableAccessControls(accessCtls, pathsChanged) + matchRes, err := accessctl.Match(accessCtls, accessctl.MatchInteractions{ + Branch: branch.Short(), + FilePathsChanged: pathsChanged, + }) if err != nil { return fmt.Errorf("could not determine applicable access controls: %w", err) } - for _, accessCtl := range accessCtls { - condInt, err := accessCtl.Condition.Interface() + for _, matchedAC := range matchRes.ChangeAccessControls { + ac := matchedAC.ChangeAccessControl + condInt, err := ac.Condition.Interface() if err != nil { - return fmt.Errorf("could not cast Condition to interface: %w", err) + return fmt.Errorf("could not cast Condition of file path pattern %q to interface: %w", + ac.FilePathPattern, err) } else if err := condInt.Satisfied(creds); err != nil { - return fmt.Errorf("access control for pattern %q not satisfied: %w", - accessCtl.Pattern, err) + return fmt.Errorf("access control of file path pattern %q not satisfied: %w\nFiles matched:\n%s", + ac.FilePathPattern, err, strings.Join(matchedAC.FilePaths, "\n")) } } @@ -204,8 +198,8 @@ func (r *Repo) assertAccessControls( } // VerifyChangeCommit verifies that the change commit at the given hash, which -// is presumably on the repo trunk, is gucci. -func (r *Repo) VerifyChangeCommit(h plumbing.Hash) error { +// is presumably on the given branch, is gucci. +func (r *Repo) VerifyChangeCommit(branch plumbing.ReferenceName, h plumbing.Hash) error { commit, err := r.GitRepo.CommitObject(h) if err != nil { return fmt.Errorf("could not retrieve commit object: %w", err) @@ -241,7 +235,7 @@ func (r *Repo) VerifyChangeCommit(h plumbing.Hash) error { err = r.assertAccessControls( cfg.AccessControls, changeCommit.Credentials, - parentTree, commitTree, + branch, parentTree, commitTree, ) if err != nil { return fmt.Errorf("failed to satisfy all access controls: %w", err) diff --git a/commit_test.go b/commit_test.go index 632f484..859ed4e 100644 --- a/commit_test.go +++ b/commit_test.go @@ -1,14 +1,13 @@ package dehub import ( - "dehub/accessctl" "dehub/sigcred" - "errors" "reflect" "strings" "testing" "github.com/davecgh/go-spew/spew" + "gopkg.in/src-d/go-git.v4" "gopkg.in/src-d/go-git.v4/plumbing" yaml "gopkg.in/yaml.v2" ) @@ -82,7 +81,7 @@ func TestChangeCommitVerify(t *testing.T) { account := h.cfg.Accounts[0] changeCommit, hash := h.changeCommit(step.msg, account.ID, h.sig) - if err := h.repo.VerifyChangeCommit(hash); err != nil { + if err := h.repo.VerifyChangeCommit(TrunkRefName, hash); err != nil { t.Fatalf("could not verify hash %v: %v", hash, err) } @@ -119,8 +118,8 @@ func TestConfigChange(t *testing.T) { _, hash := h.changeCommit("commit configuration", h.cfg.Accounts[0].ID, h.sig) hashes = append(hashes, hash) - // create a new account and add it to the configuration. It should not be - // able to actually make that commit though. + // create a new account and add it to the configuration. That commit should + // not be verifiable, though newSig, newPubKeyBody := sigcred.SignifierPGPTmp(h.rand) h.cfg.Accounts = append(h.cfg.Accounts, Account{ ID: "toot", @@ -128,21 +127,23 @@ func TestConfigChange(t *testing.T) { Body: string(newPubKeyBody), }}}, }) - h.cfg.AccessControls[0].Condition.Signature.AccountIDs = []string{"root", "toot"} - h.cfg.AccessControls[0].Condition.Signature.Count = "1" + h.cfg.AccessControls[0].ChangeAccessControls[0].Condition.Signature.AccountIDs = []string{"root", "toot"} + h.cfg.AccessControls[0].ChangeAccessControls[0].Condition.Signature.Count = "1" cfgBody, err := yaml.Marshal(h.cfg) if err != nil { t.Fatal(err) } h.stage(map[string]string{ConfigPath: string(cfgBody)}) + _, badHash := h.changeCommit("add toot user", h.cfg.Accounts[1].ID, newSig) - _, err = h.repo.NewChangeCommit("add toot user", h.cfg.Accounts[1].ID, newSig) - if aclErr := (accessctl.ErrConditionSignatureUnsatisfied{}); !errors.As(err, &aclErr) { - t.Fatalf("NewChangeCommit should have returned an ErrConditionSignatureUnsatisfied, but returned %v", err) + if err := h.repo.VerifyChangeCommit(TrunkRefName, badHash); err == nil { + t.Fatal("toot user shouldn't be able to add itself to config") } + h.reset(hash, git.HardReset) // now add with the root user, this should work. + h.stage(map[string]string{ConfigPath: string(cfgBody)}) _, hash = h.changeCommit("add toot user", h.cfg.Accounts[0].ID, h.sig) hashes = append(hashes, hash) @@ -152,7 +153,7 @@ func TestConfigChange(t *testing.T) { hashes = append(hashes, hash) for i, hash := range hashes { - if err := h.repo.VerifyChangeCommit(hash); err != nil { + if err := h.repo.VerifyChangeCommit(TrunkRefName, hash); err != nil { t.Fatalf("commit %d (%v) should have been verified but wasn't: %v", i, hash, err) } } diff --git a/config.go b/config.go index 62901c4..c95209c 100644 --- a/config.go +++ b/config.go @@ -20,8 +20,8 @@ type Account struct { // Config represents the structure of the main dehub configuration file, and is // used to marshal/unmarshal the yaml file. type Config struct { - Accounts []Account `yaml:"accounts"` - AccessControls []accessctl.AccessControl `yaml:"access_controls"` + Accounts []Account `yaml:"accounts"` + AccessControls []accessctl.BranchAccessControl `yaml:"access_controls"` } func (r *Repo) loadConfig(fs fs.FS) (Config, error) { diff --git a/repo.go b/repo.go index c5f8e13..73a8d27 100644 --- a/repo.go +++ b/repo.go @@ -12,6 +12,7 @@ import ( "gopkg.in/src-d/go-git.v4" "gopkg.in/src-d/go-git.v4/plumbing" "gopkg.in/src-d/go-git.v4/plumbing/object" + "gopkg.in/src-d/go-git.v4/storage" "gopkg.in/src-d/go-git.v4/storage/memory" ) @@ -25,8 +26,11 @@ var ( // ConfigPath defines the expected path to the Repo's configuration file. ConfigPath = filepath.Join(DehubDir, "config.yml") - // Trunk defines the reference name of the trunk branch. - Trunk = plumbing.ReferenceName("refs/heads/trunk") + // Trunk defines the name of the trunk branch. + Trunk = "trunk" + + // TrunkRefName defines the reference name of the trunk branch. + TrunkRefName = plumbing.NewBranchReferenceName(Trunk) ) type repoOpts struct { @@ -48,6 +52,7 @@ func OpenBare(bare bool) OpenOption { // Repo is an object which allows accessing and modifying the dehub repo. type Repo struct { GitRepo *git.Repository + Storer storage.Storer } // OpenRepo opens the dehub repo in the given directory and returns the object @@ -69,6 +74,7 @@ func OpenRepo(path string, options ...OpenOption) (*Repo, error) { if r.GitRepo, err = git.PlainOpenWithOptions(path, openOpts); err != nil { return nil, fmt.Errorf("could not open git repo: %w", err) } + return &r, nil } @@ -79,7 +85,19 @@ func InitMemRepo() *Repo { panic(err) } - return &Repo{GitRepo: r} + repo := &Repo{GitRepo: r} + if err := repo.init(); err != nil { + panic(err) + } + return repo +} + +func (r *Repo) init() error { + h := plumbing.NewSymbolicReference(plumbing.HEAD, TrunkRefName) + if err := r.GitRepo.Storer.SetReference(h); err != nil { + return fmt.Errorf("could not set HEAD to %q: %w", TrunkRefName, err) + } + return nil } func (r *Repo) billyFilesystem() (billy.Filesystem, error) { @@ -90,6 +108,35 @@ func (r *Repo) billyFilesystem() (billy.Filesystem, error) { return w.Filesystem, nil } +// CheckedOutBranch returns the name of the currently checked out branch. +func (r *Repo) CheckedOutBranch() (plumbing.ReferenceName, error) { + // Head() can't be used for this, because it doesn't handle the case of a + // newly initialized repo very well. + ogRef, err := r.GitRepo.Storer.Reference(plumbing.HEAD) + if err != nil { + return "", fmt.Errorf("couldn't de-reference HEAD (is it a bare repo?): %w", err) + } + + ref := ogRef + for { + if ref.Type() != plumbing.SymbolicReference { + break + } + + target := ref.Target() + if target.IsBranch() { + return target, nil + } + + ref, err = r.GitRepo.Storer.Reference(target) + if err != nil { + break + } + } + + return "", fmt.Errorf("could not de-reference HEAD to a branch: %w", err) +} + func (r *Repo) head() (*object.Commit, *object.Tree, error) { head, err := r.GitRepo.Head() if err != nil { diff --git a/repo_test.go b/repo_test.go index 8e4a424..a988f4d 100644 --- a/repo_test.go +++ b/repo_test.go @@ -10,6 +10,7 @@ import ( "runtime/debug" "testing" + "gopkg.in/src-d/go-git.v4" "gopkg.in/src-d/go-git.v4/plumbing" yaml "gopkg.in/yaml.v2" ) @@ -34,13 +35,18 @@ func newHarness(t *testing.T) *harness { Path: pubKeyPath, }}}, }}, - AccessControls: []accessctl.AccessControl{ + AccessControls: []accessctl.BranchAccessControl{ { - Pattern: "**", - Condition: accessctl.Condition{ - Signature: &accessctl.ConditionSignature{ - AccountIDs: []string{"root"}, - Count: "100%", + BranchPattern: "**", + ChangeAccessControls: []accessctl.ChangeAccessControl{ + { + FilePathPattern: "**", + Condition: accessctl.Condition{ + Signature: &accessctl.ConditionSignature{ + AccountIDs: []string{"root"}, + Count: "100%", + }, + }, }, }, }, @@ -115,6 +121,21 @@ func (h *harness) changeCommit(msg, accountID string, sig sigcred.SignifierInter return tc, hash } +func (h *harness) reset(to plumbing.Hash, mode git.ResetMode) { + w, err := h.repo.GitRepo.Worktree() + if err != nil { + h.t.Fatal(err) + } + + err = w.Reset(&git.ResetOptions{ + Commit: to, + Mode: mode, + }) + if err != nil { + h.t.Fatal(err) + } +} + func TestHasStagedChanges(t *testing.T) { harness := newHarness(t) assertHasStaged := func(expHasStaged bool) { @@ -141,3 +162,57 @@ func TestHasStagedChanges(t *testing.T) { harness.changeCommit("second commit", "root", harness.sig) assertHasStaged(false) } + +// TestOldConfig tests that having an older, now malformed, Config doesn't mess +// with the current parsing, as long as the default access controls still work. +func TestOldConfig(t *testing.T) { + harness := newHarness(t) + + // overwrite the currently staged config file with an older form + harness.stage(map[string]string{ConfigPath: ` +--- +accounts: + - id: root + signifiers: + - type: pgp_public_key_file + path: ".dehub/root.asc" + +access_controls: + - pattern: "**" + condition: + type: signature + account_ids: + - root + count: 0 +`}) + _, hash0 := harness.changeCommit("first commit, this is going great", "root", harness.sig) + + // even though that access_controls doesn't actually require any signatures, + // it should be used because it's not well formed. + harness.stage(map[string]string{"foo": "no rules!"}) + _, hash1 := harness.changeCommit("ain't no laws", "toot", nil) + + // verifying the first should work, but not the second. + if err := harness.repo.VerifyChangeCommit(TrunkRefName, hash0); err != nil { + t.Fatalf("first commit %q should be verifiable, but got: %v", hash0, err) + } else if err := harness.repo.VerifyChangeCommit(TrunkRefName, hash1); err == nil { + t.Fatalf("second commit %q should not have been verified", hash1) + } + + // reset back to hash0 + harness.reset(hash0, git.HardReset) + + // make a commit fixing the config. everything should still be fine. + harness.stage(map[string]string{ConfigPath: ` +--- +accounts: + - id: root + signifiers: + - type: pgp_public_key_file + path: ".dehub/root.asc" +`}) + _, hash2 := harness.changeCommit("Fix the config!", "root", harness.sig) + if err := harness.repo.VerifyChangeCommit(TrunkRefName, hash2); err != nil { + t.Fatalf("config fix commit %q should be verifiable, but got: %v", hash2, err) + } +}