-
Notifications
You must be signed in to change notification settings - Fork 236
fix: Exclude user page data from search response when user pages are disabled #10740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
fdd9809
Block user page searches when blocking setting is on
arvid-e ee6e4f0
Move solution to normalizeQueryString method
arvid-e 701e3cb
Update function name
arvid-e 2e31b33
Change names and use imported config maager
arvid-e 34134fa
Fix correct function import name
arvid-e 4aaefb8
Update user page exclusion function
arvid-e 68817a3
Revert previous solution
arvid-e 760cd9e
Update function name
arvid-e b95c44f
Test and remove user prefixes using regex
arvid-e ab14f02
Merge branch 'master' of https://github.com/growilabs/growi into fix/…
arvid-e a997318
Write test for user page exclusion from query
arvid-e 2074dad
Update page exclusion function to not match double slashes
arvid-e c1fa65a
Add more test cases
arvid-e be0200c
Write test for searchParseQuery in regards to disabled user pages
arvid-e 26bb194
Add test case when using alias
arvid-e 8be013a
Filter user pages also after using alias
arvid-e 312abf7
Check for existing config key
arvid-e 69e7a46
Remove import
arvid-e 88931d1
Replace spy calls with overrides
arvid-e 0e06250
Define types to get rid of as any
arvid-e 2db9311
Only return parsedQuery once in parsedQuerySearch
arvid-e File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
150 changes: 150 additions & 0 deletions
150
apps/app/src/features/search/utils/disable-user-pages.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| import type { QueryTerms } from '~/server/interfaces/search'; | ||
|
|
||
| import { excludeUserPagesFromQuery } from './disable-user-pages'; | ||
|
|
||
| describe('excludeUserPagesFromQuery()', () => { | ||
| it('should exclude user page strings from query prefix', () => { | ||
| const userString = '/user'; | ||
| const userStringSlash = '/user/'; | ||
| const userStringSubPath = '/user/settings'; | ||
| const userStringSubPathDeep = '/user/profile/edit'; | ||
| const userStringSubPathQuery = '/user/settings?ref=top'; | ||
|
|
||
| const query: QueryTerms = { | ||
| match: [], | ||
| not_match: [], | ||
| phrase: [], | ||
| not_phrase: [], | ||
| prefix: [ | ||
| userString, | ||
| userStringSlash, | ||
| userStringSubPath, | ||
| userStringSubPathDeep, | ||
| userStringSubPathQuery, | ||
| ], | ||
| not_prefix: [], | ||
| tag: [], | ||
| not_tag: [], | ||
| }; | ||
|
|
||
| excludeUserPagesFromQuery(query); | ||
|
|
||
| expect(query.prefix).not.toContain(userString); | ||
| // Should only contain '/user' | ||
| expect(query.not_prefix).toContain(userString); | ||
|
|
||
| expect(query.prefix).not.toContain(userStringSlash); | ||
| expect(query.not_prefix).not.toContain(userStringSlash); | ||
|
|
||
| expect(query.prefix).not.toContain(userStringSubPath); | ||
| expect(query.not_prefix).not.toContain(userStringSubPath); | ||
|
|
||
| expect(query.prefix).not.toContain(userStringSubPathDeep); | ||
| expect(query.not_prefix).not.toContain(userStringSubPathDeep); | ||
|
|
||
| expect(query.prefix).not.toContain(userStringSubPathQuery); | ||
| expect(query.not_prefix).not.toContain(userStringSubPathQuery); | ||
| }); | ||
|
|
||
| it('should not exclude strings similar to /user from query prefix', () => { | ||
| const userRouter = '/userouter'; | ||
| const userRoot = '/useroot'; | ||
| const userSettings = '/user-settings'; | ||
| const apiUser = '/api/user'; | ||
| const emptyString = ''; | ||
| const rootOnly = '/'; | ||
| const upperCase = '/USER'; | ||
| const doubleSlashStart = '//user/new'; | ||
| const doubleSlashSub = '/user//new'; | ||
|
|
||
| const query: QueryTerms = { | ||
| match: [], | ||
| not_match: [], | ||
| phrase: [], | ||
| not_phrase: [], | ||
| prefix: [ | ||
| userRouter, | ||
| userRoot, | ||
| userSettings, | ||
| apiUser, | ||
| emptyString, | ||
| rootOnly, | ||
| upperCase, | ||
| doubleSlashStart, | ||
| doubleSlashSub, | ||
| ], | ||
| not_prefix: [], | ||
| tag: [], | ||
| not_tag: [], | ||
| }; | ||
|
|
||
| excludeUserPagesFromQuery(query); | ||
|
|
||
| expect(query.prefix).toContain(userRouter); | ||
| expect(query.not_prefix).not.toContain(userRouter); | ||
|
|
||
| expect(query.prefix).toContain(userRoot); | ||
| expect(query.not_prefix).not.toContain(userRoot); | ||
|
|
||
| expect(query.prefix).toContain(userSettings); | ||
| expect(query.not_prefix).not.toContain(userSettings); | ||
|
|
||
| expect(query.prefix).toContain(apiUser); | ||
| expect(query.not_prefix).not.toContain(apiUser); | ||
|
|
||
| expect(query.prefix).toContain(emptyString); | ||
| expect(query.not_prefix).not.toContain(emptyString); | ||
|
|
||
| expect(query.prefix).toContain(rootOnly); | ||
| expect(query.not_prefix).not.toContain(rootOnly); | ||
|
|
||
| expect(query.prefix).toContain(upperCase); | ||
| expect(query.not_prefix).not.toContain(upperCase); | ||
|
|
||
| expect(query.prefix).toContain(doubleSlashStart); | ||
| expect(query.not_prefix).not.toContain(doubleSlashStart); | ||
|
|
||
| expect(query.prefix).toContain(doubleSlashSub); | ||
| expect(query.not_prefix).not.toContain(doubleSlashSub); | ||
| }); | ||
|
|
||
| it('should add /user to not_prefix when it is empty', () => { | ||
| const query: QueryTerms = { | ||
| match: [], | ||
| not_match: [], | ||
| phrase: [], | ||
| not_phrase: [], | ||
| prefix: [], | ||
| not_prefix: [], | ||
| tag: [], | ||
| not_tag: [], | ||
| }; | ||
|
|
||
| excludeUserPagesFromQuery(query); | ||
|
|
||
| expect(query.prefix).toHaveLength(0); | ||
| expect(query.not_prefix).toContain('/user'); | ||
| expect(query.not_prefix).toHaveLength(1); | ||
| }); | ||
|
|
||
| it('should remove existing /user strings and leave not_prefix with just one /user string', () => { | ||
| const userString = '/user'; | ||
|
|
||
| const query: QueryTerms = { | ||
| match: [], | ||
| not_match: [], | ||
| phrase: [], | ||
| not_phrase: [], | ||
| prefix: [userString, userString], | ||
| not_prefix: [userString, userString], | ||
| tag: [], | ||
| not_tag: [], | ||
| }; | ||
|
|
||
| excludeUserPagesFromQuery(query); | ||
|
|
||
| expect(query.prefix).toHaveLength(0); | ||
| expect(query.not_prefix).toContain('/user'); | ||
| expect(query.not_prefix).toHaveLength(1); | ||
| }); | ||
| }); |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 単体テスト追加してください |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import type { QueryTerms } from '~/server/interfaces/search'; | ||
|
|
||
| export function excludeUserPagesFromQuery(terms: QueryTerms): void { | ||
| const userRegex: RegExp = /^\/user($|\/(?!\/))/; | ||
|
|
||
| terms.prefix = terms.prefix.filter((p) => !userRegex.test(p)); | ||
| terms.not_prefix = terms.not_prefix.filter((p) => !userRegex.test(p)); | ||
|
|
||
| terms.not_prefix.push('/user'); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| import { vi } from 'vitest'; | ||
| import { type MockProxy, mock } from 'vitest-mock-extended'; | ||
|
|
||
| import { SearchDelegatorName } from '~/interfaces/named-query'; | ||
| import type Crowi from '~/server/crowi'; | ||
| import { configManager } from '~/server/service/config-manager/config-manager'; | ||
|
|
||
| import type { SearchDelegator } from '../interfaces/search'; | ||
| import NamedQuery from '../models/named-query'; | ||
| import SearchService from './search'; | ||
| import type ElasticsearchDelegator from './search-delegator/elasticsearch'; | ||
|
|
||
| // Mock NamedQuery | ||
| vi.mock('~/server/models/named-query', () => { | ||
| const mockModel = { | ||
| findOne: vi.fn(), | ||
| }; | ||
| return { | ||
| NamedQuery: mockModel, | ||
| default: mockModel, | ||
| }; | ||
| }); | ||
|
|
||
| // Mock config manager | ||
| vi.mock('~/server/service/config-manager/config-manager', () => { | ||
| return { | ||
| default: { | ||
| getConfig: vi.fn(), | ||
| }, | ||
| configManager: { | ||
| getConfig: vi.fn(), | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| class TestSearchService extends SearchService { | ||
| override generateFullTextSearchDelegator(): ElasticsearchDelegator { | ||
| return mock<ElasticsearchDelegator>(); | ||
| } | ||
|
|
||
| override generateNQDelegators(): { | ||
| [key in SearchDelegatorName]: SearchDelegator; | ||
| } { | ||
| return { | ||
| [SearchDelegatorName.DEFAULT]: mock<SearchDelegator>(), | ||
| [SearchDelegatorName.PRIVATE_LEGACY_PAGES]: mock<SearchDelegator>(), | ||
| }; | ||
| } | ||
|
|
||
| override registerUpdateEvent(): void {} | ||
|
|
||
| override get isConfigured(): boolean { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| describe('searchParseQuery()', () => { | ||
| let searchService: TestSearchService; | ||
| let mockCrowi: MockProxy<Crowi>; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
|
|
||
| mockCrowi = mock<Crowi>(); | ||
| mockCrowi.configManager = configManager; | ||
| searchService = new TestSearchService(mockCrowi); | ||
| }); | ||
|
|
||
| it('should contain /user in the not_prefix query when user pages are disabled', async () => { | ||
| vi.mocked(configManager.getConfig).mockImplementation((key: string) => { | ||
| if (key === 'security:disableUserPages') { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| }); | ||
|
|
||
| const result = await searchService.parseSearchQuery('/user/settings', null); | ||
|
|
||
| expect(configManager.getConfig).toHaveBeenCalledWith( | ||
| 'security:disableUserPages', | ||
| ); | ||
yuki-takei marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| expect(result.terms.not_prefix).toContain('/user'); | ||
| expect(result.terms.prefix).toHaveLength(0); | ||
| }); | ||
|
|
||
| it('should contain /user in the not_prefix even when search query is not a user page', async () => { | ||
| vi.mocked(configManager.getConfig).mockImplementation((key: string) => { | ||
| if (key === 'security:disableUserPages') { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| }); | ||
|
|
||
| const result = await searchService.parseSearchQuery('/new-task', null); | ||
|
|
||
| expect(configManager.getConfig).toHaveBeenCalledWith( | ||
| 'security:disableUserPages', | ||
| ); | ||
| expect(result.terms.not_prefix).toContain('/user'); | ||
| expect(result.terms.prefix).toHaveLength(0); | ||
| }); | ||
|
|
||
| it('should add specific user prefixes in the query when user pages are enabled', async () => { | ||
| vi.mocked(configManager.getConfig).mockImplementation((key: string) => { | ||
| if (key === 'security:disableUserPages') { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| }); | ||
|
|
||
| const result = await searchService.parseSearchQuery('/user/settings', null); | ||
|
|
||
| expect(configManager.getConfig).toHaveBeenCalledWith( | ||
| 'security:disableUserPages', | ||
| ); | ||
| expect(result.terms.not_prefix).not.toContain('/user'); | ||
| expect(result.terms.not_prefix).not.toContain('/user/settings'); | ||
| expect(result.terms.match).toContain('/user/settings'); | ||
| }); | ||
|
|
||
| it('should filter user pages even when resolved from a named query alias', async () => { | ||
| vi.mocked(configManager.getConfig).mockImplementation((key: string) => { | ||
| if (key === 'security:disableUserPages') { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| }); | ||
|
|
||
| const shortcutName = 'my-shortcut'; | ||
| const aliasPath = '/user/my-private-page'; | ||
|
|
||
| // Mock the DB response | ||
| vi.mocked(NamedQuery.findOne).mockResolvedValue({ | ||
| name: shortcutName, | ||
| aliasOf: aliasPath, | ||
| }); | ||
|
|
||
| const result = await searchService.parseSearchQuery('dummy', shortcutName); | ||
|
|
||
| expect(configManager.getConfig).toHaveBeenCalledWith( | ||
| 'security:disableUserPages', | ||
| ); | ||
| expect(result.terms.not_prefix).toContain('/user'); | ||
| expect(result.terms.match).toContain('/user/my-private-page'); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.