lgtm plugin should use trigger.TrustedUser

[nikhita]

The lgtm plugin currently does a IsCollaborator() check to determine if a user can lgtm a PR or not. Which means that even if only_org_members: true is defined in the trigger config, outside collaborators are able to lgtm PRs.

To fix this, we should use trigger.TrustedUser() to determine whether a user is trusted or not.

Additionally, if skip_collaborators: true is configured, we can then skip the trigger.TrustedUser() check.

/sig contributor-experience
/cc @cblecker @spiffxp @alvaroaleman @stevekuznetsov
related: #13002

Happy to work on this if this makes sense.

[nikhita]

This is mainly concerning because doing a /lgtm can trigger tests.

[nikhita]

related: #12785

[cblecker]

I just.. I really dislike the whole "trusted org" logic set. That the "trust" is somehow tied to not the org the actual repo is in. If that logic wasn't there, I wouldn't even have a second thought on this.

[cblecker]

ref:

test-infra/prow/plugins/trigger/trigger.go

Lines 153 to 185 in 5022463

	// TrustedUser returns true if user is trusted in repo.
	// Trusted users are either repo collaborators, org members or trusted org members.
	func TrustedUser(ghc trustedUserClient, onlyOrgMembers bool, trustedOrg, user, org, repo string) (bool, error) {
	// First check if user is a collaborator, assuming this is allowed
	if !onlyOrgMembers {
	if ok, err := ghc.IsCollaborator(org, repo, user); err != nil {
	return false, fmt.Errorf("error in IsCollaborator: %v", err)
	} else if ok {
	return true, nil
	}
	}
	// TODO(fejta): consider dropping support for org checks in the future.
	// Next see if the user is an org member
	if member, err := ghc.IsMember(org, user); err != nil {
	return false, fmt.Errorf("error in IsMember(%s): %v", org, err)
	} else if member {
	return true, nil
	}
	// Determine if there is a second org to check
	if trustedOrg == "" || trustedOrg == org {
	return false, nil // No trusted org and/or it is the same
	}
	// Check the second trusted org.
	member, err := ghc.IsMember(trustedOrg, user)
	if err != nil {
	return false, fmt.Errorf("error in IsMember(%s): %v", trustedOrg, err)
	}
	return member, nil
	}

[alvaroaleman]

I just.. I really dislike the whole "trusted org" logic set. That the "trust" is somehow tied to not the org the actual repo is in. If that logic wasn't there, I wouldn't even have a second thought on this.

Thats a fair point but IMHO an orthogonal issue. The issue we have here is that we have code to determine if a user is trusted and the LGTM plugin does something that requires trust (starting tests) without calling into that code but instead using a different check.

Independently of what TrustedUser actually does and if that always makes sense, IMHO everything that starts tests on a PullRequest should call into it to check if the user who triggered the event is trusted.

[spiffxp]

/area prow

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[cblecker]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[cblecker]

/remove-lifecycle rotten

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[alvaroaleman]

I think this is somethings we still need to fix. There is no good reason to have different codepaths to determine "trust" in different places.

Using/Not using the "trusted org" concept is completely orthogonal to this.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[alvaroaleman]

/reopen

[k8s-ci-robot]

@alvaroaleman: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[cblecker]

/remove-lifecycle rotten
/reopen

[k8s-ci-robot]

@cblecker: Reopened this issue.

In response to this:

/remove-lifecycle rotten
/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.