Branch protection: Possibility to not use whitelist but allow anyone with write access

[davidsvantesson]

fixes #8288. Related to #5731 (does not solve exactly what poster wants)

Gives three options for pushing to protected branch:

• disabled
• anyone with write access
• only whitelisted

For approvals a new checkbox is added to enable whitelist. Otherwise anyone with write access is considered as a valid approval.

Screenshot

[codecov-io]

Codecov Report

Merging #9055 into master will decrease coverage by 0.05%.
The diff coverage is 17.97%.

@@            Coverage Diff             @@
##           master    #9055      +/-   ##
==========================================
- Coverage   41.08%   41.03%   -0.06%     
==========================================
  Files         555      556       +1     
  Lines       72403    72535     +132     
==========================================
+ Hits        29750    29764      +14     
- Misses      38912    39028     +116     
- Partials     3741     3743       +2

Impacted Files	Coverage Δ
models/migrations/migrations.go	1.3% <ø> (ø)	⬆️
modules/auth/repo_form.go	43.47% <ø> (ø)	⬆️
models/migrations/v111.go	0% <0%> (ø)
services/pull/review.go	0% <0%> (ø)	⬆️
routers/private/hook.go	50.24% <0%> (ø)	⬆️
models/org_team.go	51.47% <100%> (+0.15%)	⬆️
models/pull.go	55.26% <100%> (+0.12%)	⬆️
models/review.go	46.15% <20.68%> (-7.08%)	⬇️
routers/repo/setting_protected_branch.go	40.29% <38.46%> (+0.6%)	⬆️
routers/repo/issue.go	36.36% <50%> (ø)	⬆️
... and 8 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 6460284...93ede5a. Read the comment docs.

[guillep2k]

Sorry if I didn't get this correctly; the original issue was kind of unclear, but what I understood is that the OP asked for something like...?:

• Allow forced pushs (-f)

This PR seems very useful nonetheless.

[davidsvantesson]

Reading the issue again (and also the comment that was in Chinese), I see I misunderstood what the original poster wanted. I though the problem was that you wanted to easily disable only force pushing for everyone but allowing pushing (without having to enter a whitelist), and maybe also that it shall be enforced for all branches (which can be solved by wildcard protected branches).
So yes, enabling force pushing for some users is not solved by this PR.

[lunny]

Please resolve conflicts.

[6543]

Migration number has changed +1

[guillep2k]

IMHO approvals should "remember" if the users were whitelisted at the moment they approved and this value should not be recalculated. Two reasons for this:

• This is not "list friendly"; it's very difficult to optimize as we want to have the PR list showing the number of approvals. If possible, we should resolve the list with a simple join/select.
• If rules change, history should not be rewritten. Non-counting approvals should remain non-counting and vice-versa.

[davidsvantesson]

Your argumentation makes sense, but that would require quite large code change (need new database columns). I think it should be a different PR.

[guillep2k]

I don't think it's that big of a change. I think you only need a new "Verified bool" column to the review table and move this check to the code that inserts the review. One day that "verified" column can reflect a 2FA, for example.

[davidsvantesson]

Can it be called OfficialReviewer (bool)?
You mean there will be an option to only accept 2FA users as reviewers?

[guillep2k]

Yes, it can be called OfficialReviewer, although that would lock the meaning. What I mean is that what today is a bool (verified/not verified) tomorrow can be an enum (not verified/verified/verified+signed, etc.). But whatever we may want in the future can be solved with a migration no problem, so feel free to choose whatever you feel that fits best. 👍

[guillep2k]

Try:

	var max_id int64
	if _, err = sess.Select("max(id) as `max_id`").Table("pull_request").Get(&max_id); err != nil {
		return err
	}

[davidsvantesson]

The current query seem to work, although your suggestion maybe looks a bit simpler.

[guillep2k]

This line I don't understand. If you have review.id in the row, all lines are already unique (unless there were multiple users with the same id which should never happen). A MAX(updated_unix) will be the same as updated_unix without the Group By. 🤔

What is it that you are trying to accomplish? Bring up the latest review? The query from the migration should achieve that.

[davidsvantesson]

The function shall return the latest review of each reviewer (not necessary official reviewers). I see that I actually don't use the change in this function, probably rewrote something. I think later on the information on which reviews are "official" should be shown on the pull request page.

