[server] Restrict snapshot access based on repository access #8306

jankeromnes · 2022-02-18T11:29:16Z

Description

Restrict snapshot access based on repository access.

Also refactors:

GuardedSnapshot.workspace (simplified)
WorkspaceLogAccessGuard → RepositoryResourceGuard
RepositoryService.canAccessHeadlessLogs → RepositoryProvider.hasReadAccess

Related Issue(s)

Fixes #8257

How to test

Create a snapshot for a private repo
Verify that you can open the snapshot
Verify that other members of the private repo can open the snapshot
Verify that non-members of the private repo can not open the snapshot

Also:

Create a snapshot for a public repo
Verify that anyone can open the snapshot (even if they haven't connected the repo's Git provider)

Release Notes

Restrict snapshot access based on repository access

Documentation

codecov · 2022-02-18T11:36:13Z

Codecov Report

Merging #8306 (0f83683) into main (1e5962d) will increase coverage by 2.75%.
The diff coverage is n/a.

❗ Current head 0f83683 differs from pull request most recent head 42f1a54. Consider uploading reports for the commit 42f1a54 to get more accurate results

@@            Coverage Diff            @@
##            main    #8306      +/-   ##
=========================================
+ Coverage   8.42%   11.17%   +2.75%     
=========================================
  Files         33       18      -15     
  Lines       2339      993    -1346     
=========================================
- Hits         197      111      -86     
+ Misses      2137      880    -1257     
+ Partials       5        2       -3

Flag	Coverage Δ
components-gitpod-cli-app	`11.17% <ø> (ø)`
components-installer-raw-app	`?`
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	`?`

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

Impacted Files	Coverage Δ
components/local-app/pkg/auth/pkce.go
...components/ws-manager/unpriviledged-rolebinding.go
components/installer/pkg/common/ca.go
...onents/installer/pkg/components/ws-manager/role.go
components/installer/pkg/common/display.go
...s/installer/pkg/components/ws-manager/configmap.go
components/local-app/pkg/auth/auth.go
...installer/pkg/components/ws-manager/rolebinding.go
.../installer/pkg/components/ws-manager/deployment.go
components/installer/pkg/common/storage.go
... and 5 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 1e5962d...42f1a54. Read the comment docs.

jankeromnes · 2022-02-18T11:52:13Z

If you try to open a snapshot of a private repository you don't have access to, it now fails with:

(We could also make this error screen nicer.)

components/server/src/github/github-repository-provider.ts

components/server/src/gitlab/gitlab-repository-provider.ts

JanKoehnlein · 2022-02-18T12:32:34Z

LGTM codewise, haven't tested

jankeromnes · 2022-02-18T13:07:55Z

Many thanks!

Holding because I'd like to see if I can detect the missing integration, and at least show a "Connect" button on the error screen in that case.

/hold

jldec · 2022-02-18T13:26:31Z

@jankeromnes I think the error screen needs a little love (per issue)

Showing the provider 403 error string is ok'ish, but because of the change relative to existing snapshot behavior, we also need to show a message like:

Snapshot URLs require read access to the underlying repository.
Please request access from the repository owner.

Ideally we would also include a link to the repo.

geropl

Code LGTM as well

I like the WorkspaceLogAccessGuard → RepositoryResourceGuard refactoring 👍

jankeromnes · 2022-02-18T16:29:20Z

Okay, checking for a connected provider, and throwing a proper UnauthorizedError when absent, seems to do the trick (see the new Authorize button):

However, after authorizing, starting my own snapshot for a public repo fails 🙄

I'm not sure whether that's a bug in my changes, or a bug somewhere else in Gitpod / core-dev.

Checking the logs, I see a few worrying messages:

{
    "message": "unable to parse URL from normalized contextURL: 'snapshot/bb410d69-12cf-45f2-be53-ee7c030cf335'",
    "error": `TypeError [ERR_INVALID_URL]: Invalid URL
        at new NodeError (node:internal/errors:371:5)
        at onParseError (node:internal/url:552:9)
        at new URL (node:internal/url:628:5)
        at Object.getNormalizedURL (/app/node_modules/@gitpod/gitpod-protocol/lib/context-url.js:38:20)
        at RepositoryResourceGuard.<anonymous> (/app/node_modules/@gitpod/server/dist/src/auth/resource-access.js:379:61)
        at Generator.next (<anonymous>)
        at /app/node_modules/@gitpod/server/dist/src/auth/resource-access.js:13:71
        at new Promise (<anonymous>)
        at __awaiter (/app/node_modules/@gitpod/server/dist/src/auth/resource-access.js:9:12)
        at RepositoryResourceGuard.canAccess (/app/node_modules/@gitpod/server/dist/src/auth/resource-access.js:369:16)
        at /app/node_modules/@gitpod/server/dist/src/auth/resource-access.js:48:64
        at Array.map (<anonymous>)
        at CompositeResourceAccessGuard.<anonymous> (/app/node_modules/@gitpod/server/dist/src/auth/resource-access.js:48:53)
        at Generator.next (<anonymous>)
        at /app/node_modules/@gitpod/server/dist/src/auth/resource-access.js:13:71
        at new Promise (<anonymous>)`
}
{
    "message": "Request createWorkspace unsuccessful: 460/\"unable to parse ContextURL: undefined\"",
    "payload": {
        "method": "createWorkspace",
        "args": [
            {
                "contextUrl": "snapshot/f012085f-5c9e-4f1b-8d44-c6cac2d5fa17",
                "mode": "select-if-running",
                "forceDefaultConfig": false
            }, { "_isCancelled":false }
        ]
    }
}

