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

Whitelist hooks #302

Closed
wants to merge 9 commits into from
Closed

Whitelist hooks #302

wants to merge 9 commits into from

Conversation

arbakker
Copy link

Implemented cli option to for hooks whitelisting, based on this comment. I named the cli option hooks-enabled-events so it is consistent with the other hooks options. I added the following docs to the docs/hooks.md in the pull request:


## Enabling/Disabling Hook Events

The --hooks-enabled-events option for the tusd binary works as a whitelist for hook events and takes a comma separated list of hook events. By default all hook events are enabled.

Possible values for the --hooks-enabled-events option:

  • * - enable all hook events (default value)
  • "" - disable all hook events
  • comma separated list of hook events (for instance: pre-create,post-create) - whitelist of hook events

Feedback on the code quality is more than welcome, I am still quite new to Go.

@@ -110,6 +110,8 @@ Usage of tusd:
Use Google Cloud Storage with this bucket as storage backend (requires the GCS_SERVICE_ACCOUNT_FILE environment variable to be set)
-hooks-dir string
Directory to search for available hooks scripts
-hooks-enabled-events string
Comma separated list of enabled hook events, set to "-" to disable all (default "*")
Copy link
Member

Choose a reason for hiding this comment

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

set to "-" to disable all

Is that correct? Isn't "" used to disable all hooks?

Copy link
Author

Choose a reason for hiding this comment

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

Correct, I used - in a previous version and forgot to update it in the docs, I fixed that in this commit - c93e37f which I added to the pull request later.

EnabledHooks = append(EnabledHooks, h)
}
} else {
slc := strings.Split(Flags.EnabledHooks, ",")
Copy link
Member

@Acconut Acconut Aug 16, 2019

Choose a reason for hiding this comment

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

It would be nice to have a check and corresponding output message if the specified hook name actually exists, so users get notified early when they misspell post-receive as post-recieve, for example.

@Acconut
Copy link
Member

Acconut commented Aug 16, 2019

Thank you very much for your work here, it's looking good!

I am wondering if we should actually allow to disable hook events. If the user does not want hooks at all, they should not specify the -hooks-path or -hooks-http option, IMO. What do you think about this?

@arbakker
Copy link
Author

Ha that did not occur to me, makes sense to not allow to disable all the hook events considering the hook-path and hooks-http options. I will change the behavior of the hooks-enabled-events option to only accept a comma separated list of events. If the hooks-enabled-events option is missing all hook events are enabled.

@arbakker
Copy link
Author

Implemented both your suggestions:

  • commit: removed capability to disable all events from hooks-enabled-events
  • commit: validate event strings in hooks-enabled-events option and show warning

Acconut added a commit that referenced this pull request Aug 26, 2019
@Acconut
Copy link
Member

Acconut commented Aug 26, 2019

Thank you very much! I cleaned up some code and merged this PR in f1fe5e2.

@Acconut Acconut closed this Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants