diff --git a/admin/database/database.go b/admin/database/database.go index 88d6e58830f7..161157b52a9e 100644 --- a/admin/database/database.go +++ b/admin/database/database.go @@ -300,6 +300,8 @@ type DB interface { InsertProjectAccessRequest(ctx context.Context, opts *InsertProjectAccessRequestOptions) (*ProjectAccessRequest, error) DeleteProjectAccessRequest(ctx context.Context, id string) error + // FindBookmarks returns the bookmarks in a project that are visible to the user: their own plus shared and default ones. + // resourceKind and resourceName are optional filters; when both are empty, all bookmarks in the project are returned. FindBookmarks(ctx context.Context, projectID, resourceKind, resourceName, userID string) ([]*Bookmark, error) FindBookmark(ctx context.Context, bookmarkID string) (*Bookmark, error) FindDefaultBookmark(ctx context.Context, projectID, resourceKind, resourceName string) (*Bookmark, error) diff --git a/admin/database/postgres/postgres.go b/admin/database/postgres/postgres.go index 6b600f47c36e..715e2a3b8ae8 100644 --- a/admin/database/postgres/postgres.go +++ b/admin/database/postgres/postgres.go @@ -2852,11 +2852,23 @@ func (c *connection) DeleteProjectAccessRequest(ctx context.Context, id string) return checkDeleteRow("project access request", res, err) } -// FindBookmarks returns a list of bookmarks for a user per project +// FindBookmarks returns the bookmarks in a project that are visible to the user. +// resourceKind and resourceName are optional filters; when empty, they are not applied. func (c *connection) FindBookmarks(ctx context.Context, projectID, resourceKind, resourceName, userID string) ([]*database.Bookmark, error) { + qry := `SELECT * FROM bookmarks WHERE project_id = $1 AND (user_id = $2 OR shared = true OR "default" = true)` + args := []any{projectID, userID} + if resourceKind != "" { + args = append(args, resourceKind) + qry += fmt.Sprintf(" AND resource_kind = $%d", len(args)) + } + if resourceName != "" { + args = append(args, resourceName) + qry += fmt.Sprintf(" AND lower(resource_name) = lower($%d)", len(args)) + } + qry += " ORDER BY lower(display_name), created_on" + var res []*database.Bookmark - err := c.getDB(ctx).SelectContext(ctx, &res, `SELECT * FROM bookmarks WHERE project_id = $1 and resource_kind = $2 and lower(resource_name) = lower($3) and (user_id = $4 or shared = true or "default" = true)`, - projectID, resourceKind, resourceName, userID) + err := c.getDB(ctx).SelectContext(ctx, &res, qry, args...) if err != nil { return nil, parseErr("bookmarks", err) } @@ -2903,7 +2915,7 @@ func (c *connection) UpdateBookmark(ctx context.Context, opts *database.UpdateBo if err := database.Validate(opts); err != nil { return err } - res, err := c.getDB(ctx).ExecContext(ctx, `UPDATE bookmarks SET display_name=$1, description=$2, url_search=$3, shared=$4 WHERE id=$5`, + res, err := c.getDB(ctx).ExecContext(ctx, `UPDATE bookmarks SET display_name=$1, description=$2, url_search=$3, shared=$4, updated_on=now() WHERE id=$5`, opts.DisplayName, opts.Description, opts.URLSearch, opts.Shared, opts.BookmarkID) return checkUpdateRow("bookmark", res, err) } diff --git a/admin/server/bookmarks.go b/admin/server/bookmarks.go index c166ff2c5f8e..07d813b9ee34 100644 --- a/admin/server/bookmarks.go +++ b/admin/server/bookmarks.go @@ -12,7 +12,8 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" ) -// ListBookmarks server returns the bookmarks for the user per project +// ListBookmarks returns the bookmarks in a project that are visible to the user: their own plus shared and default ones. +// The resource kind and name are optional filters; when both are empty, all bookmarks in the project are returned. func (s *Server) ListBookmarks(ctx context.Context, req *adminv1.ListBookmarksRequest) (*adminv1.ListBookmarksResponse, error) { claims := auth.GetClaims(ctx) // Error if authenticated as anything other than a user @@ -20,6 +21,25 @@ func (s *Server) ListBookmarks(ctx context.Context, req *adminv1.ListBookmarksRe return nil, status.Error(codes.Unauthenticated, "not authenticated as a user") } + if req.ResourceName != "" && req.ResourceKind == "" { + return nil, status.Error(codes.InvalidArgument, "resource_kind is required when resource_name is set") + } + + proj, err := s.admin.DB.FindProject(ctx, req.ProjectId) + if err != nil { + return nil, err + } + + permissions := claims.ProjectPermissions(ctx, proj.OrganizationID, proj.ID) + if proj.Public { + permissions.ReadProject = true + permissions.ReadProd = true + } + + if !permissions.ReadProject { + return nil, status.Error(codes.PermissionDenied, "does not have permission to read the project") + } + bookmarks, err := s.admin.DB.FindBookmarks(ctx, req.ProjectId, req.ResourceKind, req.ResourceName, claims.OwnerID()) if err != nil { return nil, err diff --git a/admin/server/bookmarks_test.go b/admin/server/bookmarks_test.go new file mode 100644 index 000000000000..55e118b46d54 --- /dev/null +++ b/admin/server/bookmarks_test.go @@ -0,0 +1,143 @@ +package server_test + +import ( + "context" + "testing" + + "github.com/rilldata/rill/admin/database" + "github.com/rilldata/rill/admin/testadmin" + adminv1 "github.com/rilldata/rill/proto/gen/rill/admin/v1" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func TestBookmarks(t *testing.T) { + ctx := context.Background() + fix := testadmin.New(t) + + const exploreKind = "rill.runtime.v1.Explore" + const canvasKind = "rill.runtime.v1.Canvas" + + // Create an admin user with an org and a project. + _, admin := fix.NewUser(t) + org, err := admin.CreateOrganization(ctx, &adminv1.CreateOrganizationRequest{Name: randomName()}) + require.NoError(t, err) + proj, err := admin.CreateProject(ctx, &adminv1.CreateProjectRequest{ + Org: org.Organization.Name, + Project: "proj1", + ProdSlots: 1, + SkipDeploy: true, + }) + require.NoError(t, err) + projectID := proj.Project.Id + + // Add a viewer to the project. + viewerUser, viewer := fix.NewUser(t) + _, err = admin.AddProjectMemberUser(ctx, &adminv1.AddProjectMemberUserRequest{ + Org: org.Organization.Name, + Project: proj.Project.Name, + Email: viewerUser.Email, + Role: database.ProjectRoleNameViewer, + }) + require.NoError(t, err) + + // A user that is not a member of the project. + _, outsider := fix.NewUser(t) + + // Bookmarks across two dashboards: a shared one and a personal one by the admin, and a personal one by the viewer. + shared, err := admin.CreateBookmark(ctx, &adminv1.CreateBookmarkRequest{ + DisplayName: "Shared explore bookmark", + ProjectId: projectID, + ResourceKind: exploreKind, + ResourceName: "explore1", + Shared: true, + UrlSearch: "?tr=P7D", + }) + require.NoError(t, err) + _, err = admin.CreateBookmark(ctx, &adminv1.CreateBookmarkRequest{ + DisplayName: "Admin personal canvas bookmark", + ProjectId: projectID, + ResourceKind: canvasKind, + ResourceName: "canvas1", + UrlSearch: "?tr=P1D", + }) + require.NoError(t, err) + viewerPersonal, err := viewer.CreateBookmark(ctx, &adminv1.CreateBookmarkRequest{ + DisplayName: "Viewer personal explore bookmark", + ProjectId: projectID, + ResourceKind: exploreKind, + ResourceName: "explore1", + UrlSearch: "?tr=P30D", + }) + require.NoError(t, err) + + t.Run("project-wide listing returns own, shared and default bookmarks in a stable order", func(t *testing.T) { + res, err := admin.ListBookmarks(ctx, &adminv1.ListBookmarksRequest{ProjectId: projectID}) + require.NoError(t, err) + require.Equal(t, []string{"Admin personal canvas bookmark", "Shared explore bookmark"}, bookmarkNames(res.Bookmarks)) + + res, err = viewer.ListBookmarks(ctx, &adminv1.ListBookmarksRequest{ProjectId: projectID}) + require.NoError(t, err) + require.Equal(t, []string{"Shared explore bookmark", "Viewer personal explore bookmark"}, bookmarkNames(res.Bookmarks)) + }) + + t.Run("listing can be filtered by resource", func(t *testing.T) { + res, err := admin.ListBookmarks(ctx, &adminv1.ListBookmarksRequest{ProjectId: projectID, ResourceKind: exploreKind, ResourceName: "explore1"}) + require.NoError(t, err) + require.Equal(t, []string{"Shared explore bookmark"}, bookmarkNames(res.Bookmarks)) + + // The resource name is matched case-insensitively. + res, err = viewer.ListBookmarks(ctx, &adminv1.ListBookmarksRequest{ProjectId: projectID, ResourceKind: exploreKind, ResourceName: "EXPLORE1"}) + require.NoError(t, err) + require.Equal(t, []string{"Shared explore bookmark", "Viewer personal explore bookmark"}, bookmarkNames(res.Bookmarks)) + + // Filtering by kind only. + res, err = admin.ListBookmarks(ctx, &adminv1.ListBookmarksRequest{ProjectId: projectID, ResourceKind: canvasKind}) + require.NoError(t, err) + require.Equal(t, []string{"Admin personal canvas bookmark"}, bookmarkNames(res.Bookmarks)) + + // A resource name without a kind is rejected. + _, err = admin.ListBookmarks(ctx, &adminv1.ListBookmarksRequest{ProjectId: projectID, ResourceName: "explore1"}) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + }) + + t.Run("listing requires read access to the project", func(t *testing.T) { + _, err := outsider.ListBookmarks(ctx, &adminv1.ListBookmarksRequest{ProjectId: projectID}) + require.Equal(t, codes.PermissionDenied, status.Code(err)) + }) + + t.Run("updating a bookmark bumps updated_on", func(t *testing.T) { + before, err := viewer.GetBookmark(ctx, &adminv1.GetBookmarkRequest{BookmarkId: viewerPersonal.Bookmark.Id}) + require.NoError(t, err) + + _, err = viewer.UpdateBookmark(ctx, &adminv1.UpdateBookmarkRequest{ + BookmarkId: viewerPersonal.Bookmark.Id, + DisplayName: "Viewer personal explore bookmark (renamed)", + UrlSearch: "?tr=P30D", + }) + require.NoError(t, err) + + after, err := viewer.GetBookmark(ctx, &adminv1.GetBookmarkRequest{BookmarkId: viewerPersonal.Bookmark.Id}) + require.NoError(t, err) + require.Equal(t, "Viewer personal explore bookmark (renamed)", after.Bookmark.DisplayName) + require.True(t, after.Bookmark.UpdatedOn.AsTime().After(before.Bookmark.UpdatedOn.AsTime())) + + // A viewer cannot update a shared bookmark. + _, err = viewer.UpdateBookmark(ctx, &adminv1.UpdateBookmarkRequest{ + BookmarkId: shared.Bookmark.Id, + DisplayName: "Hijacked", + Shared: true, + UrlSearch: "?tr=P7D", + }) + require.Equal(t, codes.PermissionDenied, status.Code(err)) + }) +} + +func bookmarkNames(bookmarks []*adminv1.Bookmark) []string { + names := make([]string, len(bookmarks)) + for i, b := range bookmarks { + names[i] = b.DisplayName + } + return names +} diff --git a/proto/gen/rill/admin/v1/admin.swagger.yaml b/proto/gen/rill/admin/v1/admin.swagger.yaml index 688313b92244..8c90b02f5897 100644 --- a/proto/gen/rill/admin/v1/admin.swagger.yaml +++ b/proto/gen/rill/admin/v1/admin.swagger.yaml @@ -4650,10 +4650,14 @@ paths: required: false type: string - name: resourceKind + description: |- + Optional filter on the kind of the resource the bookmark is for (e.g. "rill.runtime.v1.Explore"). + When both resource_kind and resource_name are empty, all bookmarks in the project are returned. in: query required: false type: string - name: resourceName + description: Optional filter on the name of the resource the bookmark is for. Requires resource_kind to be set. in: query required: false type: string diff --git a/proto/gen/rill/admin/v1/api.pb.go b/proto/gen/rill/admin/v1/api.pb.go index f30d1e54f269..5938b2562d38 100644 --- a/proto/gen/rill/admin/v1/api.pb.go +++ b/proto/gen/rill/admin/v1/api.pb.go @@ -13020,13 +13020,18 @@ func (*RevokeCurrentAuthTokenResponse) Descriptor() ([]byte, []int) { return file_rill_admin_v1_api_proto_rawDescGZIP(), []int{211} } +// ListBookmarksRequest lists the bookmarks in a project that are visible to the caller: +// the caller's own bookmarks plus shared and default bookmarks. type ListBookmarksRequest struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - ProjectId string `protobuf:"bytes,1,opt,name=project_id,json=projectId,proto3" json:"project_id,omitempty"` + ProjectId string `protobuf:"bytes,1,opt,name=project_id,json=projectId,proto3" json:"project_id,omitempty"` + // Optional filter on the kind of the resource the bookmark is for (e.g. "rill.runtime.v1.Explore"). + // When both resource_kind and resource_name are empty, all bookmarks in the project are returned. ResourceKind string `protobuf:"bytes,2,opt,name=resource_kind,json=resourceKind,proto3" json:"resource_kind,omitempty"` + // Optional filter on the name of the resource the bookmark is for. Requires resource_kind to be set. ResourceName string `protobuf:"bytes,3,opt,name=resource_name,json=resourceName,proto3" json:"resource_name,omitempty"` } diff --git a/proto/gen/rill/admin/v1/openapi.yaml b/proto/gen/rill/admin/v1/openapi.yaml index 1fbbb11a55b7..1cc64414ef7d 100644 --- a/proto/gen/rill/admin/v1/openapi.yaml +++ b/proto/gen/rill/admin/v1/openapi.yaml @@ -8153,11 +8153,15 @@ paths: name: projectId schema: type: string - - in: query + - description: |- + Optional filter on the kind of the resource the bookmark is for (e.g. "rill.runtime.v1.Explore"). + When both resource_kind and resource_name are empty, all bookmarks in the project are returned. + in: query name: resourceKind schema: type: string - - in: query + - description: Optional filter on the name of the resource the bookmark is for. Requires resource_kind to be set. + in: query name: resourceName schema: type: string diff --git a/proto/rill/admin/v1/api.proto b/proto/rill/admin/v1/api.proto index ee056e2c3b03..076b53c0cf30 100644 --- a/proto/rill/admin/v1/api.proto +++ b/proto/rill/admin/v1/api.proto @@ -2650,9 +2650,14 @@ message RevokeCurrentAuthTokenRequest {} message RevokeCurrentAuthTokenResponse { } +// ListBookmarksRequest lists the bookmarks in a project that are visible to the caller: +// the caller's own bookmarks plus shared and default bookmarks. message ListBookmarksRequest { string project_id = 1; + // Optional filter on the kind of the resource the bookmark is for (e.g. "rill.runtime.v1.Explore"). + // When both resource_kind and resource_name are empty, all bookmarks in the project are returned. string resource_kind = 2; + // Optional filter on the name of the resource the bookmark is for. Requires resource_kind to be set. string resource_name = 3; } diff --git a/web-admin/src/client/gen/index.schemas.ts b/web-admin/src/client/gen/index.schemas.ts index 4084393a1cf0..71c8b0bd7538 100644 --- a/web-admin/src/client/gen/index.schemas.ts +++ b/web-admin/src/client/gen/index.schemas.ts @@ -2534,7 +2534,14 @@ This is only allowed for superusers. */ export type AdminServiceListBookmarksParams = { projectId?: string; + /** + * Optional filter on the kind of the resource the bookmark is for (e.g. "rill.runtime.v1.Explore"). +When both resource_kind and resource_name are empty, all bookmarks in the project are returned. + */ resourceKind?: string; + /** + * Optional filter on the name of the resource the bookmark is for. Requires resource_kind to be set. + */ resourceName?: string; }; diff --git a/web-admin/src/features/bookmarks/BookmarkMetadataDialog.svelte b/web-admin/src/features/bookmarks/BookmarkMetadataDialog.svelte new file mode 100644 index 000000000000..15e20757045b --- /dev/null +++ b/web-admin/src/features/bookmarks/BookmarkMetadataDialog.svelte @@ -0,0 +1,145 @@ + + + { + if (!o) onClose(); + }} +> + + + {m.bookmark_edit()} + + +
{ + e.preventDefault(); + submit(e); + }} + > + + + {#if !bookmark.default} + +