various improvements to saved searches by sqs · Pull Request #63539 · sourcegraph/sourcegraph-public-snapshot

sqs · 2024-06-28T09:09:13Z

Remove long-deprecated and long-ineffective notifications for saved searches (removed in https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/commit/de8ae5ee28e5d942fe30c5493fd39b63fd1233f6 2.5 years ago). Note that code monitors were the replacement for saved searches and work great.
Clean up UI.
Make the UI global instead of in the user/org area.
Convert React class components to function components.
Add default patterntype: because it's required.
Use useQuery and useMutation instead of requestGraphQL.
Use a single namespace owner GraphQL arg instead of separating out userID and orgID.
Clean up GraphQL resolver code and factor out common auth checking.
Support transferring ownership of saved searches among owners (the user's own user account and the orgs they're a member of).

(I know this is not in Svelte.)

SECURITY: There is one substantive change. Site admins may now view any user's and any org's saved searches. This is so that they can audit and delete them if needed.

Test plan

Try creating, updating, and deleting saved searches, and transferring ownership of them.

Changelog

Improved the saved searches feature, which lets you save search queries to easily reuse them later and share them with other people in an organization.
Added the ability to transfer ownership of a saved search to a user's organizations or from an organization to a user's account.
Removed a long-deprecated and ineffective settings search.savedQueries field. You can manage saved searches in a user's or organization's profile area (e.g., at /user/searches).

+	// 🚨 SECURITY: Make sure the current user has permission for the specified namespace.
+	if err := CheckAuthorizedForNamespaceByIDs(ctx, db, *namespace); err != nil {
+		return nil, err
+	}
+	return namespace, nil


+	// 🚨 SECURITY: Make sure the current user has permission to get the saved search.
+	if err := graphqlbackend.CheckAuthorizedForNamespaceByIDs(ctx, r.db, ss.Owner); err != nil {
+		return nil, err
+	}
+
+	savedSearch := &savedSearchResolver{
+		db: r.db,
+		s:  *ss,
+	}
+	return savedSearch, nil


+		// 🚨 SECURITY: Make sure the current user has permission to view saved searches of the
+		// specified owner.
+		owner, err := graphqlbackend.CheckAuthorizedForNamespace(ctx, r.db, *args.Owner)
+		if err != nil {
+			return nil, err
+		}
+		connectionStore.listArgs.Owner = owner


