Have perforce permissions enforced using the new column by pjlast · Pull Request #41983 · sourcegraph/sourcegraph-public-snapshot

pjlast · 2022-09-23T09:06:10Z

This PR makes all Perforce permissions application logic use the new paths column in the sub_repo_permissions table. We have to iterate through the entire permissions list, and preference is given to the last applicable rule (as opposed to exclusions taking preference in the past).

Test plan

Added new test cases, updated test cases.

…-slash' into pjlast/41788-perforce-permissions-use-paths-column

sourcegraph-bot · 2022-09-23T09:10:08Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff b74f2e0...170becc.

Notify	File(s)
@beyang	internal/search/symbol/symbol_test.go
@camdencheek	internal/search/symbol/symbol_test.go
@indradhanush	enterprise/cmd/repo-updater/internal/authz/perms_syncer_test.go
@keegancsmith	internal/search/symbol/symbol_test.go
@unknwon	enterprise/cmd/frontend/internal/authz/resolvers/resolver.go enterprise/cmd/frontend/internal/authz/resolvers/resolver_test.go enterprise/cmd/repo-updater/internal/authz/perms_syncer_test.go enterprise/internal/authz/perforce/cmd/scanprotects/main.go enterprise/internal/authz/perforce/cmd/scanprotects/main_test.go enterprise/internal/authz/perforce/perforce_test.go enterprise/internal/authz/perforce/protects.go enterprise/internal/authz/perforce/protects_test.go internal/authz/iface.go internal/authz/sub_repo_perms.go internal/authz/sub_repo_perms_test.go

pjlast · 2022-09-23T09:24:04Z

+							Paths:        []string{"/**", "-/dev/*"},
+							PathIncludes: []string{"**"},


These have to be double stars. With just "*" as an include rule, anything in the /dev/ path could never have been matched anyways

pjlast · 2022-09-23T09:33:06Z

+		paths := make([]string, 0, len(perm.PathIncludes)+len(perm.PathExcludes))
+		for _, include := range perm.PathIncludes {
+			if !strings.HasPrefix(include, "/") { // ensure leading slash
+				include = "/" + include
+			}
+			paths = append(paths, include)
+		}
+		for _, exclude := range perm.PathExcludes {
+			if !strings.HasPrefix(exclude, "/") { // ensure leading slash
+				exclude = "/" + exclude
+			}
+			paths = append(paths, "-"+exclude) // excludes start with a minus (-)
+		}


Should we keep the current resolver, mark it as deprecated, and introduce a new endpoint that only takes Paths?

The other option is to change the current endpoint, but I fear this could break some existing functionality if someone was relying on it.

I think we should just add the new fields and deprecate the old fields but not remove them.

We may also want to document that the rules are enforced in order now.

kopancek · 2022-09-23T09:33:47Z

 						"sample": {
-							PathIncludes: []string{"*"},
+							Paths:        []string{"/**", "-/dev/*"},
+							PathIncludes: []string{"**"},
 							PathExcludes: []string{"/dev/*"},
 						},


What is the order of the rules if we still separate PathIncludes from PathExcludes in this definition?

With PathIncludes and PathExcludes, excludes take preference. So for all conversions to only Paths, excludes are appended at the end so that they still take preference (by being the last applicable rule)

…-slash' into pjlast/41788-perforce-permissions-use-paths-column

ryanslade

Nice, I've gone through it a couple of times and LGTM!

keegancsmith · 2022-09-23T11:07:02Z

+		// Iterate through all rules for the current path, and the final match takes
+		// preference.


drive-by: can't you just go through the rules backwards and then break early? Or even keep the old implementation, but just reverse the list when perforce hands it over?

I doubt this changes the benchmark, but there is one for this code path. May be worth running and comparing just to ensure there are no surprises in this new impl.

I recall thinking about this, but can't remember why I didn't go through with it. Definitely worth it, even if only out of principle. But the numbers will be interesting.

I will report back.

Storing it in reverse is probably best, as it removes an additional step during runtime, as long as it doesn't confuse anyone 🤔

I also discarded the idea of working backwards... but now that I think it through again it seems to make sense. It's an easy change to make and we have some decent test coverage now which should help us with confidence.

Just to confirm what we're thinking about:

Walk the list of rules in reverse and the first match gives us our state. So if the fist match in the reversed list is an include rule, the path is included. If exclude rule, the path is excluded. If no matches, we default to exclude as the current code does.

