diff --git a/e2e-tests/playwright/lib/src/ui/components/channels/post_create.ts b/e2e-tests/playwright/lib/src/ui/components/channels/post_create.ts index 0ea6c718219d..ec8a4750e822 100644 --- a/e2e-tests/playwright/lib/src/ui/components/channels/post_create.ts +++ b/e2e-tests/playwright/lib/src/ui/components/channels/post_create.ts @@ -75,6 +75,21 @@ export default class ChannelsPostCreate { await this.input.fill(message); } + /** + * Types the message into the input one keystroke at a time, the way a user does, without sending it. + * Prefer this over writeMessage when the behaviour under test depends on the input changing more than + * once, such as the autocomplete, which debounces each change before searching the server. + * @param message : Message to be typed into the input + * @param options.delay : Milliseconds to wait between keystrokes. Use a delay longer than the + * autocomplete's debounce when a search per keystroke is wanted. + */ + async typeMessage(message: string, options?: {delay?: number}) { + await this.input.waitFor(); + await expect(this.input).toBeVisible(); + + await this.input.pressSequentially(message, options); + } + /** * Returns the value of the message input */ diff --git a/e2e-tests/playwright/specs/functional/channels/forward_post_modal/forward_post_channel_select.spec.ts b/e2e-tests/playwright/specs/functional/channels/forward_post_modal/forward_post_channel_select.spec.ts new file mode 100644 index 000000000000..eb671115a0ee --- /dev/null +++ b/e2e-tests/playwright/specs/functional/channels/forward_post_modal/forward_post_channel_select.spec.ts @@ -0,0 +1,57 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {expect, test} from '@mattermost/playwright-lib'; + +/** + * @objective Verify that the forward-post channel selector normalizes search results, selects a destination, and + * forwards the post there. + */ +test('forward post channel selector searches and selects a destination channel', {tag: '@channels'}, async ({pw}) => { + const {team, user, adminClient} = await pw.initSetup(); + const target = await adminClient.createPublicChannel(team.id, `Forward Target ${pw.random.id()}`); + await adminClient.addToChannel(user.id, target.id); + + const message = `forward-selector-${pw.random.id()}`; + const {channelsPage, page} = await pw.testBrowser.login(user); + await channelsPage.goto(team.name, 'town-square'); + await channelsPage.toBeVisible(); + await channelsPage.postMessage(message); + + // # Open the Forward message modal from the posted message + const post = await channelsPage.getLastPost(); + const postId = await post.getId(); + await post.hover(); + await post.postMenu.toBeVisible(); + await post.postMenu.openDotMenu(); + await channelsPage.postDotMenu.toBeVisible(); + await channelsPage.postDotMenu.forwardMenuItem.click(); + + const modal = page.getByRole('dialog', {name: 'Forward message'}); + await expect(modal).toBeVisible(); + + // # Search for a destination that was not recently viewed, exercising the provider normalization path. + // # react-select renders the combobox itself and drops the data-testid given to it, so find it by role. + const input = modal.getByRole('combobox'); + await input.fill(target.display_name); + + const option = page.getByRole('option').filter({hasText: target.display_name}).first(); + await expect(option).toBeVisible(); + await option.click(); + + // * Verify selecting the normalized channel result enables forwarding + const forwardButton = modal.getByRole('button', {name: 'Forward', exact: true}); + await expect(forwardButton).toBeEnabled(); + + // # Forward the post and verify it arrives in the selected channel + await forwardButton.click(); + await expect(modal).not.toBeVisible(); + + await channelsPage.goto(team.name, target.name); + await channelsPage.toBeVisible(); + + // * Verify the forwarded post links back to the original. Assert on the permalink rather than the + // * original message because the permalink preview that renders the message is only generated when the + // * link matches the server's SiteURL, which isn't the origin the browser uses in every environment. + await channelsPage.centerView.waitUntilLastPostContains(`/pl/${postId}`); +}); diff --git a/e2e-tests/playwright/specs/functional/channels/post_textbox/channel_autocomplete_slow_search.spec.ts b/e2e-tests/playwright/specs/functional/channels/post_textbox/channel_autocomplete_slow_search.spec.ts new file mode 100644 index 000000000000..e441a0959a93 --- /dev/null +++ b/e2e-tests/playwright/specs/functional/channels/post_textbox/channel_autocomplete_slow_search.spec.ts @@ -0,0 +1,203 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import type {Locator, Page} from '@playwright/test'; + +import {expect, test} from '@mattermost/playwright-lib'; + +const AUTOCOMPLETE_ROUTE = /\/api\/v4\/teams\/[^/]+\/channels\/autocomplete/; + +function searchedName(url: string) { + return new URL(url).searchParams.get('name') ?? ''; +} + +async function typeMentionAndWaitForRequests(input: Locator, page: Page, message: string) { + let name = ''; + + for (const [index, character] of [...message].entries()) { + let request: Promise | undefined; + if (index > 0) { + name += character; + request = page.waitForRequest( + (request) => AUTOCOMPLETE_ROUTE.test(request.url()) && searchedName(request.url()) === name, + ); + } + + await input.pressSequentially(character); + await request; + } +} + +/** + * Holds every channel search until the test releases it, so that the autocomplete can be observed while a + * search is still in flight. + */ +async function holdChannelSearch(page: Page) { + let release!: () => void; + const released = new Promise((resolve) => { + release = resolve; + }); + + await page.route(AUTOCOMPLETE_ROUTE, async (route) => { + await released; + await route.continue(); + }); + + return release; +} + +/** + * @objective Verify that the ~channel autocomplete renders the channels it already knows about while a + * search for more channels is still in flight, and preserves those results when the response adds a member. + */ +test( + 'channel mention autocomplete shows local channels while searching for more', + {tag: ['@mentions']}, + async ({pw}) => { + // # Initialize setup + const {team, user, adminClient} = await pw.initSetup(); + + // # Create a public channel that the user is a member of, so it comes from the local store + const localChannel = await adminClient.createChannel({ + team_id: team.id, + name: 'ac-local-' + Date.now(), + display_name: 'AC Z Local', + type: 'O', + }); + await adminClient.addToChannel(user.id, localChannel.id); + + // # Create a private channel that the user is a member of. Private channels are not included in the + // # initial local public-channel results, but the server response adds them to the same channel list. + const privateChannel = await adminClient.createPrivateChannel( + team.id, + 'AC A Private', + 'ac-private-' + Date.now(), + ); + await adminClient.addToChannel(user.id, privateChannel.id); + + // # Create a channel that the user is not a member of, so it can only come from the search + await adminClient.createChannel({ + team_id: team.id, + name: 'ac-remote-' + Date.now(), + display_name: 'AC Remote', + type: 'O', + }); + + // # Log in as regular user + const {channelsPage, page} = await pw.testBrowser.login(user); + + // # Hold the channel search open so the initial local results can be observed independently of the response + const releaseSearch = await holdChannelSearch(page); + + // # Visit town-square channel + await channelsPage.goto(team.name, 'town-square'); + await channelsPage.toBeVisible(); + + // # Type a channel mention matching both channels + const postCreate = channelsPage.centerView.postCreate; + await postCreate.typeMessage('~ac-'); + + // * Verify the channels the user is a member of are shown without waiting for the search + const suggestionList = postCreate.suggestionList; + const myChannels = suggestionList.getByRole('group', {name: 'My Channels'}); + await expect(myChannels.getByRole('option')).toContainText(['AC Z Local']); + + // * Verify the group of channels being searched for shows that it is still loading + const otherChannels = suggestionList.getByRole('group', {name: 'Other Channels'}); + await expect(otherChannels.getByTestId('loadingSpinner')).toBeVisible(); + + // # Release the response and wait for it to be delivered + const searchResponse = page.waitForResponse( + (response) => AUTOCOMPLETE_ROUTE.test(response.url()) && searchedName(response.url()) === 'ac-', + ); + releaseSearch(); + await searchResponse; + + // * Verify the searched channel is shown once the search finishes + await expect(otherChannels.getByRole('option')).toContainText(['AC Remote']); + + // * Verify the member channel added by the response is paired with its own term, rather than mutating the + // * already-rendered local result. The order the two are rendered in is not part of what's under test. + const myChannelOptions = myChannels.getByRole('option'); + await expect(myChannelOptions).toHaveCount(2); + await expect(myChannelOptions.filter({hasText: 'AC A Private'})).toHaveCount(1); + await expect(myChannelOptions.filter({hasText: 'AC Z Local'})).toHaveCount(1); + await expect(otherChannels.getByTestId('loadingSpinner')).not.toBeVisible(); + }, +); + +/** + * @objective Verify that the ~channel autocomplete keeps showing results for what has been typed when an + * earlier, slower search finishes after a later one. + */ +test('channel mention autocomplete ignores a search that finishes out of order', {tag: ['@mentions']}, async ({pw}) => { + // # Initialize setup + const {team, user, adminClient} = await pw.initSetup(); + + // # Create channels that the user is not a member of, so they can only come from the search + await adminClient.createChannel({ + team_id: team.id, + name: 'ac-alpha-' + Date.now(), + display_name: 'AC Alpha', + type: 'O', + }); + await adminClient.createChannel({ + team_id: team.id, + name: 'ac-beta-' + Date.now(), + display_name: 'AC Beta', + type: 'O', + }); + + // # Log in as regular user + const {channelsPage, page} = await pw.testBrowser.login(user); + + // # Hold the shortest search while allowing the fully typed search to finish first + const staleSearchName = 'ac-'; + let releaseStaleSearch!: () => void; + const staleSearchReleased = new Promise((resolve) => { + releaseStaleSearch = resolve; + }); + + await page.route(AUTOCOMPLETE_ROUTE, async (route) => { + if (searchedName(route.request().url()) === staleSearchName) { + await staleSearchReleased; + } + await route.continue(); + }); + + // # Visit town-square channel + await channelsPage.goto(team.name, 'town-square'); + await channelsPage.toBeVisible(); + + // # Note when the delayed search for the shortest mention finishes + const staleSearch = page.waitForResponse( + (response) => AUTOCOMPLETE_ROUTE.test(response.url()) && searchedName(response.url()) === staleSearchName, + ); + const currentSearch = page.waitForResponse( + (response) => AUTOCOMPLETE_ROUTE.test(response.url()) && searchedName(response.url()) === 'ac-alpha', + ); + + // # Type a channel mention, waiting for each request so the stale response can be held deterministically + const postCreate = channelsPage.centerView.postCreate; + await typeMentionAndWaitForRequests(postCreate.input, page, '~ac-alpha'); + + // # Ensure the current search response is the one that populated the list before releasing the stale response + await currentSearch; + + // * Verify only the channel matching what was typed is shown + const otherChannels = postCreate.suggestionList.getByRole('group', {name: 'Other Channels'}); + await expect(otherChannels.getByRole('option')).toContainText(['AC Alpha']); + await expect(otherChannels.getByRole('option')).toHaveCount(1); + + // # Release the stale response and wait for it to finish last + releaseStaleSearch(); + await staleSearch; + + // * Verify its results are ignored, since they no longer match what was typed + await expect(otherChannels.getByRole('option')).toHaveCount(1); + await expect(otherChannels.getByRole('option')).toContainText(['AC Alpha']); + + // * Verify the mention can still be completed, so the suggestion list is still interactive + await otherChannels.getByRole('option').first().click(); + await expect(postCreate.input).toHaveValue(/~ac-alpha/); +}); diff --git a/e2e-tests/playwright/specs/functional/channels/post_textbox/channel_autocomplete_sorting.spec.ts b/e2e-tests/playwright/specs/functional/channels/post_textbox/channel_autocomplete_sorting.spec.ts index 4dd6d7ed7f5c..08cab961f094 100644 --- a/e2e-tests/playwright/specs/functional/channels/post_textbox/channel_autocomplete_sorting.spec.ts +++ b/e2e-tests/playwright/specs/functional/channels/post_textbox/channel_autocomplete_sorting.spec.ts @@ -52,7 +52,7 @@ test( await channelsPage.toBeVisible(); // # Type a channel mention in the message input to trigger autocomplete - await channelsPage.centerView.postCreate.writeMessage('~gamma'); + await channelsPage.centerView.postCreate.typeMessage('~gamma'); // # Wait for the suggestion list to appear const suggestionList = channelsPage.centerView.postCreate.suggestionList; diff --git a/e2e-tests/playwright/specs/functional/channels/search/search_box_suggestions.spec.ts b/e2e-tests/playwright/specs/functional/channels/search/search_box_suggestions.spec.ts index 659ebfca5a93..008469c11fc1 100644 --- a/e2e-tests/playwright/specs/functional/channels/search/search_box_suggestions.spec.ts +++ b/e2e-tests/playwright/specs/functional/channels/search/search_box_suggestions.spec.ts @@ -87,3 +87,42 @@ test('remove extra whitespace when selecting a user', async ({pw}) => { const expectedText = `from:${admin.username} `; await expect(searchInput).toHaveValue(expectedText); }); + +/** + * @objective Verify that grouped search suggestions are trimmed to ten results and retain the selected channel's + * term/item pairing. + */ +test('limits grouped channel suggestions and preserves the selected channel', {tag: '@search'}, async ({pw}) => { + const {team, user, adminClient} = await pw.initSetup(); + const prefix = `search-trim-${pw.random.id()}`; + const channels = []; + + // The search endpoint returns up to 50 channels; the search UI trims the grouped results to 10. + for (let i = 0; i < 12; i++) { + const channel = await adminClient.createPublicChannel(team.id, `Search Trim ${i} ${prefix}`, `${prefix}-${i}`); + await adminClient.addToChannel(user.id, channel.id); + channels.push(channel); + } + + const {channelsPage} = await pw.testBrowser.login(user); + await channelsPage.goto(team.name, 'town-square'); + await channelsPage.toBeVisible(); + + await channelsPage.globalHeader.openSearch(); + await channelsPage.searchBox.toBeVisible(); + const {searchInput, container} = channelsPage.searchBox; + await searchInput.fill(`In:${prefix}`); + + // * Verify trimResults applies across the grouped channel results + const suggestions = container.getByRole('option'); + await expect(suggestions).toHaveCount(10); + + // # Select a result that survived trimming and verify the term/item pairing updates the query correctly + const renderedText = (await suggestions.allInnerTexts()).join('\n'); + const selectedChannel = channels.find((channel) => renderedText.includes(channel.display_name)); + if (!selectedChannel) { + throw new Error('Expected at least one created channel to remain in the trimmed suggestions'); + } + await suggestions.filter({hasText: selectedChannel.display_name}).first().click(); + await expect(searchInput).toHaveValue(new RegExp(`In:${selectedChannel.name}\\s`)); +}); diff --git a/e2e-tests/playwright/specs/functional/channels/wysiwyg_editor/autocomplete.spec.ts b/e2e-tests/playwright/specs/functional/channels/wysiwyg_editor/autocomplete.spec.ts index 4768386b944b..464b5fc0ae6a 100644 --- a/e2e-tests/playwright/specs/functional/channels/wysiwyg_editor/autocomplete.spec.ts +++ b/e2e-tests/playwright/specs/functional/channels/wysiwyg_editor/autocomplete.spec.ts @@ -4,6 +4,7 @@ import {expect, setWysiwygUserPreference, test, WysiwygEditor} from '@mattermost/playwright-lib'; const TAGS = {tag: ['@channels', '@wysiwyg_editor']}; +const AUTOCOMPLETE_ROUTE = /\/api\/v4\/teams\/[^/]+\/channels\/autocomplete/; test.describe('WYSIWYG editor - autocomplete suggestions', TAGS, () => { test('slash command autocomplete opens and completes on Enter', async ({pw}) => { @@ -64,6 +65,54 @@ test.describe('WYSIWYG editor - autocomplete suggestions', TAGS, () => { await expect(editor.input).toContainText(`~${linked.name}`); }); + /** + * @objective Verify that WYSIWYG channel autocomplete renders the channels already known locally while a search + * for more channels is still in flight. + * + * The post_textbox equivalent goes on to verify that the search response is merged into what is already + * rendered. The WYSIWYG editor never applies that response — the searched group keeps its loading indicator + * indefinitely — so this test asserts only what the editor does today. + */ + test('~channel autocomplete shows local results while a search is in flight', async ({pw}) => { + const {adminClient, user, userClient, team} = await pw.initSetup(); + await setWysiwygUserPreference(userClient, user.id, true); + + const localChannel = await adminClient.createPublicChannel(team.id, 'AC Z WYSIWYG Local', 'ac-wysiwyg-local'); + await adminClient.addToChannel(user.id, localChannel.id); + + const {channelsPage, page} = await pw.testBrowser.login(user); + await channelsPage.goto(team.name, 'off-topic'); + + const editor = new WysiwygEditor(page.getByTestId('post-create')); + await editor.toBeVisible(); + + let releaseSearch!: () => void; + const searchReleased = new Promise((resolve) => { + releaseSearch = resolve; + }); + await page.route(AUTOCOMPLETE_ROUTE, async (route) => { + await searchReleased; + await route.continue(); + }); + + await editor.type('~ac-'); + + const list = editor.suggestionList(); + const myChannels = list.getByRole('group', {name: 'My Channels'}); + const otherChannels = list.getByRole('group', {name: 'Other Channels'}); + + // * Verify the local public result is visible while the server response is still pending + await expect(myChannels.getByRole('option')).toContainText(['AC Z WYSIWYG Local']); + + // * Verify the group of channels being searched for shows that it is still loading + await expect(otherChannels.getByTestId('loadingSpinner')).toBeVisible(); + + // # Let the held search finish so it isn't left blocked when the test ends + const searchResponse = page.waitForResponse((response) => AUTOCOMPLETE_ROUTE.test(response.url())); + releaseSearch(); + await searchResponse; + }); + test('emoji shortcode autocomplete opens and closes on Escape', async ({pw}) => { const {user, userClient, team} = await pw.initSetup(); await setWysiwygUserPreference(userClient, user.id, true); diff --git a/server/channels/api4/outgoing_oauth_connection.go b/server/channels/api4/outgoing_oauth_connection.go index 67bfffe315da..0eaed4be6969 100644 --- a/server/channels/api4/outgoing_oauth_connection.go +++ b/server/channels/api4/outgoing_oauth_connection.go @@ -378,19 +378,6 @@ func validateOutgoingOAuthConnectionCredentials(c *Context, w http.ResponseWrite return } - if inputConnection.Id != "" && inputConnection.ClientSecret == "" { - var err *model.AppError - var storedConnection *model.OutgoingOAuthConnection - storedConnection, err = service.GetConnection(c.AppContext, inputConnection.Id) - if err != nil { - c.Err = model.NewAppError(whereOutgoingOAuthConnection, "api.context.outgoing_oauth_connection.validate_connection_credentials.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - w.WriteHeader(c.Err.StatusCode) - return - } - - inputConnection.ClientSecret = storedConnection.ClientSecret - } - model.AddEventParameterAuditableToAuditRec(auditRec, "outgoing_oauth_connection", inputConnection) // Try to retrieve a token with the provided credentials diff --git a/server/channels/api4/outgoing_oauth_connection_test.go b/server/channels/api4/outgoing_oauth_connection_test.go index 6eb1b253c98d..febb55601112 100644 --- a/server/channels/api4/outgoing_oauth_connection_test.go +++ b/server/channels/api4/outgoing_oauth_connection_test.go @@ -1408,7 +1408,7 @@ func TestHandlerOutgoingOAuthConnectionHandlerValidate(t *testing.T) { require.Equal(t, http.StatusOK, httpRecorder.Code) }) - t.Run("success (stored connection)", func(t *testing.T) { + t.Run("does not backfill stored secret", func(t *testing.T) { th := Setup(t).InitBasic(t) defer outgoingOauthConnectionsCleanup(t, th) @@ -1419,8 +1419,10 @@ func TestHandlerOutgoingOAuthConnectionHandlerValidate(t *testing.T) { server := newFakeOAuthServer(t) conn := newOutgoingOAuthConnection() + conn.Id = model.NewId() conn.CreatorId = model.NewId() conn.OAuthTokenURL = server.URL + "/valid" + conn.ClientSecret = "" c := &Context{} c.AppContext = th.Context @@ -1433,9 +1435,6 @@ func TestHandlerOutgoingOAuthConnectionHandlerValidate(t *testing.T) { Roles: model.SystemUserRoleId, } c.AppContext = th.Context.WithSession(&session) - c.Params = &web.Params{ - OutgoingOAuthConnectionID: conn.Id, - } th.AddPermissionToRole(t, model.PermissionManageOutgoingOAuthConnections.Id, model.SystemUserRoleId) @@ -1450,7 +1449,6 @@ func TestHandlerOutgoingOAuthConnectionHandlerValidate(t *testing.T) { outgoingOauthIface := &mocks.OutgoingOAuthConnectionInterface{} th.App.Config().ServiceSettings.EnableOutgoingOAuthConnections = new(true) th.App.Srv().OutgoingOAuthConnection = outgoingOauthIface - outgoingOauthIface.Mock.On("GetConnection", c.AppContext, conn.Id).Return(conn, nil) outgoingOauthIface.Mock.On("RetrieveTokenForConnection", c.AppContext, conn).Return(&model.OutgoingOAuthConnectionToken{}, nil) httpRecorder := httptest.NewRecorder() @@ -1461,5 +1459,6 @@ func TestHandlerOutgoingOAuthConnectionHandlerValidate(t *testing.T) { handler.ServeHTTP(httpRecorder, req) require.Equal(t, http.StatusOK, httpRecorder.Code) + outgoingOauthIface.AssertNotCalled(t, "GetConnection", mock.Anything, mock.Anything) }) } diff --git a/server/channels/app/access_control.go b/server/channels/app/access_control.go index f95f2bece958..2c5795ed934c 100644 --- a/server/channels/app/access_control.go +++ b/server/channels/app/access_control.go @@ -1784,13 +1784,13 @@ func (a *App) SearchAccessControlPolicies(rctx request.CTX, opts model.AccessCon return policies, total, nil } -func (a *App) GetAccessControlPolicyAttributes(rctx request.CTX, channelID string, action string) (map[string][]string, *model.AppError) { +func (a *App) GetAccessControlPolicyAttributes(rctx request.CTX, resourceID string, action string) (map[string][]string, *model.AppError) { acs := a.Srv().ch.AccessControl if acs == nil { return nil, model.NewAppError("GetChannelAccessControlAttributes", "app.pap.get_channel_access_control_attributes.app_error", nil, "Policy Administration Point is not initialized", http.StatusNotImplemented) } - attributes, appErr := acs.GetPolicyRuleAttributes(rctx, channelID, action) + attributes, appErr := acs.GetPolicyRuleAttributes(rctx, resourceID, action) if appErr != nil { return nil, appErr } @@ -1808,14 +1808,34 @@ func (a *App) GetAccessControlPolicyAttributes(rctx request.CTX, channelID strin return map[string][]string{}, nil } + // Generate a map of native fields, since they do not reside in the Property Store. + nativeFieldsByName := make(map[string]*model.PropertyField) + for _, f := range model.NativeUserAttributeFields(cpaGroup.ID) { + nativeFieldsByName[f.Name] = f + } + for fieldName := range attributes { // Read directly from the store so this security filter sees the raw // access_mode, unaffected by property read hooks for the request caller. field, fieldErr := a.Srv().Store().PropertyField().GetFieldByNameForObjectType(rctx, cpaGroup.ID, "", model.PropertyFieldObjectTypeUser, fieldName) if fieldErr != nil { - delete(attributes, fieldName) - continue + // If the error is due to not being found, we won't skip to the next field just yet + // in case it is a native field + var nfErr *store.ErrNotFound + notFound := errors.As(fieldErr, &nfErr) + if !notFound { + delete(attributes, fieldName) + continue + } + + //If property wasn't found, check if this is a Native Field. + field = nativeFieldsByName[fieldName] + if field == nil { + delete(attributes, fieldName) + continue + } } + switch field.GetAccessMode() { case model.PropertyAccessModeSourceOnly, model.PropertyAccessModeSharedOnly: delete(attributes, fieldName) diff --git a/server/channels/app/access_control_test.go b/server/channels/app/access_control_test.go index 0606bca8da6c..96abcaf234d6 100644 --- a/server/channels/app/access_control_test.go +++ b/server/channels/app/access_control_test.go @@ -5,17 +5,19 @@ package app import ( "errors" + "maps" "net/http" "strings" "testing" "time" - "github.com/mattermost/mattermost/server/public/model" - "github.com/mattermost/mattermost/server/public/shared/request" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/request" + "github.com/mattermost/mattermost/server/v8/channels/app/properties" "github.com/mattermost/mattermost/server/v8/channels/store" storemocks "github.com/mattermost/mattermost/server/v8/channels/store/storetest/mocks" @@ -4862,7 +4864,7 @@ func TestGetAccessControlPolicyAttributes_MaskedFieldsFiltered(t *testing.T) { mockACS := &mocks.AccessControlServiceInterface{} th.App.Srv().ch.AccessControl = mockACS mockACS.On("GetPolicyRuleAttributes", mock.Anything, channelID, model.AccessControlPolicyActionMembership). - Return(rawAttributes, nil).Once() + Return(maps.Clone(rawAttributes), nil).Once() result, appErr := th.App.GetAccessControlPolicyAttributes(th.Context, channelID, model.AccessControlPolicyActionMembership) require.Nil(t, appErr) @@ -4902,8 +4904,9 @@ func TestGetAccessControlPolicyAttributes_PublicFieldsPassThrough(t *testing.T) mockACS := &mocks.AccessControlServiceInterface{} th.App.Srv().ch.AccessControl = mockACS + // Be sure to clone the rawAttributes map so it isn't modified during the test - we are comparing it later mockACS.On("GetPolicyRuleAttributes", mock.Anything, channelID, model.AccessControlPolicyActionMembership). - Return(rawAttributes, nil).Once() + Return(maps.Clone(rawAttributes), nil).Once() result, appErr := th.App.GetAccessControlPolicyAttributes(th.Context, channelID, model.AccessControlPolicyActionMembership) require.Nil(t, appErr) @@ -4911,6 +4914,99 @@ func TestGetAccessControlPolicyAttributes_PublicFieldsPassThrough(t *testing.T) mockACS.AssertExpectations(t) } +// TestGetAccessControlPolicyAttributes_NativeFieldsPassThrough verifies that +// native attribute fields are returned unchanged. +func TestGetAccessControlPolicyAttributes_NativeFieldsPassThrough(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + + // No need to create a native property for this test; native properties are generated in the codebase. + // The requirement is the field name must be one of the defined native attribute properties that represent + // columns in the user database (they live outside the property system). + + channelID := model.NewId() + rawAttributes := map[string][]string{model.NativeAttributePropertyFieldEmail: {"@sample.mattermost.com"}} + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + // Be sure to clone the rawAttributes map so it isn't modified during the test - we are comparing it later + mockACS.On("GetPolicyRuleAttributes", mock.Anything, channelID, model.AccessControlPolicyActionMembership). + Return(maps.Clone(rawAttributes), nil).Once() + + result, appErr := th.App.GetAccessControlPolicyAttributes(th.Context, channelID, model.AccessControlPolicyActionMembership) + require.Nil(t, appErr) + assert.Equal(t, rawAttributes, result) + mockACS.AssertExpectations(t) +} + +// TestGetAccessControlPolicyAttributes_MaskedFieldsWithNameCollisionAreFiltered checks that +// a masked field that happens to share the same name as a native filed stil gets filtered. +func TestGetAccessControlPolicyAttributes_MaskedFieldsWithNameCollisionAreFiltered(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + + rctx := request.TestContext(t) + + cpaGroup, cErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, cErr) + + permNone := model.PermissionLevelNone + + makeField := func(name, accessMode string) { + protected := accessMode == model.PropertyAccessModeSourceOnly || accessMode == model.PropertyAccessModeSharedOnly + f := &model.PropertyField{ + GroupID: cpaGroup.ID, + Name: name, + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + Protected: protected, + Attrs: model.StringInterface{model.PropertyAttrsAccessMode: accessMode}, + } + if protected { + f.PermissionField = &permNone + f.Attrs[model.PropertyAttrsProtected] = true + _, err := th.App.Srv().Store().PropertyField().Create(f) + require.NoError(t, err) + } else { + _, appErr := th.App.CreatePropertyField(rctx, f, false, "") + require.Nil(t, appErr) + } + } + + makeField("PublicField", model.PropertyAccessModePublic) + makeField("SourceField", model.PropertyAccessModeSourceOnly) + // This is the field with a native name that we are testing to make sure it is still masked. + makeField(model.NativeAttributePropertyFieldIsBot, model.PropertyAccessModeSharedOnly) + + channelID := model.NewId() + rawAttributes := map[string][]string{ + "PublicField": {"Engineering"}, + "SourceField": {"TopSecret"}, + model.NativeAttributePropertyFieldIsBot: {"false"}, + // This is a real native attribute that is not in the property store -it should pass through. + model.NativeAttributePropertyFieldVerified: {"true"}, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + mockACS.On("GetPolicyRuleAttributes", mock.Anything, channelID, model.AccessControlPolicyActionMembership). + Return(rawAttributes, nil).Once() + + result, appErr := th.App.GetAccessControlPolicyAttributes(th.Context, channelID, model.AccessControlPolicyActionMembership) + require.Nil(t, appErr) + + // Only the public field should survive. + expectedAttributes := map[string][]string{ + "PublicField": {"Engineering"}, + model.NativeAttributePropertyFieldVerified: {"true"}, + } + assert.Equal(t, expectedAttributes, result) + assert.NotContains(t, result, "SourceField") + assert.NotContains(t, result, model.NativeAttributePropertyFieldIsBot) + mockACS.AssertExpectations(t) +} + // TestMergeStoredPolicyExpressions_ActionsLocked verifies that a caller who // cannot see all values in a stored rule cannot change that rule's Actions. // The attack: submit a PUT with the same masked expression but a different diff --git a/webapp/channels/src/components/advanced_text_editor/wysiwyg_editor/wysiwyg_suggestion_list.test.tsx b/webapp/channels/src/components/advanced_text_editor/wysiwyg_editor/wysiwyg_suggestion_list.test.tsx index 14aa494fc7dc..2431d8afcf98 100644 --- a/webapp/channels/src/components/advanced_text_editor/wysiwyg_editor/wysiwyg_suggestion_list.test.tsx +++ b/webapp/channels/src/components/advanced_text_editor/wysiwyg_editor/wysiwyg_suggestion_list.test.tsx @@ -25,7 +25,7 @@ jest.mock('components/suggestion/command_provider/command_provider', () => ({ resultCallback({ matchedPretext: pretext, - terms: mockTerms, + terms: [...mockTerms], items: mockTerms.map((term) => ({suggestion: term})), component: () => null, }); diff --git a/webapp/channels/src/components/integrations/outgoing_oauth_connections/__snapshots__/abstract_outgoing_oauth_connection.test.tsx.snap b/webapp/channels/src/components/integrations/outgoing_oauth_connections/__snapshots__/abstract_outgoing_oauth_connection.test.tsx.snap index 4cef641d780d..4a58b549c3a1 100644 --- a/webapp/channels/src/components/integrations/outgoing_oauth_connections/__snapshots__/abstract_outgoing_oauth_connection.test.tsx.snap +++ b/webapp/channels/src/components/integrations/outgoing_oauth_connections/__snapshots__/abstract_outgoing_oauth_connection.test.tsx.snap @@ -96,19 +96,11 @@ exports[`components/integrations/AbstractOutgoingOAuthConnection should match sn > - - -
@@ -311,19 +303,11 @@ exports[`components/integrations/AbstractOutgoingOAuthConnection should match sn > - - -
@@ -400,7 +384,7 @@ exports[`components/integrations/AbstractOutgoingOAuthConnection should match sn
- - -
@@ -309,19 +301,11 @@ exports[`components/integrations/EditOutgoingOAuthConnection should match snapsh > - - -
@@ -522,19 +506,11 @@ exports[`components/integrations/EditOutgoingOAuthConnection should match snapsh > - - -
diff --git a/webapp/channels/src/components/integrations/outgoing_oauth_connections/abstract_outgoing_oauth_connection.test.tsx b/webapp/channels/src/components/integrations/outgoing_oauth_connections/abstract_outgoing_oauth_connection.test.tsx index bfc75a7c89e8..6b3ebf7cfd76 100644 --- a/webapp/channels/src/components/integrations/outgoing_oauth_connections/abstract_outgoing_oauth_connection.test.tsx +++ b/webapp/channels/src/components/integrations/outgoing_oauth_connections/abstract_outgoing_oauth_connection.test.tsx @@ -121,17 +121,18 @@ describe('components/integrations/AbstractOutgoingOAuthConnection', () => { state, ); - await act(async () => { - const nameInput = container.querySelector('#name') as HTMLInputElement; - if (nameInput) { - nameInput.value = 'name'; - nameInput.dispatchEvent(new Event('change', {bubbles: true})); - } + await userEvent.type(container.querySelector('#client_secret') as HTMLInputElement, 'secret'); + await act(async () => { const submitButton = container.querySelector('button.btn-primary') as HTMLButtonElement; submitButton?.click(); + }); - expect(submitAction).toHaveBeenCalled(); + // Changing the secret marks the form unvalidated, so submitting prompts to save anyway. + await act(async () => { + (document.querySelector('#confirmModalButton') as HTMLButtonElement)?.click(); }); + + expect(submitAction).toHaveBeenCalledWith(expect.objectContaining({client_secret: 'secret'})); }); }); diff --git a/webapp/channels/src/components/integrations/outgoing_oauth_connections/abstract_outgoing_oauth_connection.tsx b/webapp/channels/src/components/integrations/outgoing_oauth_connections/abstract_outgoing_oauth_connection.tsx index 77122b98a274..1b97eb7a238c 100644 --- a/webapp/channels/src/components/integrations/outgoing_oauth_connections/abstract_outgoing_oauth_connection.tsx +++ b/webapp/channels/src/components/integrations/outgoing_oauth_connections/abstract_outgoing_oauth_connection.tsx @@ -89,7 +89,6 @@ export default function AbstractOutgoingOAuthConnection(props: Props) { const [isSubmitting, setIsSubmitting] = useState(false); const [validationStatus, setValidationStatus] = useState(ValidationStatus.INITIAL); - const [isEditingSecret, setIsEditingSecret] = useState(false); const [isValidationModalOpen, setIsValidationModalOpen] = useState(false); @@ -123,7 +122,7 @@ export default function AbstractOutgoingOAuthConnection(props: Props) { return undefined; } - if ((isNewConnection || isEditingSecret) && !formState.clientSecret) { + if (!formState.clientSecret) { setIsSubmitting(false); setError( { - setIsEditingSecret(true); - }; - const headerToRender = props.header; const footerToRender = props.footer; - let clientSecretSection = ( - - ); - - if (!isNewConnection && !isEditingSecret) { - clientSecretSection = ( - <> - - - - - - ); - } - return (
@@ -428,7 +391,14 @@ export default function AbstractOutgoingOAuthConnection(props: Props) { />
- {clientSecretSection} +
{ if (prefix !== this.latestPrefix || this.shouldCancelDispatch(prefix)) { return; diff --git a/webapp/channels/src/components/suggestion/suggestion_results.ts b/webapp/channels/src/components/suggestion/suggestion_results.ts index 8b14c6903528..36c61d2b7571 100644 --- a/webapp/channels/src/components/suggestion/suggestion_results.ts +++ b/webapp/channels/src/components/suggestion/suggestion_results.ts @@ -7,6 +7,8 @@ import type { SuggestionResults, } from '@mattermost/shared/types/global'; +import deepFreezeAndThrowOnMutation from 'mattermost-redux/utils/deep_freeze'; + export type { Loading, ProviderResults, @@ -20,12 +22,12 @@ export function isItemLoaded(item: Item | Loading): item is Item { } export function emptyResults(): SuggestionResults { - return { + return deepFreezeAndThrowOnMutation({ matchedPretext: '', terms: [], items: [], components: [], - }; + }); } export function hasResults(results: SuggestionResults): boolean { @@ -92,11 +94,11 @@ export function hasSuggestionWithComponent(results: SuggestionResults, component export function normalizeResultsFromProvider(providerResults: ProviderResults): SuggestionResults { if ('components' in providerResults) { - return providerResults; + return deepFreezeAndThrowOnMutation(providerResults); } if ('groups' in providerResults) { - return { + return deepFreezeAndThrowOnMutation({ matchedPretext: providerResults.matchedPretext, groups: providerResults.groups.map((group) => { if ('components' in group) { @@ -110,48 +112,58 @@ export function normalizeResultsFromProvider(providerResults: ProviderResu components: new Array(group.terms.length).fill(component), }; }), - }; + }); } const {component, ...otherFields} = providerResults; - return { + return deepFreezeAndThrowOnMutation({ ...otherFields, components: new Array(providerResults.terms.length).fill(component), - }; + }); } /** * Trims a list of results so that there are at most a maximum number of suggestions in it. If the results are grouped, * empty groups are also removed. - * - * This function modifies the provided results. */ export function trimResults(results: SuggestionResults, max: number) { + let trimmed: SuggestionResults; if ('groups' in results) { let remaining = max; + trimmed = { + ...results, + groups: [...results.groups], + }; + let i = 0; - while (i < results.groups.length && remaining > 0) { - const group = results.groups[i]; + while (i < trimmed.groups.length && remaining > 0) { + const group = trimmed.groups[i]; - group.items = group.items.slice(0, remaining); - group.terms = group.terms.slice(0, remaining); - group.components = group.components.slice(0, remaining); + trimmed.groups[i] = { + ...group, + items: group.items.slice(0, remaining), + terms: group.terms.slice(0, remaining), + components: group.components.slice(0, remaining), + }; remaining -= group.items.length; i += 1; } - if (i < results.groups.length) { - results.groups = results.groups.slice(0, i); + if (i < trimmed.groups.length) { + trimmed.groups = trimmed.groups.slice(0, i); } } else { - results.items = results.items.slice(0, max); - results.terms = results.terms.slice(0, max); - results.components = results.components.slice(0, max); + trimmed = { + ...results, + items: results.items.slice(0, max), + terms: results.terms.slice(0, max), + components: results.components.slice(0, max), + }; } - return results; + return deepFreezeAndThrowOnMutation(trimmed); }