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

Jump to bottom

Save initial signup information for users to aid in spam prevention #31852

Merged
merged 14 commits into from
Sep 9, 2024
Merged

Save initial signup information for users to aid in spam prevention #31852

merged 14 commits into from
Sep 9, 2024

Conversation

techknowlogick
Copy link
Member

This will allow instance admins to view signup pattern patterns for public instances. It is modelled after discourse, mastodon, and MediaWiki's approaches.

Note: This has privacy implications, but as the above-stated open-source projects take this approach, especially MediaWiki, which I have no doubt looked into this thoroughly, it is likely okay for us, too. However, I would be appreciative of any feedback on how this could be improved.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 17, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 17, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Aug 17, 2024
@techknowlogick techknowlogick mentioned this pull request Aug 17, 2024
Open
denyskon
denyskon suggested changes Aug 17, 2024
View reviewed changes
Copy link
Member

@denyskon denyskon left a comment

Choose a reason for hiding this comment

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

I support integrating this functionality, however, please make it opt-out so that instance admins can decide on their own if they do not want this feature due to privacy reasons.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 17, 2024
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 17, 2024
@github-actions github-actions bot added the docs-update-needed The document needs to be updated synchronously label Aug 17, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Aug 17, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 17, 2024
wxiaoguang
wxiaoguang reviewed Aug 18, 2024
View reviewed changes
models/user/user.go Outdated Show resolved Hide resolved
wxiaoguang
wxiaoguang reviewed Aug 18, 2024
View reviewed changes
models/user/user.go Outdated Show resolved Hide resolved
@techknowlogick
Update setting name
e798b2d 
Use consts
Cleanup code meant for different branch
@lafriks
Copy link
Member

lafriks commented Aug 19, 2024 •
edited
Loading

I support integrating this functionality, however, please make it opt-out so that instance admins can decide on their own if they do not want this feature due to privacy reasons.

I don't really think there is a privacy problem here as IP addresses should be kept for auditing purposes anyway and browser user agent string is not person identifying information or anyhow related to privacy

@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Sep 7, 2024
@techknowlogick techknowlogick added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 7, 2024
@techknowlogick
Copy link
Member Author

@lafriks, I'm inclined to merge this as is, with it disabled by default. Then, you could open a new PR to change it so a more in-depth discussion can be had.

delvh
delvh approved these changes Sep 7, 2024
View reviewed changes
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

I don't understand how this can be useful to detect spam/abuse.
But the implementation seems fine as it is.

@delvh
Copy link
Member

delvh commented Sep 7, 2024

What I do understand however, is that this is mostly intended for public instances.
So perhaps we should describe in the config docs that especially those instances should consider enabling it?

@lunny
Copy link
Member

lunny commented Sep 7, 2024

How about creating a CreateUserOptions as a parameter, user and others could be parameter of that struct.

@techknowlogick
Copy link
Member Author

@delvh by recording signup IPs you can track if multiple users share the same address in case a spam ring creates many accounts. And the user agent is useful too as you can see additional behaviours

@techknowlogick
Copy link
Member Author

@lunny yes, that can be done, but it's out of scope for this PR as I don't want to refactor too much in toys one.

@techknowlogick techknowlogick merged commit f183783 into go-gitea:main Sep 9, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.24.0 milestone Sep 9, 2024
@techknowlogick techknowlogick deleted the signup-ip branch September 9, 2024 21:05
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 9, 2024
@yp05327
Copy link
Contributor

yp05327 commented Sep 10, 2024

@lunny
Should we move this PR to 1.23.0 milestone? As 1.23.0 is not released yet.

@lunny lunny modified the milestones: 1.24.0, 1.23.0 Sep 10, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 12, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
bc3be4a 
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Failed authentications are logged to level Warning (go-gitea#32016)
  Fix `/repos/{owner}/{repo}/pulls/{index}/files` endpoint not populating `previous_filename` (go-gitea#32017)
  Support allowed hosts for migrations to work with proxy (go-gitea#32025)
  Support migration from AWS CodeCommit (go-gitea#31981)
  bump to go 1.23 (go-gitea#31855)
  Enable compression for Actions logs by default (go-gitea#32013)
  Save initial signup information for users to aid in spam prevention (go-gitea#31852)
  Increase `cacheContextLifetime` to reduce false reports (go-gitea#32011)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@delvh delvh delvh approved these changes

@denyskon denyskon denyskon approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
docs-update-needed The document needs to be updated synchronously lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.
8 participants
@techknowlogick @lafriks @delvh @lunny @yp05327 @wxiaoguang @denyskon @GiteaBot