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

Disable custom Git Hooks globally via configuration file #2450

Merged
merged 2 commits into from
Sep 12, 2017
Merged

Disable custom Git Hooks globally via configuration file #2450

merged 2 commits into from
Sep 12, 2017

Conversation

techknowlogick
Copy link
Member

Fixes #2449 by allowing the disabling of custom git hooks via configuration file

@@ -817,6 +818,7 @@ func NewContext() {
ReverseProxyAuthUser = sec.Key("REVERSE_PROXY_AUTHENTICATION_USER").MustString("X-WEBAUTH-USER")
MinPasswordLength = sec.Key("MIN_PASSWORD_LENGTH").MustInt(6)
ImportLocalPaths = sec.Key("IMPORT_LOCAL_PATHS").MustBool(false)
DisableGitHooks = sec.Key("DISABLE_GIT_HOOKS").MustBool(false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this also to the app.ini file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JonasFranzDEV Thanks for this feedback. I've just pushed a new commit with the option in app.ini

@lunny lunny added this to the 1.x.x milestone Sep 2, 2017
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Sep 2, 2017
@@ -237,7 +237,7 @@ func (u *User) CanCreateOrganization() bool {

// CanEditGitHook returns true if user can edit Git hooks.
func (u *User) CanEditGitHook() bool {
return u.IsAdmin || u.AllowGitHook
return !setting.DisableGitHooks && (u.IsAdmin || u.AllowGitHook)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this feedback, especially linking me to where I could look. Investigating this (by calling the API to see what the response would be), I found that it only returns the standard gitea/slack/discord/etc.. hooks, and couldn't find it returning the githooks.

@sapk
Copy link
Member

sapk commented Sep 5, 2017

LGTM

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Sep 5, 2017
Copy link
Member

@ethantkoenig ethantkoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit

conf/app.ini Outdated
@@ -206,6 +206,8 @@ REVERSE_PROXY_AUTHENTICATION_USER = X-WEBAUTH-USER
MIN_PASSWORD_LENGTH = 6
; True when users are allowed to import local server paths
IMPORT_LOCAL_PATHS = false
; Disable all (including admin) users to create custom git-hooks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest Prevent all users (including admin) from creating custom git hooks

@techknowlogick
Copy link
Member Author

Thanks @ethantkoenig. I've update the comment in app.ini to align with what you suggested. It is now much more clear.

Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

@@ -86,7 +86,7 @@
<div class="inline field">
<div class="ui checkbox">
<label><strong>{{.i18n.Tr "admin.users.allow_git_hook"}}</strong></label>
<input name="allow_git_hook" type="checkbox" {{if .User.CanEditGitHook}}checked{{end}}>
<input name="allow_git_hook" type="checkbox" {{if .User.CanEditGitHook}}checked{{end}} {{if DisableGitHooks}}disabled{{end}}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since User.CanEditGitHook already checks this, is this change needed?

https://github.com/go-gitea/gitea/pull/2450/files#diff-46259196476f860fea33754fcb22e9eeR240

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @bkcsoft. This change is needed so that an admin doesn't get confused. If the global setting is set to disable githooks, then the template change will prevent the admin from checking the box in the admin dashboard and wondering why the change doesn't take effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah right, missed the "disabled" part :)

@ethantkoenig
Copy link
Member

LGTM

@tboerger tboerger 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 Sep 11, 2017
@bkcsoft
Copy link
Member

bkcsoft commented Sep 11, 2017

@techknowlogick try rebasing on master and see if the build-failures still persist

@techknowlogick
Copy link
Member Author

@bkcsoft I've just rebased against master, and the build failed. However, I think that could just be the upgrade for Drone, as I looked at a couple other builds from others and they failed in the same way.

@techknowlogick
Copy link
Member Author

@bkcsoft I've just rebased again with the most recent changes and drone is now reporting a success.

@lafriks
Copy link
Member

lafriks commented Sep 12, 2017

@techknowlogick please rabase again

@techknowlogick
Copy link
Member Author

@lafriks I've just rebased. The build was cancelled. Should I push again so Drone tries to re-run, or was it cancelled for a reason?

@lafriks
Copy link
Member

lafriks commented Sep 12, 2017

@techknowlogick yes, do force push

@techknowlogick
Copy link
Member Author

@lafriks I had force-pushed after this most recent rebase, however as now it is the same as my local I can't force push again. Is there another way to trigger drone?

@lafriks
Copy link
Member

lafriks commented Sep 12, 2017

@techknowlogick you should still be able to do git push -f origin

@techknowlogick
Copy link
Member Author

@lafriks nevermind. I was able to force push again.

@lafriks
Copy link
Member

lafriks commented Sep 12, 2017

@techknowlogick I'm sorry but you will have to rebase again as other PR build was first :)

Signed-off-by: Matti Ranta <matti@mdranta.net>
Signed-off-by: Matti Ranta <matti@mdranta.net>
@lafriks lafriks modified the milestones: 1.3.0, 1.x.x Sep 12, 2017
@lafriks
Copy link
Member

lafriks commented Sep 12, 2017

Make LG-TM work

@lafriks lafriks merged commit 9bdbfbf into go-gitea:master Sep 12, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable custom Git Hooks globally via configuration value
8 participants