and:

{
    "message": "cannot watch imagebuild logs for workspaceId",
    "error": `Error: upstream ended with status code: 2
        at /app/node_modules/@gitpod/server/dist/src/workspace/headless-log-service.js:135:37
        at /app/node_modules/@gitpod/supervisor-api-grpcweb/lib/status_pb_service.js:259:9
        at Array.forEach (<anonymous>)
        at onEnd (/app/node_modules/@gitpod/supervisor-api-grpcweb/lib/status_pb_service.js:258:21)
        at /app/node_modules/@improbable-eng/grpc-web/dist/grpc-web-client.js:1:11490
        at Array.forEach (<anonymous>)
        at e.rawOnError (/app/node_modules/@improbable-eng/grpc-web/dist/grpc-web-client.js:1:11452)
        at e.onTransportEnd (/app/node_modules/@improbable-eng/grpc-web/dist/grpc-web-client.js:1:10318)
        at WebSocket.ws.onclose (/app/node_modules/@gitpod/server/dist/src/util/grpc-web-ws-transport.js:86:25)
        at WebSocket.onClose (/app/node_modules/ws/lib/event-target.js:136:16)
        at WebSocket.emit (node:events:390:28)
        at WebSocket.emit (node:domain:475:12)
        at WebSocket.emitClose (/app/node_modules/ws/lib/websocket.js:236:12)
        at Object.onceWrapper (node:events:509:28)
        at ClientRequest.emit (node:events:390:28)
        at ClientRequest.emit (node:domain:475:12)`
}
{
    "message": "Request watchWorkspaceImageBuildLogs unsuccessful: 640/\"cannot watch imagebuild logs for workspaceId\"",
     "payload": {
        "method": "watchWorkspaceImageBuildLogs",
        "args": [ "bronze-goat-sbkglq6tewo", {"_isCancelled":false} ]
    }
}

Questions:

