[3.1.5] Restore 3.1 handling for admin_channels, admin_roles (#6770)

Omitting default collection admin_channels, admin_roles on a PUT /db/_user should not remove those channels - callers must explicitly specify an empty array.

Handling for named collections is unchanged - if scopes/collection is specified in collection_access, contents are treated as a replace.

Added tests for interaction with JWT channel updates.

db/users.go

@@ -113,7 +113,7 @@ func (dbc *DatabaseContext) UpdatePrincipal(ctx context.Context, updates *auth.P
 		if updatedExplicitChannels == nil {
 			updatedExplicitChannels = ch.TimedSet{}
 		}
-		if !updatedExplicitChannels.Equals(updates.ExplicitChannels) {
+		if updates.ExplicitChannels != nil && !updatedExplicitChannels.Equals(updates.ExplicitChannels) {
 			changed = true
 		}
 		collectionAccessChanged, err := dbc.RequiresCollectionAccessUpdate(ctx, princ, updates.CollectionAccess)
@@ -149,7 +149,7 @@ func (dbc *DatabaseContext) UpdatePrincipal(ctx context.Context, updates *auth.P
 			if updatedExplicitRoles == nil {
 				updatedExplicitRoles = ch.TimedSet{}
 			}
-			if !updatedExplicitRoles.Equals(updates.ExplicitRoleNames) {
+			if updates.ExplicitRoleNames != nil && !updatedExplicitRoles.Equals(updates.ExplicitRoleNames) {
 				changed = true
 			}
 
@@ -190,7 +190,7 @@ func (dbc *DatabaseContext) UpdatePrincipal(ctx context.Context, updates *auth.P
 		princ.SetSequence(nextSeq)
 
 		// Now update the Principal object from the properties in the request, first the channels:
-		if updatedExplicitChannels.UpdateAtSequence(updates.ExplicitChannels, nextSeq) {
+		if updates.ExplicitChannels != nil && updatedExplicitChannels.UpdateAtSequence(updates.ExplicitChannels, nextSeq) {
 			princ.SetExplicitChannels(updatedExplicitChannels, nextSeq)
 		}
 
@@ -199,7 +199,7 @@ func (dbc *DatabaseContext) UpdatePrincipal(ctx context.Context, updates *auth.P
 		}
 
 		if isUser {
-			if updatedExplicitRoles.UpdateAtSequence(updates.ExplicitRoleNames, nextSeq) {
+			if updates.ExplicitRoleNames != nil && updatedExplicitRoles.UpdateAtSequence(updates.ExplicitRoleNames, nextSeq) {
 				user.SetExplicitRoles(updatedExplicitRoles, nextSeq)
 			}
 			var hasJWTUpdates bool

rest/oidc_api_test.go

@@ -1285,6 +1285,80 @@ func TestUserAPIReadOnlyFields(t *testing.T) {
 	assert.Equal(t, userOutput, resp.Body.String())
 }
 
+// TestAdminAndJWTChannels validates interaction between admin channels and JWT channels, to ensure each are preserved when
+// the other is changed
+func TestAdminAndJWTChannels(t *testing.T) {
+	base.SetUpTestLogging(t, base.LevelInfo, base.KeyAll)
+
+	testProviders := auth.OIDCProviderMap{
+		"foo": mockProviderWith("foo", mockProviderUserPrefix{"foo"}, mockProviderChannelsClaim{"channels"}),
+	}
+	defaultProvider := "foo"
+
+	mockAuthServer, err := newMockAuthServer()
+	require.NoError(t, err, "Error creating mock oauth2 server")
+	mockAuthServer.Start()
+	defer mockAuthServer.Shutdown()
+	mockAuthServer.options.issuer = mockAuthServer.URL + "/" + defaultProvider
+	refreshProviderConfig(testProviders, mockAuthServer.URL)
+
+	opts := auth.OIDCOptions{Providers: testProviders, DefaultProvider: &defaultProvider}
+	restTesterConfig := RestTesterConfig{SyncFn: channels.DocChannelsSyncFunction, DatabaseConfig: &DatabaseConfig{DbConfig: DbConfig{OIDCConfig: &opts}}}
+
+	// JWT claim based grants do not support named collections
+	restTester := NewRestTesterDefaultCollection(t, &restTesterConfig)
+	require.NoError(t, restTester.SetAdminParty(false))
+	defer restTester.Close()
+
+	// Create user with admin channels and roles
+	collection := restTester.GetSingleTestDatabaseCollection()
+	payload := GetUserPayload(t, "foo_noah", "pass", "", collection, []string{"ABC"}, []string{"role1"})
+	response := restTester.SendAdminRequest(http.MethodPut, "/db/_user/foo_noah", payload)
+	RequireStatus(t, response, http.StatusCreated)
+
+	token, err := mockAuthServer.makeToken(claimsAuthenticWithExtraClaims(map[string]interface{}{"channels": []string{"foo"}}))
+	require.NoError(t, err, "Error obtaining signed token from OpenID Connect provider")
+	require.NotEmpty(t, token, "Empty token retrieved from OpenID Connect provider")
+
+	// Use bearer token in a keyspace request to update channels
+	resp := restTester.SendRequestWithHeaders(http.MethodGet, "/{{.db}}/", "", map[string]string{"Authorization": BearerToken + " " + token})
+	RequireStatus(t, resp, http.StatusOK)
+
+	userConfig := restTester.GetUserAdminAPI("foo_noah")
+	requireAdminChannels(t, base.SetFromArray([]string{"ABC"}), userConfig, collection)
+	requireJWTChannels(t, base.SetFromArray([]string{"foo"}), userConfig, collection)
+
+	// Update the admin channels to DEF, ensure JWT channels are preserved
+	payload = GetUserPayload(t, "", "", "", collection, []string{"DEF"}, nil)
+	response = restTester.SendAdminRequest(http.MethodPut, "/db/_user/foo_noah", payload)
+	RequireStatus(t, response, http.StatusOK)
+
+	userConfig = restTester.GetUserAdminAPI("foo_noah")
+	requireAdminChannels(t, base.SetFromArray([]string{"DEF"}), userConfig, collection)
+	requireJWTChannels(t, base.SetFromArray([]string{"foo"}), userConfig, collection)
+
+	// Update token to update the channels claim to 'bar'
+	updatedToken, err := mockAuthServer.makeToken(claimsAuthenticWithExtraClaims(map[string]interface{}{"channels": []string{"bar"}}))
+	require.NoError(t, err, "Error obtaining signed token from OpenID Connect provider")
+	require.NotEmpty(t, token, "Empty token retrieved from OpenID Connect provider")
+	resp = restTester.SendRequestWithHeaders(http.MethodGet, "/{{.db}}/", "", map[string]string{"Authorization": BearerToken + " " + updatedToken})
+	RequireStatus(t, resp, http.StatusOK)
+
+	userConfig = restTester.GetUserAdminAPI("foo_noah")
+	requireAdminChannels(t, base.SetFromArray([]string{"DEF"}), userConfig, collection)
+	requireJWTChannels(t, base.SetFromArray([]string{"bar"}), userConfig, collection)
+
+	// Update token with no channel claims, ensure it removes JWT channels and preserves admin channels
+	updatedToken, err = mockAuthServer.makeToken(claimsAuthenticWithExtraClaims(map[string]interface{}{"otherClaim": "not a channel"}))
+	require.NoError(t, err, "Error obtaining signed token from OpenID Connect provider")
+	require.NotEmpty(t, token, "Empty token retrieved from OpenID Connect provider")
+	resp = restTester.SendRequestWithHeaders(http.MethodGet, "/{{.db}}/", "", map[string]string{"Authorization": BearerToken + " " + updatedToken})
+	RequireStatus(t, resp, http.StatusOK)
+	userConfig = restTester.GetUserAdminAPI("foo_noah")
+	requireAdminChannels(t, base.SetFromArray([]string{"DEF"}), userConfig, collection)
+	requireJWTChannels(t, base.Set(nil), userConfig, collection)
+}
+
 // checkGoodAuthResponse asserts expected session response values against the given response.
 func checkGoodAuthResponse(t *testing.T, rt *RestTester, response *http.Response, username string) {
 	var responseBodyExpected map[string]interface{}

rest/user_api_test.go

@@ -1282,40 +1282,87 @@ func TestGetUserCollectionAccess(t *testing.T) {
 	RequireStatus(t, putResponse, 404)
 }
 
-// TestPutUserUnsetAdminChannels ensures that a PUT on the /_user/... endpoint with `null` admin_channels can be used to unset channels.
-// Repros CBG-3610, a regression introduced in 3.1.0
-func TestPutUserUnsetAdminChannels(t *testing.T) {
-	rt := NewRestTester(t, nil)
+// TestPutUserUnsetAdminChannelsDefaultCollection ensures that a PUT on the /_user/... endpoint that omits admin_channels for the default collection will not unset channels, but empty array will.
+// See CBG-3883
+func TestPutUserUnsetAdminChannelsDefaultCollection(t *testing.T) {
+	rt := NewRestTesterDefaultCollection(t, nil)
 	defer rt.Close()
 
-	defaultCollection := rt.GetSingleTestDatabaseCollection().IsDefaultCollection()
+	collection := rt.GetSingleTestDatabaseCollection()
+	userRoles := []string{"role1"}
 
 	// Create a user with some admin channels and a role
-	payload := GetUserPayload(t, "demo", "password1", "", rt.GetSingleTestDatabaseCollection(), []string{"foo", "bar"}, []string{"quux"})
+	payload := GetUserPayload(t, "demo", "password1", "", collection, []string{"foo", "bar"}, userRoles)
 	response := rt.SendAdminRequest(http.MethodPut, "/db/_user/demo", payload)
 	RequireStatus(t, response, http.StatusCreated)
 
 	// Note: Password not a required field for updates (only creations)
-	// Update the user to unset the admin channels
-	payload = GetUserPayload(t, "demo", "", "", rt.GetSingleTestDatabaseCollection(), nil, nil)
+	// Update the user with empty payload, ensure no changes
+	payload = GetUserPayload(t, "", "", "", collection, nil, nil)
 	response = rt.SendAdminRequest(http.MethodPut, "/db/_user/demo", payload)
 	RequireStatus(t, response, http.StatusOK)
 
-	// Check that the user no longer has access to the channels - they should've been removed in the above update
-	response = rt.SendAdminRequest(http.MethodGet, "/db/_user/demo", "")
+	// Check that the user still has access to the channels
+	userConfig := rt.GetUserAdminAPI("demo")
+	requireAdminChannels(t, base.SetFromArray([]string{"foo", "bar"}), userConfig, collection)
+	assert.Equal(t, base.SetFromArray(userRoles), userConfig.ExplicitRoleNames)
+
+	// Update the user with an empty admin channels to remove them
+	payload = `{"admin_channels":[]}`
+	response = rt.SendAdminRequest(http.MethodPut, "/db/_user/demo", payload)
 	RequireStatus(t, response, http.StatusOK)
-	var responseConfig auth.PrincipalConfig
-	err := json.Unmarshal(response.Body.Bytes(), &responseConfig)
-	require.NoError(t, err)
 
-	explicitChannels := responseConfig.ExplicitChannels
-	explicitRoles := responseConfig.ExplicitRoleNames
-	if !defaultCollection {
-		explicitChannels = responseConfig.CollectionAccess[rt.GetDbCollections()[0].ScopeName][rt.GetDbCollections()[0].Name].ExplicitChannels_
+	// Verify that the user still has access to the channels
+	userConfig = rt.GetUserAdminAPI("demo")
+	requireAdminChannels(t, base.Set(nil), userConfig, collection)
+	assert.Equal(t, base.SetFromArray(userRoles), userConfig.ExplicitRoleNames)
+}
+
+// TestPutUserUnsetAdminChannelsNamedCollection ensures that a PUT on the /_user/... endpoint that omits admin_channels for a named collection will unset channels (since it's the
+// only writable property in collection_access via the REST API.
+// See CBG-3883
+func TestPutUserUnsetAdminChannelsNamedCollection(t *testing.T) {
+	rt := NewRestTester(t, nil)
+	defer rt.Close()
+
+	collection := rt.GetSingleTestDatabaseCollection()
+	if collection.IsDefaultCollection() {
+		t.Skip("Named collection test")
 	}
+	userRoles := []string{"role1"}
 
-	assert.Equal(t, base.Set(nil), explicitChannels)
-	assert.Equal(t, base.Set(nil), explicitRoles)
+	// Create a user with some admin channels and a role
+	payload := GetUserPayload(t, "demo", "password1", "", collection, []string{"foo", "bar"}, userRoles)
+	response := rt.SendAdminRequest(http.MethodPut, "/db/_user/demo", payload)
+	RequireStatus(t, response, http.StatusCreated)
+
+	// Note: Password not a required field for updates (only creations)
+	// Update the user with empty admin channels, ensure channels are removed but roles are preserved
+	payload = GetUserPayload(t, "", "", "", collection, nil, nil)
+	response = rt.SendAdminRequest(http.MethodPut, "/db/_user/demo", payload)
+	RequireStatus(t, response, http.StatusOK)
+
+	// Check that channel access has been removed, but role access is unchanged
+	userConfig := rt.GetUserAdminAPI("demo")
+	requireAdminChannels(t, base.Set(nil), userConfig, collection)
+	assert.Equal(t, base.SetFromArray(userRoles), userConfig.ExplicitRoleNames)
+
+}
+
+func requireAdminChannels(t *testing.T, expectedChannels base.Set, principalConfig auth.PrincipalConfig, collection *db.DatabaseCollection) {
+	configChannels := principalConfig.ExplicitChannels
+	if !collection.IsDefaultCollection() {
+		configChannels = principalConfig.CollectionAccess[collection.ScopeName][collection.Name].ExplicitChannels_
+	}
+	require.Equal(t, expectedChannels, configChannels)
+}
+
+func requireJWTChannels(t *testing.T, expectedChannels base.Set, principalConfig auth.PrincipalConfig, collection *db.DatabaseCollection) {
+	configChannels := principalConfig.JWTChannels
+	if !collection.IsDefaultCollection() {
+		configChannels = principalConfig.CollectionAccess[collection.ScopeName][collection.Name].JWTChannels_
+	}
+	require.Equal(t, expectedChannels, configChannels)
 }
 
 func TestPutUserCollectionAccess(t *testing.T) {

rest/utilities_testing.go

@@ -484,6 +484,15 @@ func (rt *RestTester) CreateUser(username string, channels []string) {
 	RequireStatus(rt.TB, response, http.StatusCreated)
 }
 
+func (rt *RestTester) GetUserAdminAPI(username string) auth.PrincipalConfig {
+	response := rt.SendAdminRequest(http.MethodGet, "/{{.db}}/_user/"+username, "")
+	RequireStatus(rt.TB, response, http.StatusOK)
+	var responseConfig auth.PrincipalConfig
+	err := json.Unmarshal(response.Body.Bytes(), &responseConfig)
+	require.NoError(rt.TB, err)
+	return responseConfig
+}
+
 // GetSingleTestDatabaseCollection will return a DatabaseCollection if there is only one. Depending on test environment configuration, it may or may not be the default collection.
 func (rt *RestTester) GetSingleTestDatabaseCollection() *db.DatabaseCollection {
 	return db.GetSingleDatabaseCollection(rt.TB, rt.GetDatabase())