[Backport 5.1] [permissions] Revert default behaviour for explicit permissions

CHANGELOG.md

@@ -26,7 +26,7 @@ All notable changes to Sourcegraph are documented in this file.
 
 ### Fixed
 
--
+- Fixed the default behaviour when the explicit permissions API is enabled. Repositories are no longer marked as unrestricted by default. [#54419](https://github.com/sourcegraph/sourcegraph/pull/54419)
 
 ### Removed
 

doc/admin/permissions/index.md

@@ -112,7 +112,7 @@ of `alice` are the following union set: [`horsegraph/global`, `horsegraph/hay-v1
 1. Go to **Site Admin > Migrations** page. There is a migration called `Migrate data from user_permissions table to unified user_repo_permissions.`. 
 Make sure that it finished migrating all the data (it reports as 100%). Contact support if the migration does not seem to complete for a long time (multiple days). 
 
-1. Enable the experimental feature in the [site configuration](../config/site_config.md):
+1. (Not required for Sourcegraph 5.1+) Enable the experimental feature in the [site configuration](../config/site_config.md):
 ```json
 {
   "experimentalFeatures": {
@@ -122,8 +122,6 @@ Make sure that it finished migrating all the data (it reports as 100%). Contact
 }
 ```
 1. Continue [configuring the explicit permissions API](api.md#configuration) as you would before. 
-Both mechanisms work at the same time, thanks to a new behind the scenes data model that allows 
-what was previously impossible. 
 
 ### Permission updates
 

internal/database/external_services.go

@@ -18,6 +18,7 @@ import (
 	"github.com/sourcegraph/log"
 
 	"github.com/sourcegraph/sourcegraph/cmd/frontend/envvar"
+	"github.com/sourcegraph/sourcegraph/cmd/frontend/globals"
 	"github.com/sourcegraph/sourcegraph/internal/conf"
 	"github.com/sourcegraph/sourcegraph/internal/database/basestore"
 	"github.com/sourcegraph/sourcegraph/internal/database/dbutil"
@@ -1578,8 +1579,12 @@ func (e *externalServiceStore) recalculateFields(es *types.ExternalService, rawC
 	// For existing auth providers, this is forwards compatible. While at the same time if they also
 	// wanted to get on the `enforcePermissions` pattern, this change is backwards compatible.
 	enforcePermissions := gjson.Get(rawConfig, "enforcePermissions")
-	if !envvar.SourcegraphDotComMode() && enforcePermissions.Exists() {
-		es.Unrestricted = !enforcePermissions.Bool()
+	if !envvar.SourcegraphDotComMode() {
+		if globals.PermissionsUserMapping().Enabled {
+			es.Unrestricted = false
+		} else if enforcePermissions.Exists() {
+			es.Unrestricted = !enforcePermissions.Bool()
+		}
 	}
 
 	hasWebhooks := false

internal/database/external_services_test.go

@@ -21,13 +21,15 @@ import (
 	"github.com/sourcegraph/log"
 
 	"github.com/sourcegraph/sourcegraph/internal/api"
+	"github.com/sourcegraph/sourcegraph/internal/jsonc"
 	"github.com/sourcegraph/sourcegraph/internal/timeutil"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 	"github.com/sourcegraph/sourcegraph/lib/pointers"
 
 	"github.com/sourcegraph/log/logtest"
 
 	"github.com/sourcegraph/sourcegraph/cmd/frontend/envvar"
+	"github.com/sourcegraph/sourcegraph/cmd/frontend/globals"
 	"github.com/sourcegraph/sourcegraph/internal/actor"
 	"github.com/sourcegraph/sourcegraph/internal/conf"
 	"github.com/sourcegraph/sourcegraph/internal/database/basestore"
@@ -2389,6 +2391,61 @@ func TestConfigurationHasWebhooks(t *testing.T) {
 	})
 }
 
+func TestExternalServiceStore_recalculateFields(t *testing.T) {
+	tests := map[string]struct {
+		explicitPermsEnabled bool
+		authorizationSet     bool
+		expectUnrestricted   bool
+	}{
+		"default state": {
+			expectUnrestricted: true,
+		},
+		"explicit perms set": {
+			explicitPermsEnabled: true,
+			expectUnrestricted:   false,
+		},
+		"authorization set": {
+			authorizationSet:   true,
+			expectUnrestricted: false,
+		},
+		"authorization and explicit perms set": {
+			explicitPermsEnabled: true,
+			authorizationSet:     true,
+			expectUnrestricted:   false,
+		},
+	}
+
+	e := &externalServiceStore{logger: logtest.NoOp(t)}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			pmu := globals.PermissionsUserMapping()
+			t.Cleanup(func() {
+				globals.SetPermissionsUserMapping(pmu)
+			})
+
+			es := &types.ExternalService{}
+
+			if tc.explicitPermsEnabled {
+				globals.SetPermissionsUserMapping(&schema.PermissionsUserMapping{
+					BindID:  "email",
+					Enabled: true,
+				})
+			}
+			rawConfig := "{}"
+			var err error
+			if tc.authorizationSet {
+				rawConfig, err = jsonc.Edit(rawConfig, struct{}{}, "authorization")
+				require.NoError(t, err)
+			}
+
+			require.NoError(t, e.recalculateFields(es, rawConfig))
+
+			require.Equal(t, es.Unrestricted, tc.expectUnrestricted)
+		})
+	}
+}
+
 func TestExternalServiceStore_ListRepos(t *testing.T) {
 	if testing.Short() {
 		t.Skip()

internal/repos/awscodecommit.go

@@ -147,6 +147,7 @@ func (s *AWSCodeCommitSource) makeRepo(r *awscodecommit.Repository) *types.Repo
 			},
 		},
 		Metadata: r,
+		Private:  !s.svc.Unrestricted,
 	}
 }
 

internal/repos/gitolite.go

@@ -123,5 +123,6 @@ func (s GitoliteSource) makeRepo(repo *gitolite.Repo) *types.Repo {
 			},
 		},
 		Metadata: repo,
+		Private:  !s.svc.Unrestricted,
 	}
 }

internal/repos/other.go

@@ -198,6 +198,7 @@ func (s OtherSource) otherRepoFromCloneURL(urn string, u *url.URL) (*types.Repo,
 		Metadata: &extsvc.OtherRepoMetadata{
 			RelativePath: strings.TrimPrefix(repoURL, serviceID),
 		},
+		Private: !s.svc.Unrestricted,
 	}, nil
 }
 

internal/repos/phabricator.go

@@ -168,6 +168,7 @@ func (s *PhabricatorSource) makeRepo(repo *phabricator.Repo) (*types.Repo, error
 			},
 		},
 		Metadata: repo,
+		Private:  !s.svc.Unrestricted,
 	}, nil
 }
 

schema/site.schema.json

@@ -1113,12 +1113,12 @@
       "default": "disabled"
     },
     "permissions.userMapping": {
-      "description": "Settings for Sourcegraph permissions, which allow the site admin to explicitly manage repository permissions via the GraphQL API. This setting cannot be enabled if repository permissions for any specific external service are enabled (i.e., when the external service's `authorization` field is set).",
+      "description": "Settings for Sourcegraph explicit permissions, which allow the site admin to explicitly manage repository permissions via the GraphQL API. This will mark repositories as restricted by default.",
       "type": "object",
       "additionalProperties": false,
       "properties": {
         "enabled": {
-          "description": "Whether permissions user mapping is enabled. There must be no `authorization` field in any external service configuration before enabling this.",
+          "description": "Whether permissions user mapping is enabled.",
           "type": "boolean",
           "default": false
         },