How does the RepositoryResourceGuard ever come across a snapshot contextURL? (It's supposed to only get the contextURL from the original workspace from which the snapshot was taken)
Why does opening my snapshot try to watch image build logs? (This should never happen for a snapshot, since it already has a fully-built-and-resolved image, right?)

jankeromnes · 2022-02-18T16:58:22Z

Left to do:

Understand and maybe resolve the snapshot bugs mentioned above
Surface the message Snapshot URLs require read access to the underlying repository. Please request access from the repository owner. in the UI when snapshot access fails

jankeromnes · 2022-02-19T13:48:28Z

Hmm, I've added some debug logs, but could not reproduce the errors above. I guess this works as expected now. ✅

I've also added a more specific error message when you don't have repo access:

Integration connected, repo access

Opening the snapshot works ✅

Integration connected, no repo access

Integration not connected (can't verify access)

I feel like we could still make these error pages look nicer somehow, but this could be a follow-up issue. 💭

Ready for final approval. 🚢

components/server/src/bitbucket-server/bitbucket-server-repository-provider.ts

AlexTugarev · 2022-02-21T09:08:08Z

components/server/src/repohost/repository-provider.ts

@@ -14,4 +14,5 @@ export interface RepositoryProvider {
    getBranches(user: User, owner: string, repo: string): Promise<Branch[]>;
    getCommitInfo(user: User, owner: string, repo: string, ref: string): Promise<CommitInfo | undefined>;
    getUserRepos(user: User): Promise<string[]>;
+    hasReadAccess(user: User, owner: string, repo: string): Promise<boolean>;


Also refactor: - Simplify GuardedSnapshot.workspace - WorkspaceLogAccessGuard → RepositoryResourceGuard - RepositoryService.canAccessHeadlessLogs → RepositoryProvider.hasReadAccess

AlexTugarev

LGTM

jldec · 2022-02-21T10:13:26Z

This looks good now @jankeromnes - thanks for improving the error message.

jldec

merci bien

jankeromnes · 2022-02-21T10:16:22Z

Many thanks for the reviews! 🙏

Releasing the hold-down clamps, go for launch 🚀

/unhold

jankeromnes requested a review from a team February 18, 2022 11:29

roboquat added the release-note label Feb 18, 2022

jankeromnes marked this pull request as draft February 18, 2022 11:29

roboquat added do-not-merge/work-in-progress size/L labels Feb 18, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Feb 18, 2022

jankeromnes marked this pull request as ready for review February 18, 2022 11:51

roboquat removed the do-not-merge/work-in-progress label Feb 18, 2022

jankeromnes commented Feb 18, 2022

View reviewed changes

components/server/src/github/github-repository-provider.ts Show resolved Hide resolved

jankeromnes commented Feb 18, 2022

View reviewed changes

components/server/src/gitlab/gitlab-repository-provider.ts Show resolved Hide resolved

roboquat added the do-not-merge/hold label Feb 18, 2022

geropl previously approved these changes Feb 18, 2022

View reviewed changes

jankeromnes dismissed geropl’s stale review via 0e09d0a February 18, 2022 15:53

jankeromnes force-pushed the jx/snapshot-access branch from 1ec4d35 to 0e09d0a Compare February 18, 2022 15:53

jankeromnes force-pushed the jx/snapshot-access branch from 580c136 to 0f83683 Compare February 19, 2022 13:03

jankeromnes force-pushed the jx/snapshot-access branch 2 times, most recently from 2cf8ab5 to a989331 Compare February 20, 2022 14:23

AlexTugarev reviewed Feb 21, 2022

View reviewed changes

components/server/src/bitbucket-server/bitbucket-server-repository-provider.ts Outdated Show resolved Hide resolved

AlexTugarev reviewed Feb 21, 2022

View reviewed changes

[server] Restrict snapshot access based on repository access

42f1a54

Also refactor: - Simplify GuardedSnapshot.workspace - WorkspaceLogAccessGuard → RepositoryResourceGuard - RepositoryService.canAccessHeadlessLogs → RepositoryProvider.hasReadAccess

jankeromnes force-pushed the jx/snapshot-access branch from a989331 to 42f1a54 Compare February 21, 2022 09:45

AlexTugarev approved these changes Feb 21, 2022

View reviewed changes

jldec approved these changes Feb 21, 2022

View reviewed changes

roboquat removed the do-not-merge/hold label Feb 21, 2022

roboquat merged commit 2d44392 into main Feb 21, 2022

roboquat deleted the jx/snapshot-access branch February 21, 2022 10:17

gtsiolis mentioned this pull request Feb 24, 2022

Allow a project owner to disable shared workspaces #6328

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[server] Restrict snapshot access based on repository access #8306

[server] Restrict snapshot access based on repository access #8306

jankeromnes commented Feb 18, 2022 •

edited

Loading

codecov bot commented Feb 18, 2022 •

edited

Loading

jankeromnes commented Feb 18, 2022

JanKoehnlein commented Feb 18, 2022

jankeromnes commented Feb 18, 2022

jldec commented Feb 18, 2022 •

edited

Loading

geropl left a comment

jankeromnes commented Feb 18, 2022 •

edited

Loading

jankeromnes commented Feb 18, 2022 •

edited

Loading

jankeromnes commented Feb 19, 2022 •

edited

Loading

AlexTugarev Feb 21, 2022

AlexTugarev left a comment

jldec commented Feb 21, 2022

jldec left a comment

jankeromnes commented Feb 21, 2022 •

edited by werft-gitpod-dev-com bot

Loading

[server] Restrict snapshot access based on repository access #8306

[server] Restrict snapshot access based on repository access #8306

Conversation

jankeromnes commented Feb 18, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

codecov bot commented Feb 18, 2022 • edited Loading

Codecov Report

jankeromnes commented Feb 18, 2022

JanKoehnlein commented Feb 18, 2022

jankeromnes commented Feb 18, 2022

jldec commented Feb 18, 2022 • edited Loading

geropl left a comment

Choose a reason for hiding this comment

jankeromnes commented Feb 18, 2022 • edited Loading

jankeromnes commented Feb 18, 2022 • edited Loading

jankeromnes commented Feb 19, 2022 • edited Loading

AlexTugarev Feb 21, 2022

Choose a reason for hiding this comment

AlexTugarev left a comment

Choose a reason for hiding this comment

jldec commented Feb 21, 2022

jldec left a comment

Choose a reason for hiding this comment

jankeromnes commented Feb 21, 2022 • edited by werft-gitpod-dev-com bot Loading

jankeromnes commented Feb 18, 2022 •

edited

Loading

codecov bot commented Feb 18, 2022 •

edited

Loading

jldec commented Feb 18, 2022 •

edited

Loading

jankeromnes commented Feb 18, 2022 •

edited

Loading

jankeromnes commented Feb 18, 2022 •

edited

Loading

jankeromnes commented Feb 19, 2022 •

edited

Loading

jankeromnes commented Feb 21, 2022 •

edited by werft-gitpod-dev-com bot

Loading