Fix Bitbucket push event handling #7833

AlexTugarev · 2022-01-26T08:15:37Z

This PR also fixes the linking of prebuilds to the related project.

Fix Bitbucket push event handling

roboquat · 2022-01-26T08:15:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from alextugarev after the PR has been reviewed.

Associated issue: #7683

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

components/server/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2022-01-26T08:25:15Z

Codecov Report

Merging #7833 (50e3740) into main (01dc940) will decrease coverage by 1.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #7833      +/-   ##
==========================================
- Coverage   12.01%   10.86%   -1.16%     
==========================================
  Files          20       18       -2     
  Lines        1190     1022     -168     
==========================================
- Hits          143      111      -32     
+ Misses       1043      909     -134     
+ Partials        4        2       -2

Flag	Coverage Δ
components-gitpod-cli-app	`10.86% <ø> (ø)`
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/local-app/pkg/auth/auth.go

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 01dc940...50e3740. Read the comment docs.

jankeromnes · 2022-01-28T10:01:33Z

Taking a look now 👀

jankeromnes

Many thanks for adding the project association for Bitbucket push events! 👍

This change looks good to me in principle, except for the part about impersonating a random team owner (please see inline).

I've also tested that triggering Prebuilds by pushing commits to Bitbucket still works 👍 (both for user projects and team projects).

jankeromnes · 2022-01-28T09:47:00Z

components/server/ee/src/prebuilds/bitbucket-app.ts