I think we should store in the original order and the for loop that iterates over it can be in reverse.

keegancsmith · 2022-09-23T11:20:22Z

as long as it doesn't confuse anyone 🤔

Maybe move the matching logic into the compiledRules struct and give a lovely long doc string explaining what it means to match a rule. Even better, have a well defined and typed meaning for the rules stored in the DB, that perforce ensures it matches. Right now its a little bit unsettling that when we fetch rules from the provider, we are normalizing them/interpreting them as exclusions/etc. I would hope that happens at another layer TBH. But I don't wanna derail stuff, can be follow up work/discussion.

…as well, but check that either both of them are set, or paths is set

ryanslade · 2022-09-23T13:02:07Z

+		var pathExcludes []string
+		if perm.Paths == nil {
+			paths = make([]string, 0, len(*perm.PathIncludes)+len(*perm.PathExcludes))
+			pathIncludes = make([]string, 0, len(*perm.PathIncludes))


Not sure if this might panic if perm.PathIncludes in nil? Same for the next line

PathIncludes cannot be nil at this point, there is a check above this that would have caught it

ryanslade · 2022-09-23T13:04:51Z

 		BindID       string
-		PathIncludes []string
-		PathExcludes []string
+		PathIncludes *[]string


Do these have to be pointers? A slice is already a pointer and can be nil

I honestly did not know this. This should simplify things somewhat, thanks!

GraphQL does not seem to agree

Yeah, our GraphQL uses pointers for "optional" params or nullable return values.

ryanslade

LGTM, just a couple of questions about possible nil panics

) Remove all references to PathIncludes and PathExcludes

Have perforce permissions enforced using the new column

8b89095

cla-bot Bot added the cla-signed label Sep 23, 2022

Merge branch 'pjlast/41927-perforce-paths-should-start-with-a-forward…

9a986a7

…-slash' into pjlast/41788-perforce-permissions-use-paths-column

pjlast linked an issue Sep 23, 2022 that may be closed by this pull request

Perforce: change permission applications to use paths column #41788

Closed

pjlast self-assigned this Sep 23, 2022

Update sub_repo_perms.go

b255b10

pjlast requested review from a team, mrnugget, ryanslade and sashaostrikov September 23, 2022 09:17

pjlast commented Sep 23, 2022

View reviewed changes

kopancek reviewed Sep 23, 2022

View reviewed changes

pjlast added 2 commits September 23, 2022 11:35

Merge branch 'pjlast/41927-perforce-paths-should-start-with-a-forward…

6628a30

…-slash' into pjlast/41788-perforce-permissions-use-paths-column

Combine include and exclude loops

44eace7

ryanslade reviewed Sep 23, 2022

View reviewed changes

Comment thread enterprise/internal/authz/perforce/protects.go Outdated

pjlast added 2 commits September 23, 2022 12:06

Adjust tests

efb4be1

Remove redundant else

1d9f8d2

ryanslade approved these changes Sep 23, 2022

View reviewed changes

keegancsmith reviewed Sep 23, 2022

View reviewed changes

pjlast added 3 commits September 23, 2022 13:56

Add optional path field to GraphQL query. Make other fields optional …

849ff87

…as well, but check that either both of them are set, or paths is set

Traverse paths in reverse and exit early

277fbc9

Separate rule matching out to its own function with a docstring

28fe4fe

ryanslade reviewed Sep 23, 2022

View reviewed changes

ryanslade approved these changes Sep 23, 2022

View reviewed changes

pjlast added 2 commits September 23, 2022 15:18

Perforce: Remove all references to PathIncludes and PathExcludes (#41994

c0d4451

) Remove all references to PathIncludes and PathExcludes

Fix accidental logger change

e59b1db

Remove unused import

170becc

pjlast merged commit 3fc101d into pjlast/41927-perforce-paths-should-start-with-a-forward-slash Sep 23, 2022

pjlast deleted the pjlast/41788-perforce-permissions-use-paths-column branch September 23, 2022 13:57

		Paths: []string{"/*", "-/dev/"},
		PathIncludes: []string{"**"},

		// Iterate through all rules for the current path, and the final match takes
		// preference.

Conversation

pjlast commented Sep 23, 2022

Test plan

Uh oh!

sourcegraph-bot commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanslade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keegancsmith commented Sep 23, 2022 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanslade left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sourcegraph-bot commented Sep 23, 2022 •

edited

Loading