From 04ccf3d401e8b935a1c037ede1d7b9bc708efe46 Mon Sep 17 00:00:00 2001 From: Adam Fraser Date: Wed, 10 Apr 2024 20:18:58 -0700 Subject: [PATCH] [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 | 8 ++-- rest/oidc_api_test.go | 74 ++++++++++++++++++++++++++++++++++ rest/user_api_test.go | 85 ++++++++++++++++++++++++++++++--------- rest/utilities_testing.go | 9 +++++ 4 files changed, 153 insertions(+), 23 deletions(-) diff --git a/db/users.go b/db/users.go index e4371ba009..92afbac80b 100644 --- a/db/users.go +++ b/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 diff --git a/rest/oidc_api_test.go b/rest/oidc_api_test.go index 669bb332d3..47860709da 100644 --- a/rest/oidc_api_test.go +++ b/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{} diff --git a/rest/user_api_test.go b/rest/user_api_test.go index a5cfb5108d..8367e1a904 100644 --- a/rest/user_api_test.go +++ b/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) { diff --git a/rest/utilities_testing.go b/rest/utilities_testing.go index 36f1ef5470..17ac5cfecd 100644 --- a/rest/utilities_testing.go +++ b/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())