Perforce paths should start with a forward slash by pjlast · Pull Request #41972 · sourcegraph/sourcegraph-public-snapshot

pjlast · 2022-09-23T06:02:40Z

Solves #41927

There is a bug in the gobwas/glob package we use where the file something.java cannot be matched to the path **/something.java. This is because **/something.java get's converted to a suffix of /something.java, which != something.java.

One solution is to prepend all file checks with a /, however this breaks another case:
We store all paths without forward slashes. So a path like dev/test/** then becomes unmatchable to /dev/test/some_test.java, due to the difference in forward slash at the front.

By ensuring that all paths and file matches start with a forward slash (i.e. being explicit about the root directory), this solves both cases.

Test plan

Updated unit tests. Added new unit tests

sourcegraph-bot · 2022-09-23T06:04:35Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff f32c06d...3fc101d.

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-23T06:14:16Z

+				if !strings.HasPrefix(rule, "/") {
+					rule = "/" + rule
+				}


This is so that existing path rules get a leading slash when compiled (if they don't have one), so that we don't have to worry about migrations.

I think you can add a code comment here that explains this (because sooner or later someone will come along and think "don't we add these slashes when writing PathIncludes?")

pjlast · 2022-09-23T06:15:25Z

 	// see one level of the tree at a time so we have no way of knowing which path leads
 	// to a file the user is allowed to see.
-	if strings.HasPrefix(rule, "*") {
+	if strings.HasPrefix(rule, "*") || strings.HasPrefix(rule, "/*") {


We just have to check for both cases here, as we cannot be sure where this function is called (It's also possible to do the prefix adding above here)

Can we instead make sure that the rule string starts with / as we do in other places? Would make the code follow the same convention everywhere.

Yeah, I think that's better (hence my comment above). Only thought of it after working through my own PR. But now that I have a second opinion feeling the same, I'll add it

pjlast · 2022-09-23T07:53:10Z

 	for {
 		lastSlash := strings.LastIndex(rule, "/")
-		if lastSlash == -1 {
+		if lastSlash <= 0 {


Since all rules start with a slash now, the lastSlash index will never be -1, so we check for <= 0 instead. (i.e. it used to break on something.java, now it needs to break on /something.java

mrnugget

Leaving final approval to everyone else, since I still miss too much context, but on the surface this looks good!

mrnugget · 2022-09-23T07:59:46Z

+				if !strings.HasPrefix(rule, "/") {
+					rule = "/" + rule
+				}


I think you can add a code comment here that explains this (because sooner or later someone will come along and think "don't we add these slashes when writing PathIncludes?")

kopancek · 2022-09-23T08:32:17Z

-			parsedLine.isExclusion = true           // is an exclusion
-			parsedLine.match = parsedLine.match[1:] // trim leading -
+			parsedLine.isExclusion = true                                // is an exclusion
+			parsedLine.match = strings.TrimPrefix(parsedLine.match, "-") // trim leading -


+1 for this change, love that it makes the code more explicit in intentions.

kopancek · 2022-09-23T08:36:49Z

 	s = strings.TrimPrefix(s, "//")
+	if !strings.HasPrefix(s, "/") {
+		s = "/" + s // make sure path starts with a '/'
+	}


Instead of trimming the slashes from prefix and then adding them later, could we do some regex magic here? Or is this performance critical?

It's pretty performance critical, especially now that we have to exhaust the entire list of rules to make sure we enforce them correctly.

But I'd say the fact that it's generally referred to as "regex magic" is the bigger deterrent for not using it, especially here where we want to be sure (and understand) what we're doing

Haha, I read "regex magic" in my email inbox, put on my Batcape to come swooping in here and point at the "no magic!" sign on the wall and you beat me to it, Petri.

In all seriousness: I like how simple this is, so my vote goes for this, even if it can look a bit redundant.

kopancek · 2022-09-23T08:39:00Z

 	// see one level of the tree at a time so we have no way of knowing which path leads
 	// to a file the user is allowed to see.
-	if strings.HasPrefix(rule, "*") {
+	if strings.HasPrefix(rule, "*") || strings.HasPrefix(rule, "/*") {


Can we instead make sure that the rule string starts with / as we do in other places? Would make the code follow the same convention everywhere.

…perms.go

ryanslade · 2022-09-23T09:23:33Z

  setSubRepositoryPermissionsForUsers(
    repository: "UmVwb3NpdG9yeTox"
-    userPermissions: [{bindID: "alice", pathIncludes: ["*"], pathExcludes: ["*_test.go"]}]
+    userPermissions: [{bindID: "alice", pathIncludes: ["/*"], pathExcludes: ["/*_test.go"]}]


Now that we always expect a / prefix, do we want to update this resolver to enforce that?

ryanslade · 2022-09-23T09:25:47Z

+read   group   Rome    *   //depot/.../something.java   ## Can read all files named 'something.java'
+read   group   Rome    *   -//depot/dev/prodA/...   ## Except files in this directory
+`,
+			cannotReadAny: []string{"dev/prodA/something.java", "dev/prodA/another_dir/something.java"},


Can we also confirm that it doesn't match these paths with leading slahes? For example /dev/prodA/something.java

camdencheek · 2022-09-23T14:02:57Z

There is a bug in the gobwas/glob package we use

Should we open a patch for this upstream?

pjlast · 2022-09-23T14:17:26Z

@camdencheek we could try. At this point I'm not 100% sure if it's a bug, or a misunderstanding (either from us, or from the package maintainer), because the package seems very solid with a vast array of test cases.

I can open an issue and see if they reply

Add a forward slash infront of perforce paths

ff8b897

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

github-actions Bot added the team/iam label Sep 23, 2022

Add additional test cases

0f371fb

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

pjlast commented Sep 23, 2022

View reviewed changes

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

Perforce: paths should start with a forward slash #41927

Closed

pjlast self-assigned this Sep 23, 2022

pjlast added 2 commits September 23, 2022 09:11

Remove incorrect comment

b8ffb60

Handle edge case for zero-index slash

50ae3f5

pjlast commented Sep 23, 2022

View reviewed changes

mrnugget approved these changes Sep 23, 2022

View reviewed changes

Add comment explaining slash prepend of rule

b3b64ce

kopancek approved these changes Sep 23, 2022

View reviewed changes

Have rule leading slash follow convention in internal/authz/sub_repo_…

2b228a0

…perms.go

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 11:31

Have resolver enforce slash prefix

b74f2e0

Have perforce permissions enforced using the new column (#41983)

3fc101d

pjlast merged commit 7d3ab17 into pjlast/41781-create-database-migrations Sep 23, 2022

pjlast deleted the pjlast/41927-perforce-paths-should-start-with-a-forward-slash branch September 23, 2022 14:29

sashaostrikov pushed a commit that referenced this pull request Sep 29, 2022

Perforce paths should start with a forward slash (#41972)

2537e30

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

mrnugget 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!

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!

camdencheek commented Sep 23, 2022

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pjlast commented Sep 23, 2022 •

edited

Loading

sourcegraph-bot commented Sep 23, 2022 •

edited

Loading

pjlast commented Sep 23, 2022 •

edited

Loading