Add require signed commit for protected branch #9708

zeripath · 2020-01-11T18:01:59Z

Adds new require signed commit option to protect branch.
Adds indicators for CRUD and Merge screens to show if commits will be signed
Prevents CRUD and merge if these would affect a protected branch with require signed commit.
API merge and CRUD actions are handled
Fix bug in delete file API action

Fix #4249

Screenshots

New setting on protected branch

Merge Signed

Prevent Merge Unsigned

CRUD signed

Prevent CRUD unsigned

zeripath · 2020-01-11T18:03:28Z

It's annoying but you can't just use gogitRepository to get these as it appears that the quarantine incoming checks don't work correctly.

routers/private/hook.go

6543 · 2020-01-11T18:52:42Z

also close #8584

codecov-io · 2020-01-11T20:27:43Z

Codecov Report

Merging #9708 into master will decrease coverage by <.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #9708      +/-   ##
==========================================
- Coverage   42.21%   42.21%   -0.01%     
==========================================
  Files         602      602              
  Lines       78669    78669              
==========================================
- Hits        33213    33209       -4     
- Misses      41392    41395       +3     
- Partials     4064     4065       +1

Impacted Files	Coverage Δ
models/repo.go	`46.74% <ø> (ø)`	⬆️
services/repository/transfer.go	`66.66% <66.66%> (ø)`	⬆️
services/pull/check.go	`53.37% <0%> (-2.03%)`	⬇️
models/error.go	`30.76% <0%> (-0.55%)`	⬇️
modules/log/event.go	`65.64% <0%> (+1.02%)`	⬆️

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 2d454cd...788ed2f. Read the comment docs.

…ath/gitea into protect-branch-signed-commits-only

options/locale/locale_en-US.ini

routers/api/v1/repo/pull.go

modules/git/commit_reader.go

routers/private/hook.go

services/pull/merge.go

Co-Authored-By: guillep2k <[email protected]>

guillep2k

Only a concern about very large commits. In a future PR we should make it so the signature is checked as the payload is being loaded (and discarded) in order to require less memory.

modules/git/commit_reader.go

zeripath · 2020-01-14T20:23:21Z

~~The issue will be verification~~

Nope it's that I failed to get the payload regeneration correct in the last few changes.

6543

not working

EDIT: looks like 7a04a47 break something (worked before - with this commit it dont)

sapk · 2020-01-15T08:35:24Z

@zeripath remember to claim the bounty.

davidsvantesson · 2020-01-15T16:47:41Z

templates/repo/issue/view_content/pull.tmpl

+							<i class="icon lock green"></i>
+							{{$.i18n.Tr "repo.signing.will_sign" .SigningKey}}
+						</div>
+					{{else}}


I don't think this else case should be here. It should only be shown if RequireSigned and is already handled above.

Users should know if the commit will be signed or not.

Yes but the green text + icon if it will be signed would be enough? Many teams/companies don't use signing at all, so if you don't require signed commits it would be a bit annoying to have a yellow text "Commits are never signed" which many users will not even know what it means.

GitHub allow admin to override also signing. If signed commits are not required, GitHub doesn't show anything about signing.

So on the editor screen I was able to add a simple unlock icon to precede the Create Commit which could be mouseovered but there's not really a place on the merge screen to do that hence I felt that just adding it as another status box on merge is reasonable.

You need to know if your commits are going to be signed and why - on GH it's impossible to know why and if a commit is going to be signed.

In reality it's a status box that is only visible if you can merge.

davidsvantesson · 2020-01-15T16:49:18Z

templates/repo/issue/view_content/pull.tmpl

+				{{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .RequireSigned (not .WillSign)) (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}}
+				{{if and (or $.IsRepoAdmin (not $notAllOk)) (or (not .RequireSigned) .WillSign)}}


Shall repo admin be able to bypass "require signed" or not? Currently the term in $notAllOk has no effect.

It's a question I put on the original issue. I would guess we need an additional suboption to allow the repo admin to bypass this. The trouble is signing is different from pr approval or whitelisting.

If you really want every commit to be signed I think it doesn't make sense for admin to override it?
So the condition to $notAllOk are more there for future, in case we allow admin to merge?

I haven't changed $notAllOK - those are the other checks that were there previously. As I've written it RequireSign overrides all other checks as it's not overridable.

Perhaps I should have changed the name of notAllOK to notAllOverridableStatusChecksOK but it felt a little over the top.

But you have added (and .RequireSigned (not .WillSign)), is that not intentional?

yeah that was future proofing.

It also meant that the UI would keep sane.

The logic would need to be revised anyhow if allowing overriding signing. Otherwise you will get this:

yeah exactly.

Add require signed commit for protected branch

ba15ed9

zeripath added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Jan 11, 2020

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 11, 2020

Fix fmt

f482e28

6543 reviewed Jan 11, 2020

View reviewed changes

routers/private/hook.go Outdated Show resolved Hide resolved

6543 reviewed Jan 11, 2020

View reviewed changes

routers/private/hook.go Outdated Show resolved Hide resolved

Merge branch 'master' into protect-branch-signed-commits-only

43c5df5

zeripath added 4 commits January 11, 2020 20:43

Make editor show if they will be signed

f9f8557

Merge branch 'protect-branch-signed-commits-only' of github.com:zerip…

27c0354

…ath/gitea into protect-branch-signed-commits-only

