Skip to content
Permalink
Browse files Browse the repository at this point in the history
Saved searches: fix update permissions (#36674)
  • Loading branch information
camdencheek committed Jun 6, 2022
1 parent 057fd61 commit 2832d78
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 23 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -33,6 +33,7 @@ All notable changes to Sourcegraph are documented in this file.
### Fixed

- A common source of searcher evictions on kubernetes when running large structural searches. [#34828](https://github.com/sourcegraph/sourcegraph/issues/34828)
- An issue with permissions evaluation for saved searches

### Removed

Expand Down
38 changes: 16 additions & 22 deletions cmd/frontend/graphqlbackend/saved_searches.go
Expand Up @@ -196,35 +196,29 @@ func (r *schemaResolver) UpdateSavedSearch(ctx context.Context, args *struct {
OrgID *graphql.ID
UserID *graphql.ID
}) (*savedSearchResolver, error) {
var userID, orgID *int32
id, err := unmarshalSavedSearchID(args.ID)
if err != nil {
return nil, err
}

old, err := r.db.SavedSearches().GetByID(ctx, id)
if err != nil {
return nil, errors.Wrap(err, "fetch old saved search")
}

// 🚨 SECURITY: Make sure the current user has permission to update a saved search for the specified user or org.
if args.UserID != nil {
u, err := unmarshalSavedSearchID(*args.UserID)
if err != nil {
return nil, err
}
userID = &u
if err := backend.CheckSiteAdminOrSameUser(ctx, r.db, u); err != nil {
return nil, err
}
} else if args.OrgID != nil {
o, err := unmarshalSavedSearchID(*args.OrgID)
if err != nil {
if old.Config.UserID != nil {
if err := backend.CheckSiteAdminOrSameUser(ctx, r.db, *old.Config.UserID); err != nil {
return nil, err
}
orgID = &o
if err := backend.CheckOrgAccessOrSiteAdmin(ctx, r.db, o); err != nil {
} else if old.Config.OrgID != nil {
if err := backend.CheckOrgAccessOrSiteAdmin(ctx, r.db, *old.Config.OrgID); err != nil {
return nil, err
}
} else {
return nil, errors.New("failed to update saved search: no Org ID or User ID associated with saved search")
}

id, err := unmarshalSavedSearchID(args.ID)
if err != nil {
return nil, err
}

if !queryHasPatternType(args.Query) {
return nil, errMissingPatternType
}
Expand All @@ -235,8 +229,8 @@ func (r *schemaResolver) UpdateSavedSearch(ctx context.Context, args *struct {
Query: args.Query,
Notify: args.NotifyOwner,
NotifySlack: args.NotifySlack,
UserID: userID,
OrgID: orgID,
UserID: old.Config.UserID,
OrgID: old.Config.OrgID,
})
if err != nil {
return nil, err
Expand Down
115 changes: 114 additions & 1 deletion cmd/frontend/graphqlbackend/saved_searches_test.go
Expand Up @@ -7,7 +7,9 @@ import (

mockrequire "github.com/derision-test/go-mockgen/testutil/require"
"github.com/graph-gophers/graphql-go"
"github.com/stretchr/testify/require"

"github.com/sourcegraph/sourcegraph/cmd/frontend/backend"
"github.com/sourcegraph/sourcegraph/internal/actor"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/database"
Expand Down Expand Up @@ -219,6 +221,11 @@ func TestUpdateSavedSearch(t *testing.T) {
OrgID: savedSearch.OrgID,
}, nil
})
ss.GetByIDFunc.SetDefaultReturn(&api.SavedQuerySpecAndConfig{
Config: api.ConfigSavedQuery{
UserID: &key,
},
}, nil)

db := database.NewMockDB()
db.UsersFunc.SetDefaultReturn(users)
Expand All @@ -233,7 +240,13 @@ func TestUpdateSavedSearch(t *testing.T) {
NotifySlack bool
OrgID *graphql.ID
UserID *graphql.ID
}{ID: marshalSavedSearchID(key), Description: "updated query description", Query: "test type:diff patternType:regexp", OrgID: nil, UserID: &userID})
}{
ID: marshalSavedSearchID(key),
Description: "updated query description",
Query: "test type:diff patternType:regexp",
OrgID: nil,
UserID: &userID,
})
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -267,6 +280,106 @@ func TestUpdateSavedSearch(t *testing.T) {
}
}

func TestUpdateSavedSearchPermissions(t *testing.T) {
user1 := &types.User{ID: 42}
user2 := &types.User{ID: 43}
admin := &types.User{ID: 44, SiteAdmin: true}
org1 := &types.Org{ID: 42}
org2 := &types.Org{ID: 43}

cases := []struct {
execUser *types.User
ssUserID *int32
ssOrgID *int32
errIs error
}{{
execUser: user1,
ssUserID: &user1.ID,
errIs: nil,
}, {
execUser: user1,
ssUserID: &user2.ID,
errIs: &backend.InsufficientAuthorizationError{},
}, {
execUser: user1,
ssOrgID: &org1.ID,
errIs: nil,
}, {
execUser: user1,
ssOrgID: &org2.ID,
errIs: backend.ErrNotAnOrgMember,
}, {
execUser: admin,
ssOrgID: &user1.ID,
errIs: nil,
}, {
execUser: admin,
ssOrgID: &org1.ID,
errIs: nil,
}}

for _, tt := range cases {
t.Run("", func(t *testing.T) {
ctx := actor.WithActor(context.Background(), actor.FromUser(tt.execUser.ID))
users := database.NewMockUserStore()
users.GetByCurrentAuthUserFunc.SetDefaultHook(func(ctx context.Context) (*types.User, error) {
switch actor.FromContext(ctx).UID {
case user1.ID:
return user1, nil
case user2.ID:
return user2, nil
case admin.ID:
return admin, nil
default:
panic("bad actor")
}
})

savedSearches := database.NewMockSavedSearchStore()
savedSearches.UpdateFunc.SetDefaultHook(func(_ context.Context, ss *types.SavedSearch) (*types.SavedSearch, error) {
return ss, nil
})
savedSearches.GetByIDFunc.SetDefaultReturn(&api.SavedQuerySpecAndConfig{
Config: api.ConfigSavedQuery{
UserID: tt.ssUserID,
OrgID: tt.ssOrgID,
},
}, nil)

orgMembers := database.NewMockOrgMemberStore()
orgMembers.GetByOrgIDAndUserIDFunc.SetDefaultHook(func(_ context.Context, orgID int32, userID int32) (*types.OrgMembership, error) {
if orgID == userID {
return &types.OrgMembership{}, nil
}
return nil, nil
})

db := database.NewMockDB()
db.UsersFunc.SetDefaultReturn(users)
db.SavedSearchesFunc.SetDefaultReturn(savedSearches)
db.OrgMembersFunc.SetDefaultReturn(orgMembers)

_, err := (&schemaResolver{db: db}).UpdateSavedSearch(ctx, &struct {
ID graphql.ID
Description string
Query string
NotifyOwner bool
NotifySlack bool
OrgID *graphql.ID
UserID *graphql.ID
}{
ID: marshalSavedSearchID(1),
Query: "patterntype:literal",
})
if tt.errIs == nil {
require.NoError(t, err)
} else {
require.ErrorAs(t, err, &tt.errIs)
}
})
}
}

func TestDeleteSavedSearch(t *testing.T) {
ctx := context.Background()

Expand Down

0 comments on commit 2832d78

Please sign in to comment.