Add Webhook authorization header

[oliverpool]

This is a different approach to #20267, I took the liberty of adapting some parts, see below

Context

In some cases, a weebhook endpoint requires some kind of authentication. The usual way is by sending a static Authorization header, with a given token. For instance:

• Matrix expects a Bearer <token> (already implemented, by storing the header cleartext in the metadata - which is buggy on retry #19872)
• TeamCity #18667
• Gitea instances #20267
• SourceHut https://man.sr.ht/graphql.md#authentication-strategies (this is my actual personal need :)

Proposed solution

Add a dedicated encrypt column to the webhook table (instead of storing it as meta as proposed in #20267), so that it gets available for all present and future hook types (especially the custom ones #19307).

This would also solve the buggy matrix retry #19872.

As a first step, I would recommend focusing on the backend logic and improve the frontend at a later stage. For now the UI is a simple Authorization field (which could be later customized with Bearer and Basic switches):

The header name is hard-coded, since I couldn't fine any usecase justifying otherwise.

Questions

• What do you think of this approach? @justusbunsi @Gusted @silverwind
• ~~How are the migrations generated? Do I have to manually create a new file, or is there a command for that?~~
• ~~I started adding it to the API: should I complete it or should I drop it? (I don't know how much the API is actually used)~~

Done as well:

• add a migration for the existing matrix webhooks and remove the Authorization logic there

Closes #19872

[oliverpool]

I updated the settings template, to give some hint regarding the access_token (and make it mandatory):

[codecov-commenter]

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@085f717). Click here to learn what that means. The diff coverage is 40.54%.

@@           Coverage Diff           @@
##             main   #20926   +/-   ##
=======================================
  Coverage        ?   48.17%           
=======================================
  Files           ?     1032           
  Lines           ?   140365           
  Branches        ?        0           
=======================================
  Hits            ?    67625           
  Misses          ?    64629           
  Partials        ?     8111           

Impacted Files	Coverage Δ
modules/structs/hook.go	4.34% <ø> (ø)
routers/api/v1/org/hook.go	0.00% <0.00%> (ø)
routers/api/v1/repo/hook.go	30.38% <0.00%> (ø)
routers/web/repo/webhook.go	2.09% <0.00%> (ø)
services/forms/repo_form.go	39.50% <ø> (ø)
routers/api/v1/utils/hook.go	22.31% <14.63%> (ø)
services/webhook/deliver.go	56.39% <33.33%> (ø)
models/webhook/webhook.go	67.42% <56.25%> (ø)
modules/convert/convert.go	76.14% <86.66%> (ø)
services/webhook/matrix.go	73.38% <93.10%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[oliverpool]

I have added a migration to ensure that the hook_task doesn't have any remaining access_token in its payload.

[oliverpool]

@Gusted do you have any more comment? Is there anything I can do to move this PR forward?

Thanks

[Gusted]

@Gusted do you have any more comment?

Generally looks good now, I've added the other two reviewers from the other PR(#20267) to this PR.

[oliverpool]

@silverwind thanks for the review, I implemented your suggestions.

I haved squashed all my commits and rebased on the latest commit of main.

[oliverpool]

@lunny let me know if I can do anything to push this PR forward :-)

[oliverpool]

The drone failure looks like a glitch amd64 > test-mssql > TestAPIRepoMigrate

Migration failed: Clone: exit status 128 - fatal: unable to access 'https://github.com/go-gitea/test_repo.git/': gnutls_handshake() failed: Error in the pull function.

Edit: I squashed and rebased all my commits to force a new pipeline run.

[wxiaoguang]

Ping @justusbunsi @Gusted @silverwind, especially for the WIP: Add authorization header support for Gitea webhooks #20267 , which one wins? (I haven't read these two)

[justusbunsi]

I haven't reviewed or tested this. Apologies, especially because I was mentioned from the beginning. As long as this PR includes the functionality of #20267 I'd suggest to prefer this one here. It seem to tackle some issues that mine wouldn't.

[oliverpool]

@delvh is there anything I should fix before fixing the conflicts ?

[delvh]

Erm… Is it now required or optional?

[oliverpool]

If it is a matrix hook, it is required (as it was before this PR) - albeit only on the frontend now.

For other hooks, it is optional.

[delvh]

Perhaps we should either add some additional help text for the matrix one or use a new help string? Otherwise, it looks a bit strange as to why it appears optional but is required.

[oliverpool]

I can simply hide the help text for now for matrix. This way it is similar to the current form.

(As I mentioned, I would prefer this PR to be "focusing on the backend logic and improve the frontend at a later stage" - hiding the help text seems a good compromise, don't you think?)

[delvh]

Yep, sounds good.

[oliverpool]

@delvh the help text is now hidden for matrix. I squashed my changes and rebased on top of master.

[lunny]

Please resolve the conflicts.

[oliverpool]

@lunny, I find you comment very dry. I have been working on this PR for 3 months, I tried to quickly address all comments of the 5 reviewers, (I am still waiting for feedback on some: https://github.com/go-gitea/gitea/pull/20926#discussion_r979856917, https://github.com/go-gitea/gitea/pull/20926#discussion_r1001431292), and resolved conflicts a couple of times. After all those efforts, I have only 1 LGTM and I recently learned that this PR had no chance of being integrated in the 1.18 release :disappointed:

I know that all people working on this projects are volunteers and I am thankful for the time they took to look at my code. However it would have nicer if those people could also have taken time to answer my question on their own reviews (like @silverwind did - thank you!).

I am going to resolve the conflicts (not trivial, since I just found out that #17940 changed the exported fields of structs this PR is using), but I have to confess that I am getting tired of trying to contribute to this project (again, I am grateful for all the voluntary work that you do).

---

@delvh I did a merge of origin/main: is this the right workflow?

[lunny]

@lunny, I find you comment very dry. I have been working on this PR for 3 months, I tried to quickly address all comments of the 5 reviewers, (I am still waiting for feedback on some: #20926 (comment), #20926 (comment)), and resolved conflicts a couple of times. After all those efforts, I have only 1 LGTM and I recently learned that this PR had no chance of being integrated in the 1.18 release 😞

I know that all people working on this projects are volunteers and I am thankful for the time they took to look at my code. However it would have nicer if those people could also have taken time to answer my question on their own reviews (like @silverwind did - thank you!).

I am going to resolve the conflicts (not trivial, since I just found out that #17940 changed the exported fields of structs this PR is using), but I have to confess that I am getting tired of trying to contribute to this project (again, I am grateful for all the voluntary work that you do).

@delvh I did a merge of origin/main: is this the right workflow?

I also would like to merge this one into v1.18 and will review it again.

[lunny]

It's an accident approval because the ref comment included a word L-G-T-M.

[oliverpool]

I think there is a bug in the matrix-payload-unsafe conversion (hence this PR marked as draft).

[oliverpool]

Bug resolved: when removing payload_content from the hook_task in batch, we should always start at 0 (because the previous records are now filtered, since they don't contain payload_content anymore).

To show this bug, I could add 51 records to the test fixture : the last one wouldn't have been cleaned

[delvh]

https://github.com/go-gitea/gitea/releases/tag/v1.18.0-rc0

[oliverpool]

@lunny @delvh anything I can do to get this PR merged? (Besides resolving migration conflicts time and again…)

[lunny]

@lunny @delvh anything I can do to get this PR merged? (Besides resolving migration conflicts time and again…)

Please resolve the conflicts, I think it's ready for a long time but it's waiting to merge.

[oliverpool]

@lunny merge conflicts resolved.

[justusbunsi]

🚀 I guess I'll close my WIP PR then.

[Thaodan]

Does it make sense to backport this to 1.18.x?

[jolheiser]

Does it make sense to backport this to 1.18.x?

Generally only bugfixes are backported currently.