Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Conversation

@mrnugget
Copy link
Contributor

@mrnugget mrnugget commented May 28, 2020

This implements part 1/2 of https://github.com/sourcegraph/sourcegraph/issues/10809 by incorporating repository permission in the read-path of the campaigns resolvers and hiding information from users that don't have access to associated repositories.

The PR also depends on sourcegraph/src-cli#213, which will be merged and released after this PR in a src-cli minor version release. After that release I'll bump the min-required src-cli in sourcegraph.

What this PR does, in concrete terms:

  • Split up the Patch and ExternalChangeset types in the GraphQL schema into Patch/HiddenPatch and ExternalChangeset/HiddenExternalChangeset. The types should implement the same interface but the "hidden" variants hide most information besides their own ID and the campaign they're associated with.
  • In every ChangesetsConnection return a HiddenExternalChangeset when the user doesn't have access to the underlying repository.
  • In every PatchesConnection return a HiddenPatch when the user doesn't have access to the underlying repository.
  • In ChangesetByID,PatchByID return a hidden changeset/patch if the user doesn't have access to the repository. Note: This is a slight variation from the original task in the ticket, since we don't return a 404, but I think this makes far more sense, since we expose the "hidden" types with IDs.
  • In Campaign.status.errors filter out the errors if the user doesn't have access to the underlying repositories.
  • In Campaign.diffStat do not include stats for patches/changesets the user doesn't have access to.
  • In PatchSet.diffStat do not include stats for patches the user doesn't have access to.

In a follow-up PR I'm going to incorporate repository permissions into the mutations of the API.

Note: Merging is safe, since permission levels are already in place and we still have the admin check around campaigns.

Screenshots

screenshot_2020-06-02_16 33 30@2x

screenshot_2020-06-02_16 34 56@2x

screenshot_2020-06-02_16 36 05@2x

screenshot_2020-06-02_16 38 38@2x

@mrnugget mrnugget added campaigns RFC-157 Tickets related to RFC 157 - Campaigns authorization model labels May 28, 2020
@mrnugget mrnugget force-pushed the campaigns/repo-permissions branch from 2f71412 to f369569 Compare May 29, 2020 10:48
@mrnugget mrnugget changed the title WIP: Incorporate repository permissions in campaigns Hide information in campaigns based on repository permissions Jun 2, 2020
@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #11071 into master will increase coverage by 0.09%.
The diff coverage is 72.69%.

@@            Coverage Diff             @@
##           master   #11071      +/-   ##
==========================================
+ Coverage   46.45%   46.54%   +0.09%     
==========================================
  Files        1368     1373       +5     
  Lines       77325    77508     +183     
  Branches     6627     6582      -45     
==========================================
+ Hits        35919    36074     +155     
- Misses      37982    38004      +22     
- Partials     3424     3430       +6     
Flag Coverage Δ
#go 50.67% <79.75%> (+0.11%) ⬆️
#storybook 6.95% <ø> (ø)
#typescript 35.81% <52.38%> (+0.01%) ⬆️
#unit 46.33% <72.69%> (+0.09%) ⬆️
Impacted Files Coverage Δ
cmd/frontend/graphqlbackend/campaigns.go 0.00% <0.00%> (ø)
...e/internal/campaigns/resolvers/changeset_events.go 29.54% <0.00%> (-3.79%) ⬇️
web/src/enterprise/campaigns/detail/backend.ts 10.71% <ø> (ø)
...campaigns/detail/changesets/CampaignChangesets.tsx 61.53% <ø> (ø)
...prise/campaigns/detail/patches/CampaignPatches.tsx 0.00% <ø> (ø)
...prise/campaigns/detail/patches/HiddenPatchNode.tsx 0.00% <0.00%> (ø)
...se/campaigns/detail/patches/PatchInterfaceNode.tsx 0.00% <0.00%> (ø)
.../enterprise/campaigns/detail/patches/PatchNode.tsx 62.96% <ø> (ø)
...prise/campaigns/detail/patches/PatchSetPatches.tsx 0.00% <ø> (ø)
...rise/campaigns/detail/changesets/ChangesetNode.tsx 25.00% <25.00%> (-24.06%) ⬇️
... and 21 more

@sqs
Copy link
Member

sqs commented Jun 2, 2020

"Changeset/patch in a private repository" is a fine label for now. I will think of what it should be (and how it should look), and that doesn't need to block this PR. 👍

@mrnugget mrnugget marked this pull request as ready for review June 3, 2020 12:33
@mrnugget mrnugget requested review from a team and emidoots as code owners June 3, 2020 12:33
@mrnugget mrnugget requested review from a team and removed request for a team and emidoots June 3, 2020 12:33
@mrnugget mrnugget force-pushed the campaigns/repo-permissions branch from 7d9ee62 to 6042fa5 Compare June 3, 2020 13:01
@mrnugget mrnugget requested a review from a team June 3, 2020 14:03
@mrnugget
Copy link
Contributor Author

mrnugget commented Jun 3, 2020

@LawnGnome @ryanslade Along with @eseliger's review, would be great if one of you could also take a look at this :)

Copy link
Contributor

@ryanslade ryanslade left a comment

Choose a reason for hiding this comment

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

LGTM

I'm a bit confused about the various changeset resolvers. Maybe it's worth adding some comments where they're defined?

We have ChangesetResolver and HiddenExternalChangesetResolver and ExternalChangesetResolver that all appear to contain the same interface:

type CommonInterface interface {
	ID() graphql.ID

	CreatedAt() DateTime
	UpdatedAt() DateTime
	NextSyncAt() *DateTime
	State() campaigns.ChangesetState
	Campaigns(ctx context.Context, args *ListCampaignArgs) (CampaignsConnectionResolver, error)

	ToExternalChangeset() (ExternalChangesetResolver, bool)
	ToHiddenExternalChangeset() (HiddenExternalChangesetResolver, bool)
}

I wonder if embedding that interface in all three would make that clearer?

@mrnugget
Copy link
Contributor Author

mrnugget commented Jun 3, 2020

I wonder if embedding that interface in all three would make that clearer?

Ah, I see what you mean. Yeah, I'll try that. Although I have to say that some part of the confusion seems to me to be "unsolvable" since we have to match GraphQL interfaces/types, but can't use the same "types" as in GraphQL (if GraphQL schema says "interface" we can't necessarily use a Go interface for that, etc.)

mrnugget referenced this pull request Jun 3, 2020
@mrnugget mrnugget merged commit 9aa073f into master Jun 3, 2020
@mrnugget mrnugget deleted the campaigns/repo-permissions branch June 3, 2020 16:21
@chrispine chrispine added the batch-changes Issues related to Batch Changes label Aug 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

batch-changes Issues related to Batch Changes RFC-157 Tickets related to RFC 157 - Campaigns authorization model

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants