Skip to content
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

Only allow exact-matching of workspace search #8632

Merged
merged 1 commit into from
Mar 7, 2022
Merged

Conversation

laushinka
Copy link
Contributor

Description

This PR disables empty string and partial match search.

Related Issue(s)

Fixes #8614

How to test

  1. Go to https://lm-empty-8614.staging.gitpod-dev.com/admin/workspaces
  2. Click the Search button without any search queries. It should not do anything.
  3. Enter a partial match of a workspace ID. It should not do anything.
  4. Enter exact match of a workspace ID. It should return a result.

Release Notes

Admins cannot search empty strings or partial matches on workspace search.

Documentation

@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #8632 (3bd44bd) into main (103f835) will decrease coverage by 3.69%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8632      +/-   ##
==========================================
- Coverage   14.87%   11.17%   -3.70%     
==========================================
  Files          52       18      -34     
  Lines        4963      993    -3970     
==========================================
- Hits          738      111     -627     
+ Misses       4153      880    -3273     
+ Partials       72        2      -70     
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
components-ws-daemon-app ?
components-ws-daemon-lib ?
install-installer-raw-app ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...onents/ws-daemon/pkg/internal/session/workspace.go
components/ws-daemon/pkg/cpulimit/dispatch.go
...l/installer/pkg/components/ws-manager/tlssecret.go
components/ws-daemon/pkg/quota/xfs.go
components/ws-daemon/pkg/internal/session/store.go
...components/ws-manager/unpriviledged-rolebinding.go
components/ws-daemon/pkg/cpulimit/bandwidth.go
install/installer/pkg/common/display.go
install/installer/pkg/common/storage.go
.../installer/pkg/components/ws-manager/deployment.go
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 103f835...3bd44bd. Read the comment docs.

@laushinka laushinka requested review from gtsiolis and geropl March 7, 2022 09:59
@laushinka laushinka marked this pull request as ready for review March 7, 2022 09:59
@laushinka laushinka requested a review from a team March 7, 2022 09:59
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Mar 7, 2022
if (!props.user) { // In the workspace search page
// This only allows searching for exact-matches of workspace or instance IDs
if ((queryTerm.length === 0) ||
(!matchesNewWorkspaceIdExactly(queryTerm) &&
Copy link
Member

@geropl geropl Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should try to avoid calling the matches functions multiple times.

One way do to that is to move it too line 79 👇

There we could do:

if (!props.user) {
   if (queryTerm.length === 0 || (!query.instanceIdOrWorkspaceId && !query.workspaceId)) {
      return;
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point!

Copy link
Contributor Author

@laushinka laushinka Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geropl I was thinking we could return early when there is an empty search, and then check further down if they're partial matches, like so:

    const search = async () => {
        // Disables empty search on the workspace search page
        if (!props.user && queryTerm.length === 0) {
            return;
        }

        setSearching(true);
        try {
            ...
            if (!query.ownerId && !query.instanceIdOrWorkspaceId && !query.workspaceId) {
                return;
            }

What do you think?

@laushinka laushinka requested a review from geropl March 7, 2022 10:50
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thx for caring and improving safety! 🙏

@roboquat roboquat merged commit 728e515 into main Mar 7, 2022
@roboquat roboquat deleted the lm/empty-8614 branch March 7, 2022 10:56
@@ -105,6 +114,10 @@ export function WorkspaceSearch(props: Props) {
<button disabled={searching} onClick={search}>Search</button>
</div>
</div>
<div className={'flex rounded-xl bg-gray-200 dark:bg-gray-800 text-gray-600 dark:text-gray-400 p-2 w-2/3 mb-2'}>
<img className="w-4 h-4 m-1 ml-2 mr-4" alt="info" src={info} />
<span>Please enter complete IDs - this search does not perform partial-matching.</span>
Copy link
Contributor

@gtsiolis gtsiolis Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one hurt! Opened #8701 to improve this.

@gtsiolis gtsiolis mentioned this pull request Jul 22, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note size/S team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable empty string search on admin workspace search
4 participants