Show real mail in Webhooks

[JakobDev]

Regression of #25097
Fixes #27918

The API should not return the real mail of a User, if you are not logged in. Webhooks however should show the real mail. As toUser() is used in a lot of places, I decided that it is easier to just use a context variable to identify if the context belongs to a Webhook than refactoring half Gitea to pass an argument.

[KN4CK3R]

I don't like that solution. You could get the same result with a simple ToWebhookUser() { toUser(ctx, user, true, true) }. But are we sure we want to ignore the users wish to not expose the email address?

[JakobDev]

You could get the same result with a simple ToWebhookUser() { toUser(ctx, user, true, true) }

ToUser is also called by some other functions e.g. ToRepo, so it's not that easy. I already thought about this, but we would need to add many functions.

are we sure we want to ignore the users wish to not expose the email address?

That's not ignored. If the User has enabled hiding the address, it will still be hidden.

[KN4CK3R]

You could get the same result with a simple ToWebhookUser() { toUser(ctx, user, true, true) }

ToUser is also called by some other functions e.g. ToRepo, so it's not that easy. I already thought about this, but we would need to add many functions.

How about using ToUserWithAccessMode which ToRepo uses? The webhook code could just pass an appropriate access mode.

are we sure we want to ignore the users wish to not expose the email address?

That's not ignored. If the User has enabled hiding the address, it will still be hidden.

Correct, my bad.

[JakobDev]

Repo was only a example. There are other objects like Issues or PullRequests, so this would involve a lot of work. Using the context is just the easiest way and I see no problems with this. I'm also not sure if a new Permission mode is the right place for that.

[KN4CK3R]

The context is the same as a global variable and using it here for control flow is bad design.

[JakobDev]

A context is not for global variables. the variables are just used within this context and are passed down the line.

[KN4CK3R]

Sure, still the context acts like a global variable (or thread local if you want) and you use it to pass variables down the callstack. That's a typical anti pattern.

https://news.ycombinator.com/item?id=24323564

• And the worst of all is passing things that decide business logic in your context variables! First, you lose all benefits of go's static typing. Second, you will not realize all of the crazy places that a context scoped variable is coming from, have fun debugging that!

https://medium.com/@gosamv/context-patterns-antipatterns-6bc48eaf774e

Passing required arguments a long way
Sometimes, you have a function which is very deep into the call stack, and it needs an ID or something which you know is available at a higher level. Resist the temptation to just throw it in the context: this hides what should be an explicit, required parameter.

[JakobDev]

I think in this case using a context is OK.

1. It not just passing down a single call stack. There are a lot of functions involved.
2. This just marks, that a context belongs to a Webhook. This is static and will never changed during the lifetime of the context, so you won't have to deal with typical problems of a global variable, like it's changed else where in the code.

[yp05327]

some related issues about hidden email, maybe we need a summary.
#29879
#27133

[wxiaoguang]

What do you think about " Fix missed doer #30231 " ? I think 30231 looks better and more intuitive

[wxiaoguang]

I have tested, the proper fix is #30231 , this PR could be closed.

-> Test webhook email #33033