bugfix

f2a28e8

Merge branch 'master' into protect-branch-signed-commits-only

9a7ec7f

lunny mentioned this pull request Jan 12, 2020

WIP: Add require signed commits feature #8584

Closed

zeripath added 6 commits January 12, 2020 05:38

Merge branch 'master' into protect-branch-signed-commits-only

e4d987a

Add basic merge check and better information for CRUD

594126a

linting comment

dd05f6a

Add descriptors to merge signing

de67865

Slight refactor

97b811d

Merge branch 'master' into protect-branch-signed-commits-only

ea21e79

zeripath mentioned this pull request Jan 12, 2020

Allow signed commits to be required #4249

Closed

zeripath added 3 commits January 12, 2020 20:17

Slight improvement to appearances

68a8e11

Handle Merge API

acf011a

manage CRUD API

24eb4a2

zeripath changed the title ~~WIP: Add require signed commit for protected branch~~ Add require signed commit for protected branch Jan 12, 2020

zeripath removed the pr/wip This PR is not ready for review label Jan 12, 2020

lafriks added this to the 1.12.0 milestone Jan 12, 2020

lafriks approved these changes Jan 12, 2020

View reviewed changes

Merge branch 'master' into protect-branch-signed-commits-only

6793e8d

guillep2k reviewed Jan 14, 2020

View reviewed changes

zeripath and others added 3 commits January 14, 2020 09:30

need to tolerate \r\n in message

7a04a47

check protected branch before trying to load it

161d110

Apply suggestions from code review

335ec50

Co-Authored-By: guillep2k <[email protected]>

guillep2k approved these changes Jan 14, 2020

View reviewed changes

modules/git/commit_reader.go Show resolved Hide resolved

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 14, 2020

Merge branch 'master' into protect-branch-signed-commits-only

5ad5a56

This comment has been minimized.

Sign in to view

6543 requested changes Jan 14, 2020

View reviewed changes

fix commit-reader

ea40bd6

6543 approved these changes Jan 14, 2020

View reviewed changes

zeripath added 3 commits January 14, 2020 22:12

Merge branch 'master' into protect-branch-signed-commits-only

d554a62

Merge branch 'master' into protect-branch-signed-commits-only

2d454cd

Merge branch 'master' into protect-branch-signed-commits-only

788ed2f

sapk merged commit 66ee9b8 into go-gitea:master Jan 15, 2020

zeripath deleted the protect-branch-signed-commits-only branch January 15, 2020 10:59

davidsvantesson reviewed Jan 15, 2020

View reviewed changes

davidsvantesson mentioned this pull request Jan 15, 2020

Fix admin handling at merge of PR #9749

Merged

This was referenced Jan 16, 2020

[Docs] Update Feature 'Reject unsigned commits' #9793

Merged

Committer verification #2770

Closed

davidsvantesson mentioned this pull request Jan 17, 2020

Change won't sign information text to black. #9843

Merged

alexanderadam mentioned this pull request Jun 24, 2020

Default branch settings for an organisation #12040

Open

jolheiser mentioned this pull request Jul 10, 2020

Block non-signed commits per-repo or per-instance #12204

Closed

go-gitea locked and limited conversation to collaborators Nov 24, 2020

delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add require signed commit for protected branch #9708

Add require signed commit for protected branch #9708

zeripath commented Jan 11, 2020 •

edited

Loading

zeripath commented Jan 11, 2020

6543 commented Jan 11, 2020

codecov-io commented Jan 11, 2020 •

edited

Loading

guillep2k left a comment

This comment has been minimized.

zeripath commented Jan 14, 2020 •

edited

Loading

6543 left a comment •

edited

Loading

sapk commented Jan 15, 2020

davidsvantesson Jan 15, 2020

zeripath Jan 15, 2020

davidsvantesson Jan 15, 2020

davidsvantesson Jan 15, 2020

zeripath Jan 15, 2020

davidsvantesson Jan 15, 2020

zeripath Jan 15, 2020

davidsvantesson Jan 15, 2020

zeripath Jan 15, 2020

davidsvantesson Jan 15, 2020

zeripath Jan 15, 2020

zeripath Jan 15, 2020

davidsvantesson Jan 15, 2020

zeripath Jan 15, 2020

		{{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .RequireSigned (not .WillSign)) (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}}
		{{if and (or $.IsRepoAdmin (not $notAllOk)) (or (not .RequireSigned) .WillSign)}}

Add require signed commit for protected branch #9708

Add require signed commit for protected branch #9708

Conversation

zeripath commented Jan 11, 2020 • edited Loading

Screenshots

New setting on protected branch

Merge Signed

Prevent Merge Unsigned

CRUD signed

Prevent CRUD unsigned

zeripath commented Jan 11, 2020

6543 commented Jan 11, 2020

codecov-io commented Jan 11, 2020 • edited Loading

Codecov Report

guillep2k left a comment

Choose a reason for hiding this comment

This comment has been minimized.

zeripath commented Jan 14, 2020 • edited Loading

6543 left a comment • edited Loading

Choose a reason for hiding this comment

sapk commented Jan 15, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeripath commented Jan 11, 2020 •

edited

Loading

codecov-io commented Jan 11, 2020 •

edited

Loading

zeripath commented Jan 14, 2020 •

edited

Loading

6543 left a comment •

edited

Loading