Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prebuilds stop working for team projects when an owner without repo access joins the team #8128

Closed
jldec opened this issue Feb 9, 2022 · 5 comments · Fixed by #8132
Closed
Assignees
Labels
team: webapp Issue belongs to the WebApp team type: bug Something isn't working

Comments

@jldec
Copy link
Contributor

jldec commented Feb 9, 2022

This issue is a result of investigating #7648 and #8118.

Team-project prebuilds depend on the 1st (returned from DB) team-owner's permissions. If that user does not have the right access to the repo, then the prebuilds stop working. This can start happening if another team-owner is added later, after the project has been working for a while.

findProjectOwner() is called during handlePushEvent(), and the owner is then used to do fetchConfig() which will fail silently if the owner does not have permissions, causing the prebuild not to be triggered.

Examples of these fetchConfig errors can be found in traces and trace-detail. (internal)

: (await this.teamDB.findMembersByTeam(project.teamId || '')).filter(m => m.role === "owner")[0];

To reproduce

  • create a team, add a project with prebuilds, confirm that prebuilds are triggered
  • invite a new member to the team who does not have access to the project repo
  • set the role of the new member to owner
  • observe that prebuilds not being triggered as expected anymore
  • remove the new owner from the team
  • observe prebuilds resume
@jldec jldec added type: bug Something isn't working team: webapp Issue belongs to the WebApp team labels Feb 9, 2022
@jldec jldec moved this to Scheduled in 🍎 WebApp Team Feb 9, 2022
@jldec
Copy link
Contributor Author

jldec commented Feb 9, 2022

One suggestion for a fix from @konne is to separate the roles of admin and owner.

@jldec
Copy link
Contributor Author

jldec commented Feb 9, 2022

another suggestion from @jankeromnes

Thankfully, I think @AlexTugarev recently implemented a new behavior that I think we should adopt everywhere:

  • If the installer of the app is (still) a member of the team, use their token (best case)
  • If the team doesn't have the app's original installer anymore (left the team / deleted account / etc), fall back to any member that has a connected identity with the targeted Git host
  • (If this still doesn't work, we can't do better, and should probably tell the team the integration is in a bad state / needs to be re-installed)

@AlexTugarev AlexTugarev self-assigned this Feb 10, 2022
@konne
Copy link

konne commented Feb 10, 2022

@jankeromnes

The idea is great to have less friction and support for users because it will in most cases just automatically work.
The disadvantage is for me that if I look into enterprise readiness it has not a predicted usage of tokens and even sometimes in that case you want to really define which token is used for access. This is in most cases related to forbidden or limited usage tracking or fraud prevention.

The other point came into my mind. What happens if I only have one owner and this user gets blocked by Github (whatever the reason is) This lead to the situation that the team can not be managed anymore but you can also not create a new team and add the projects, because the projects are already added to that team.

@AlexTugarev
Copy link
Member

it has not a predicted usage of tokens

@konne, given that we'll fix the selection of a user account to fetch repo installation with, I would like to understand that point of yours. If a project belongs to a team, any team member's token with necessary permissions should be good to use for prebuilds. That should solve problems with rate limitation and improve reliability.

Repository owner moved this from In Progress to Done in 🍎 WebApp Team Feb 11, 2022
@konne
Copy link

konne commented Feb 11, 2022

@AlexTugarev for example in enterprise usage of GitLab I know companies that check the usage of API tokens also per IP.
If the system just selects a random one it can be that this generates a false positive alarm.
I don't see you current in general as a problem it will work in 99,999% of customers. I just wanted to highlight that sometime admins want to have better control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team: webapp Issue belongs to the WebApp team type: bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants