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.

Sign up for GitHub

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

Add List-Unsubscribe header #17804

Merged
merged 5 commits into from
Dec 21, 2021
Merged

Add List-Unsubscribe header #17804

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4831ada
Add List-Unsubscribe header
mscherer Nov 24, 2021
01f59ec
Merge branch 'main' into fix_13283
zeripath Dec 20, 2021
c917dec
Merge branch 'main' into fix_13283
lunny Dec 21, 2021
a54fd00
Merge branch 'main' into fix_13283
lunny Dec 21, 2021
809a613
Merge branch 'main' into fix_13283
zeripath Dec 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion services/mailer/mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ func generateAdditionalHeaders(ctx *mailCommentContext, reason string, recipient
// https://datatracker.ietf.org/doc/html/rfc2369
"List-Archive": fmt.Sprintf("<%s>", repo.HTMLURL()),
//"List-Post": https://github.com/go-gitea/gitea/pull/13585
//"List-Unsubscribe": https://github.com/go-gitea/gitea/issues/10808, https://github.com/go-gitea/gitea/issues/13283
"List-Unsubscribe": ctx.Issue.HTMLURL(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ...).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@silverwind silverwind Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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".

Copy link
Member

Choose a reason for hiding this comment

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

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).


"X-Gitea-Reason": reason,
"X-Gitea-Sender": ctx.Doer.DisplayName(),
Expand Down
1 change: 1 addition & 0 deletions services/mailer/mail_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ func TestGenerateAdditionalHeaders(t *testing.T) {
expected := map[string]string{
"List-ID": "user2/repo1 <repo1.user2.localhost>",
"List-Archive": "<https://try.gitea.io/user2/repo1>",
"List-Unsubscribe": "https://try.gitea.io/user2/repo1/issues/1",
"X-Gitea-Reason": "dummy-reason",
"X-Gitea-Sender": "< U<se>r Tw<o > ><",
"X-Gitea-Recipient": "Test",
Expand Down