+        if (project) {
+            const owner = !!project.userId
+                ? { userId: project.userId }
+                : (await this.teamDB.findMembersByTeam(project.teamId || '')).filter(m => m.role === "owner")[0];


Why is it useful to pick a random owner from the team here, as opposed to just picking the user that started the prebuild as before?

It still seems weird and potentially dangerous to me that I can potentially perform actions as someone else (impersonation), as opposed to always running Gitpod actions as myself.

It still seems weird and potentially dangerous to me that I can potentially perform actions as someone else (impersonation), as opposed to always running Gitpod actions as myself.

First of, here we're are handling projects of a team in that case and this is about the ownership of a prebuild – nothing else. Then also, tagging with any of the current team owners seems to better than potentially tagging with a non-existing userId, which led to this pattern in the first place. BTW it'd be just consistent to do it like this, or change it also for GitHub and GitLab projects/prebuilds.

Why is it useful to pick a random owner from the team here, as opposed to just picking the user that started the prebuild as before?

Another explainer: have a user A create a project in a team and then let user A leave the team, which account to use to tag the prebuild?

@jankeromnes, can we discuss this in a call?

Hm. I tend to agree with Jan that we should not try to do sth intimate (use token) that is unexpected.
How does the BitBucket "installation flow" look like? E.g., can we store a userId there that we can use here? That way we'd at least maintain the expectation that who registered a repo will also execute the prebuilds (like for GitHub apps).

Another explainer: have a user A create a project in a team and then let user A leave the team, which account to use to tag the prebuild?

This sounds like a very interesting question to me: What is projectId.userId useful for, anyway? 🤔

Suggestion: For the sake of this PR, let's separate the "who owns the Team" and "who owns the BitBucket 'installation'" discussions into (a) separate issue(s). And simply exit here if project.userId is not set. Would that work, @AlexTugarev ?

The point is that we're storing the users token in the webhook registration, which is part of history when we had no project introduced yet. I believe it would be more robust to add the project id in the webhook registration and use any available token of any team owner (with access to that BB repo) when doing updates to the webook registration. This is something we concluded while discussing about the ownership of a project within a team, and what should happen when one leaves a team, e.g. should the project be removed?

I'm also ok with trying to maintain the installer's link as far as possible and fallback to a team owner after the installer was removed from the team.

@jankeromnes and @geropl, I changed to prefer the previous webhook installer, which should meet the expectation in most cases. I suggest to overhaul the semantics for all providers in a separate issues, because it seems that we did not cover all cases. That's something we can and should cover in unit tests for clear communications.

Sounds good!

@jankeromnes and @geropl, thanks once again for pushing this! after re-re-reading the flow and testing once more, I realized that's a bad decision to limit to project owners 🙄. the fallback mechanism should, if applied, try to find and team member to rescue the state of the project, if the original installer was removed from the team. so, priority is to maintain the webhook membership and then to fallback to any of the team member with the relevant provider connection.

I took the change and aligned the gitlab app code as well, because that was the blueprint here.

let's continue the clean up of this in the effort to provide a state of the webhook installation.

components/server/ee/src/prebuilds/bitbucket-app.ts

geropl · 2022-02-08T09:10:18Z

/hold
/lgtm

The code LGTM except that I think we should add a comment with the gist of our discussion to findProjectOwner.

Please ping for new /lgtm then!

geropl · 2022-02-08T12:35:45Z

components/server/ee/src/prebuilds/bitbucket-app.ts

+    /**
+     * Finds the relevant user account and project to the provided webhook event information.
+     *
+     * First of all it tries to find the project for the given `cloneURL`, then it tries to


components/server/ee/src/prebuilds/gitlab-app.ts

geropl

Code looks good! 👍

Thx for pushing this till the end!

geropl · 2022-02-08T12:49:06Z

/unhold

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed labels Jan 26, 2022

roboquat added the size/M label Jan 26, 2022

roboquat added release-note and removed do-not-merge/release-note-label-needed labels Jan 26, 2022

AlexTugarev force-pushed the at/bitbucket-7683 branch from 29ff4f4 to 4f67c14 Compare January 26, 2022 09:06

AlexTugarev requested review from jankeromnes and geropl January 26, 2022 10:04

AlexTugarev marked this pull request as ready for review January 26, 2022 10:04

roboquat removed the do-not-merge/work-in-progress label Jan 26, 2022

gtsiolis added the team: webapp Issue belongs to the WebApp team label Jan 26, 2022

jankeromnes reviewed Jan 28, 2022

View reviewed changes

AlexTugarev mentioned this pull request Jan 31, 2022

[server] Add project and branch to bitbucket prebuilds #7745

Closed

geropl reviewed Feb 3, 2022

View reviewed changes

components/server/ee/src/prebuilds/bitbucket-app.ts Show resolved Hide resolved

[bitbucket] ignore branch deletion events

06c7447

AlexTugarev force-pushed the at/bitbucket-7683 branch from 4f67c14 to b294dd2 Compare February 7, 2022 11:08

AlexTugarev requested review from a team, jankeromnes and geropl February 7, 2022 11:08

geropl reviewed Feb 8, 2022

View reviewed changes

components/server/ee/src/prebuilds/bitbucket-app.ts Outdated Show resolved Hide resolved

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

[bitbucket] fix linking projects to prebuilds on push event

b75971e

AlexTugarev force-pushed the at/bitbucket-7683 branch from b294dd2 to a4f5cbc Compare February 8, 2022 12:20

roboquat added size/L and removed size/M labels Feb 8, 2022

geropl reviewed Feb 8, 2022

View reviewed changes

components/server/ee/src/prebuilds/gitlab-app.ts Outdated Show resolved Hide resolved

[gitlab] align findProjectAndOwner

50e3740

AlexTugarev force-pushed the at/bitbucket-7683 branch from a4f5cbc to 50e3740 Compare February 8, 2022 12:39

geropl approved these changes Feb 8, 2022

View reviewed changes

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

roboquat merged commit bbf7036 into main Feb 8, 2022

roboquat deleted the at/bitbucket-7683 branch February 8, 2022 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Bitbucket push event handling #7833

Fix Bitbucket push event handling #7833

AlexTugarev commented Jan 26, 2022 •

edited

Loading

roboquat commented Jan 26, 2022

codecov bot commented Jan 26, 2022 •

edited

Loading

jankeromnes commented Jan 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes left a comment

jankeromnes Jan 28, 2022

AlexTugarev Jan 31, 2022

AlexTugarev Jan 31, 2022

AlexTugarev Feb 3, 2022

geropl Feb 3, 2022

geropl Feb 3, 2022

AlexTugarev Feb 7, 2022

AlexTugarev Feb 7, 2022

geropl Feb 8, 2022

AlexTugarev Feb 8, 2022

geropl commented Feb 8, 2022

geropl Feb 8, 2022

geropl left a comment •

edited

Loading

geropl commented Feb 8, 2022

Fix Bitbucket push event handling #7833

Fix Bitbucket push event handling #7833

Conversation

AlexTugarev commented Jan 26, 2022 • edited Loading

roboquat commented Jan 26, 2022

codecov bot commented Jan 26, 2022 • edited Loading

Codecov Report

jankeromnes commented Jan 28, 2022 • edited by werft-gitpod-dev-com bot Loading

jankeromnes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geropl commented Feb 8, 2022

Choose a reason for hiding this comment

geropl left a comment • edited Loading

Choose a reason for hiding this comment

geropl commented Feb 8, 2022

AlexTugarev commented Jan 26, 2022 •

edited

Loading

codecov bot commented Jan 26, 2022 •

edited

Loading

jankeromnes commented Jan 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading

geropl left a comment •

edited

Loading