The max(review.updated_unix) as review_updated_unix is used to sort the list and ensure the group by gets the latest review. However it might have been enough to get the max(id) instead, I don't see how reviews from same reviewer could change order.

[guillep2k]

The function shall return the latest review of each reviewer (not necessary official reviewers). I see that I actually don't use the change in this function, probably rewrote something. I think later on the information on which reviews are "official" should be shown on the pull request page.

The max(review.updated_unix) as review_updated_unix is used to sort the list and ensure the group by gets the latest review. However it might have been enough to get the max(id) instead, I don't see how reviews from same reviewer could change order.

This kind of query will not work. If you have two different review.type (e.g. comment + approval) from the same user, you'll get two rows because Group By will see them as different values. To get the latest record you need to do a select in select like the one used in the migration:

if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND type in (?, ?) GROUP BY issue_id, reviewer_id)",
			issueID, models.ReviewTypeApprove, models.ReviewTypeReject).
			Find(&reviews); err != nil {
			return err
		}

Of course the example above requires some adaptation to get the JOIN and the columns you're expecting.

[davidsvantesson]

It is actually solved further down in the function (I don't say it is a good solution 😄)

	// We need to group our results by the fields we want, otherwise some databases will fail (only_full_group_by).
	// But becaus we're doing this, we need to manually filter out multiple reviews of different types by the
	// same person because we only want to show the newest review grouped by user. Thats why we're using a map here.

[davidsvantesson]

Your query might work... although it doesn't work to just remove some of the group by fields for pgsql and mysql (due to some only_full_group_by setting).

[davidsvantesson]

So I didn't want to touch this function more than needed (and actually I can revert the changes I done). But the only thing I added was the Official field to the results, and hence added in SQL where necessary. The multiple rows is handled by the code later already.

I think maybe this function tries to do too much in SQL which makes it a bit complex.

[guillep2k]

To exemplify this, let me give you one example. Suppose you have this in the review table:

review ID	user_id	review_type	review_date
1	1	comment	Jan 1
2	2	request-changes	Jan 2
3	1	comment	Jan 3
4	3	comment	Jan 4
5	2	approval	Jan 5

If you do:

select max(review_id), user_id, review_type, max(review_date)
from review
group by user_id, review_type;

This is what you will get:

review ID	user_id	review_type	review_date
2	2	request-changes	Jan 2
3	1	comment	Jan 3
4	3	comment	Jan 4
5	2	approval	Jan 5

As you see, the two reviews from user #1 were consolidated in one row, as the fields user_id and review_type are the same for all records; user #3 is not a problem, but the two reviews from user #2 show up as different rows.

That's why the only way to get this SQL right is with a sub-query:

select max(id), user_id from review group by user_id;

Will give you:

review ID	user_id
3	1
4	3
5	2

So, these are the records that you're looking for:

select review_id, user_id, review_type, review_date
from `review`
where review_id in (
        select max(id), user_id from `review` as `review_inner` group by user_id
    )

returns:

review ID	user_id	review_type	date
3	1	comment	Jan 3
4	3	comment	Jan 4
5	2	approval	Jan 5

which is what you want. With this method you can perform joins too:

select review_id, user_id, review_type, review_date, `user`.*
from `review`
INNER JOIN `user` ON `user`.`id` = `review`.`user_id`
where review_id in (
        select max(id), user_id from `review` as `review_inner` group by user_id
    )

There's probably a nice way to write all of this with a couple of xorm directives (e.g. I think you can write the subquery in builder and use it in the In() operator, but I'm not sure), so I think it will be just quicker to do it with sess.SQL() like you did in migrations.

[davidsvantesson]

Very long informative response 😄.
I just wanted to point out that the existing code actually works, although not very efficient or elegant. I will see if I can rewrite the SQL query in a better way.

[guillep2k]

Oh, sorry! I didn't realize that the code wasn't yours. It's my tendency of checking the right column more thoroughly than the left one. 😋

If you just don't feel like doing that change just let me know, because that's the only thing I was waiting for to give my 👍.

[davidsvantesson]

I rewrote the code and think it is easier to understand now although there will be a few more sql queries, but normally there will not be that many reviews for a PR.

[davidsvantesson]

So finally I think this PR is ready....

[guillep2k]

Thanks for all the good work!