+	// 🚨 SECURITY: Make sure the current user has permission to create a saved search in the
+	// specified owner namespace.
+	namespace, err := graphqlbackend.CheckAuthorizedForNamespace(ctx, r.db, args.Input.Owner)
+	if err != nil {
+		return nil, err
+	}
+
+	if !queryHasPatternType(args.Input.Query) {
+		return nil, errMissingPatternType
+	}
+
+	ss, err := r.db.SavedSearches().Create(ctx, &types.SavedSearch{
+		Description: args.Input.Description,
+		Query:       args.Input.Query,
+		Owner:       *namespace,
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	return r.toSavedSearchResolver(*ss), nil


+	// 🚨 SECURITY: ...and check the user can administer saved searches in the new owner's
+	// namespace:
+	newOwner, err := graphqlbackend.CheckAuthorizedForNamespace(ctx, r.db, args.NewOwner)
+	if err != nil {
+		return nil, err
+	}
+
+	ss, err = r.db.SavedSearches().UpdateOwner(ctx, id, *newOwner)
+	if err != nil {
+		return nil, err
+	}
+	return r.toSavedSearchResolver(*ss), nil


+// 🚨 SECURITY: This method does NOT verify the user's identity or that the user is an admin. It is
+// the caller's responsibility to ensure the user has proper permissions to perform the update.
+func (s *savedSearchStore) Update(ctx context.Context, savedSearch *types.SavedSearch) (updated *types.SavedSearch, err error) {
+	tr, ctx := trace.New(ctx, "database.SavedSearches.Update")
 	defer tr.EndWithErr(&err)

-	q := sqlf.Sprintf(`SELECT
-		id,
-		description,
-		query,
-		notify_owner,
-		notify_slack,
-		user_id,
-		org_id,
-		slack_webhook_url FROM saved_searches
-	`)
-	rows, err := s.Query(ctx, q)
-	if err != nil {
-		return nil, errors.Wrap(err, "QueryContext")
-	}
+	return s.update(ctx, savedSearch.ID, []*sqlf.Query{
+		sqlf.Sprintf("description=%s", savedSearch.Description),
+		sqlf.Sprintf("query=%s", savedSearch.Query),
+	})
+}

-	for rows.Next() {
-		var sq api.SavedQuerySpecAndConfig
-		if err := rows.Scan(
-			&sq.Config.Key,
-			&sq.Config.Description,
-			&sq.Config.Query,
-			&sq.Config.Notify,
-			&sq.Config.NotifySlack,
-			&sq.Config.UserID,
-			&sq.Config.OrgID,
-			&sq.Config.SlackWebhookURL); err != nil {
-			return nil, errors.Wrap(err, "Scan")
-		}
-		sq.Spec.Key = sq.Config.Key
-		if sq.Config.UserID != nil {
-			sq.Spec.Subject.User = sq.Config.UserID
-		} else if sq.Config.OrgID != nil {
-			sq.Spec.Subject.Org = sq.Config.OrgID
-		}
+// UpdateOwner updates the owner of an existing saved search.
+//
+// 🚨 SECURITY: This method does NOT verify that the user has permissions to do this. The caller
+// MUST do so.
+func (s *savedSearchStore) UpdateOwner(ctx context.Context, id int32, newOwner types.Namespace) (updated *types.SavedSearch, err error) {
+	tr, ctx := trace.New(ctx, "database.SavedSearches.UpdateOwner")
+	defer tr.EndWithErr(&err)
+	return s.update(ctx, id, []*sqlf.Query{
+		sqlf.Sprintf("user_id=%v", newOwner.User),
+		sqlf.Sprintf("org_id=%v", newOwner.Org),
+	})
+}

-		savedSearches = append(savedSearches, sq)
-	}
-	return savedSearches, nil
+func (s *savedSearchStore) update(ctx context.Context, id int32, updates []*sqlf.Query) (updated *types.SavedSearch, err error) {
+	updates = append(updates, sqlf.Sprintf("updated_at=now()"))
+	return scanSavedSearch(
+		s.QueryRow(ctx,
+			sqlf.Sprintf(
+				`UPDATE saved_searches SET %s WHERE id=%v RETURNING id, %v`,


camdencheek

I attempted to give this a real review (and also pulled it down and did some manual poking around), but it's also large, so relying a bit on your confidence 🙂

camdencheek · 2024-07-08T19:18:56Z

Thank you for splitting this out into its own file!

camdencheek · 2024-07-08T19:22:25Z

+    """
+    The owner of the saved search, either a user or organization.
+    """
+    owner: Namespace!


Maybe you came across this during the refactor: do you know why we have a special type for Namespace? Seems like it would be more ergonomic to just type it in GraphQL as User | Org.

Because we’ll want it to also support global and team namespaces. That was part of the original idea from years ago, and I think it’s finally nearing time for that again (with multi-tenant private code this year and supporting groupings within an org).

camdencheek · 2024-07-09T19:18:19Z

-	return &types.SavedSearches{
-		TotalSavedSearches:   int32(totalSavedSearches),
-		UniqueUsers:          int32(uniqueUsers),
-		NotificationsSent:    int32(notificationsSent),
-		NotificationsClicked: int32(notificationsClicked),
-		UniqueUserPageViews:  int32(uniqueUserPageViews),
-		OrgSavedSearches:     int32(orgSavedSearches),
-	}, nil


Previously, I think if we updated the payload shape for pings, we needed to update the schema found here. That might have changed, so also cc @dadlerj

@dadlerj said:

That JSON file should be updated yes and then the changes have to be published to BigQuery. I'll file a ticket on my team for us to go do all this after your PR.

It's fine to merge now though, unless this is an urgent patch release going out to customers asap... It'll be a while til this goes live and our changes only need to be done on our side.

Nice, thanks for following up 👍

camdencheek · 2024-07-09T19:18:49Z

+-- no default value
+ALTER TABLE saved_searches ALTER COLUMN notify_owner DROP DEFAULT;
+ALTER TABLE saved_searches ALTER COLUMN notify_slack DROP DEFAULT;


Thanks! I forgot to ever follow up on removing these columns.

- Remove long-deprecated and long-ineffective notifications for saved searches (removed in https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/commit/de8ae5ee28e5d942fe30c5493fd39b63fd1233f6 2.5 years ago). Note that code monitors were the replacement for saved searches and work great. - Clean up UI. - Make the UI global instead of in the user/org area. - Convert React class components to function components. - Add default `patterntype:` because it's required. - Use `useQuery` and `useMutation` instead of `requestGraphQL`. - Use a single namespace `owner` GraphQL arg instead of separating out `userID` and `orgID`. - Clean up GraphQL resolver code and factor out common auth checking. - Support transferring ownership of saved searches among owners (the user's own user account and the orgs they're a member of). (I know this is not in Svelte.) SECURITY: There is one substantive change. Site admins may now view any user's and any org's saved searches. This is so that they can audit and delete them if needed. ![image](https://github.com/sourcegraph/sourcegraph/assets/1976/7ba22c1c-b92e-4089-836b-135a503c96a0) ![image](https://github.com/sourcegraph/sourcegraph/assets/1976/a1f2f43d-f681-4ec9-b2a1-8273707b34ee) ![image](https://github.com/sourcegraph/sourcegraph/assets/1976/5ee1164f-ed2e-4144-9aca-db61fa7c20f4) ![image](https://github.com/sourcegraph/sourcegraph/assets/1976/d631529e-6c0d-49c6-9be1-33a7ff53ed97) Test plan: Try creating, updating, and deleting saved searches, and transferring ownership of them. Changelog: - Improved the saved searches feature, which lets you save search queries to easily reuse them later and share them with other people in an organization. - Added the ability to transfer ownership of a saved search to a user's organizations or from an organization to a user's account. - Removed a long-deprecated and ineffective settings `search.savedQueries` field. You can manage saved searches in a user's or organization's profile area (e.g., at `/user/searches`).

sqs requested a review from a team June 28, 2024 09:09

cla-bot Bot added the cla-signed label Jun 28, 2024

sqs changed the base branch from main to sqs/query-form-control June 28, 2024 09:09

sqs force-pushed the sqs/query-form-control branch from dadf645 to 1288e3f Compare June 29, 2024 03:55

sqs force-pushed the sqs/modernize-saved-searches branch from 4029982 to 965bfe9 Compare June 29, 2024 03:57

sqs force-pushed the sqs/query-form-control branch from 1288e3f to f3c38f9 Compare June 29, 2024 04:10

sqs force-pushed the sqs/modernize-saved-searches branch 2 times, most recently from e89f99d to 34a18ab Compare June 29, 2024 04:56

sqs force-pushed the sqs/query-form-control branch from f3c38f9 to 117daec Compare June 29, 2024 05:03

sqs force-pushed the sqs/modernize-saved-searches branch 3 times, most recently from 915302a to 600bab0 Compare June 29, 2024 05:24

sqs force-pushed the sqs/query-form-control branch from 117daec to 87057f5 Compare June 29, 2024 05:25

sqs force-pushed the sqs/modernize-saved-searches branch 2 times, most recently from 644de16 to 09a049e Compare June 29, 2024 06:23

github-advanced-security AI found potential problems Jun 29, 2024

View reviewed changes

sqs force-pushed the sqs/modernize-saved-searches branch 2 times, most recently from 2d59b24 to 460f423 Compare June 29, 2024 20:01

sqs marked this pull request as draft July 1, 2024 05:40

sqs force-pushed the sqs/query-form-control branch 2 times, most recently from 19d47ec to 0821d14 Compare July 2, 2024 00:13

Base automatically changed from sqs/query-form-control to main July 2, 2024 00:34

sqs force-pushed the sqs/modernize-saved-searches branch from 460f423 to 40b67f5 Compare July 3, 2024 10:16

github-advanced-security AI found potential problems Jul 3, 2024

View reviewed changes

sqs mentioned this pull request Jul 3, 2024

fetch NamespaceSelector namespaces from GraphQL (viewer { affiliatedNamespaces }) #63592

Merged

sqs force-pushed the sqs/modernize-saved-searches branch 5 times, most recently from 659dc81 to b9ab5c0 Compare July 5, 2024 13:40

sqs marked this pull request as ready for review July 5, 2024 13:40

github-advanced-security AI found potential problems Jul 5, 2024

View reviewed changes

sqs force-pushed the sqs/modernize-saved-searches branch 3 times, most recently from 745d1b9 to 5f87978 Compare July 5, 2024 15:47

camdencheek approved these changes Jul 9, 2024

View reviewed changes

sqs force-pushed the sqs/modernize-saved-searches branch 3 times, most recently from 1933074 to 67dafc0 Compare July 14, 2024 23:16

github-advanced-security AI found potential problems Jul 14, 2024

View reviewed changes

sqs force-pushed the sqs/modernize-saved-searches branch 3 times, most recently from 1851293 to 1fbf274 Compare July 15, 2024 02:43

github-advanced-security AI found potential problems Jul 15, 2024

View reviewed changes

sqs force-pushed the sqs/modernize-saved-searches branch 3 times, most recently from 5747699 to 63cb9ce Compare July 15, 2024 05:13

sqs changed the base branch from main to sqs/gql-conn July 15, 2024 19:01

sqs force-pushed the sqs/modernize-saved-searches branch from 63cb9ce to 919aa0e Compare July 15, 2024 19:01

sqs force-pushed the sqs/gql-conn branch from c627887 to 5c1d86c Compare July 15, 2024 19:11

sqs force-pushed the sqs/modernize-saved-searches branch from 919aa0e to aa65e93 Compare July 15, 2024 19:11

Base automatically changed from sqs/gql-conn to main July 15, 2024 19:18

sqs force-pushed the sqs/modernize-saved-searches branch from aa65e93 to 37928cb Compare July 15, 2024 20:01

sqs enabled auto-merge (squash) July 15, 2024 20:01

sqs merged commit 7ba706e into main Jul 15, 2024

sqs deleted the sqs/modernize-saved-searches branch July 15, 2024 20:12

Conversation

sqs commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Changelog

Uh oh!

Uh oh!

Check notice

Check notice

Check notice

Check notice

Uh oh!

Check notice

Check notice

Check notice

Uh oh!

Uh oh!

Uh oh!

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

camdencheek left a comment

Choose a reason for hiding this comment

Uh oh!

camdencheek Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

camdencheek Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

sqs Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

camdencheek Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

sqs Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

camdencheek Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

camdencheek Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Check notice

Check notice

Check notice

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sqs commented Jun 28, 2024 •

edited

Loading

camdencheek Jul 9, 2024 •

edited

Loading