Add List-Unsubscribe header

[mscherer]

Fixes #13283

[Gusted]

Well I do like simple solutions, I will play the annoying reviewer. I think, we can do better than this. the best would be if we could append something like ?unscribe-me=true whereby the front-end(or the back-end would render a different page) that could show a confirm for unsubscribing.

[mscherer]

I agree, it can be improved. A form seems a good idea, but then, should the interface show 1 button to unsub from this issue, or to unsub from all issues ?

And should it impact just email, or all notifications ?

Looking at prior art (eg, github), it has a link to https://github.com/notifications/unsubscribe/someopaqueidofthenotification, that go to the issue directly, and unsubscribe me from it.

Not sure how gitlab behave, but I would be tempted to start simple, and get a new url to unsubscribe from a issue (eg a streamlined interface with 1 big button), and we can later add more if we want (I am using gitea for my own stuff so I am not getting much notifications from it ...).

[Gusted]

I think we can hold it simple for now, and show a simple form with "Would you like to be unsubscribed from issue xxx" with a confirm button.

[lafriks]

We can still render issue page that would display dialog on load to confirm unsubscribe

[silverwind]

I guess we could directly unsubscribe on page open and render a info box "You have been unsubscribed from this issue" and render the regular issue/pr page.

[Gusted]

Well, let it be then. Let's hold it simple and do it that way. If it works then it works 😄

[mscherer]

So, we agree on directly unsubscribe on 1 single issue for that header for now ? (we can switch to a different behavior later)

I guess this requires a new route, I will try to find a name that make sense (especially if we want to change later). I guess https://git.example.org/org/repo/issues/4/unsubscribe, I will try to code it this week.

[lafriks]

No, such url can not be GET, it will be problematic from security perspective. GitHub generates per-user and issue combination unique unguessable URL

[mscherer]

Yeah, a attacker could guess the link and place it as a image or some others stuff.

But so, would using JWT in the URL be ok ?

I am not keen on adding a table "notification id" <-> "user/issue", as that look like a recipe for unbounded database growth (maybe not a problem in practice). Using a token in the URL allow us to have a stateless system, and we can reuse the existing JWT secret. We can just encode the user and issue ID in json, and unsubscribe based on that, since that's signed. The issue ID is public, so that should be ok.

I am not sure if we should use a internal ID (eg ID) or LoginName. ID seems better (shorter, not going to change), but not sure if that's suitable for use outside of the DB. The LoginName is public, but some system support login change, which would break that feature.

Given gitea do not use JWT that much for now, I do not think a token type is needed (eg, some member "action": "unsubscribe"). The main reason to add one would be to prevent reusing a token made for action X to be used on action Y (for example, a url to subscribe, using the same JWT format, would be vulnerable to a token reuse to subscribe someone with a token used to unsubscribe). But I do not have a non convoluted example, so maybe that's not a issue.

And if the token is not correctly signed (eg, JWT key rotated ), we can also redirect the user to the issue page with a popup "we can't verify the token".

[silverwind]

If we can not make it GET (and I tend to agree that generally doing actions on GET is bad practice), I think we should just link to the Issue as done in this PR make it a JS UI interaction.

On idea may be to link to the issue while appending #unsubscribe or ?unsubscribe to the URL, which then can trigger a JS dialog to confirm the action. Benefit of using hash is that the server won't see it (it doesn't need to when it only concerns the UI).

[silverwind]

I guess we can land it like this and improve upon it later.

[zeripath]

Agreed with @silverwind although a direct unsubscribe link would be better - this can be merged as is at the moment.

[codecov-commenter]

Codecov Report

Merging #17804 (809a613) into main (7be82f4) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #17804      +/-   ##
==========================================
- Coverage   45.21%   45.21%   -0.01%     
==========================================
  Files         824      824              
  Lines       91441    91442       +1     
==========================================
  Hits        41346    41346              
- Misses      43492    43495       +3     
+ Partials     6603     6601       -2     

Impacted Files	Coverage Δ
services/mailer/mail.go	63.89% <100.00%> (+0.13%)	⬆️
modules/queue/workerpool.go	50.00% <0.00%> (-3.06%)	⬇️
models/repo_list.go	83.19% <0.00%> (-0.82%)	⬇️
modules/process/manager.go	82.94% <0.00%> (+1.55%)	⬆️
modules/log/file.go	75.20% <0.00%> (+1.60%)	⬆️
modules/queue/queue_bytefifo.go	62.87% <0.00%> (+2.99%)	⬆️

---

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 7be82f4...809a613. Read the comment docs.