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.

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

www: Email rate limit #1448

Merged
merged 26 commits into from
Jul 21, 2021
Merged

www: Email rate limit #1448

merged 26 commits into from
Jul 21, 2021

Conversation

thi4go
Copy link
Member

@thi4go thi4go commented Jul 2, 2021

This diff adds a email rate limiter functionality for the mail package. It extendsclient with the rate limiting functionality. This is done in order to avoid malicious behaviour and avoid spams on the politeia's smtp server.

  • Adds a Mailer interface to the mail package, providing an API to interact with the smtp client.

    • IsEnabled returns wether the smtp server is enabled
    • SendTo sends emails to anyone without rate limiting functionality
    • SendToUsers sends emails to politeiawww users with rate limiting functionality
  • Adds a new email_histories table to the user database.

    • user_id as primary key.
    • blob containing the user's email history, which in turn contains the unix timestamps in which the last emails were sent in a 24h window for that user, and a bool to tell if the user has already received the rate limit warning email.
  • Adds a MailerDB interface to the user package, used to interact with the email_histories table.

    • EmailHistoriesSave upsert email histories for the given users
    • EmailHistoriesGet retrieve email histories for the given users
  • Adds a TestMailerDB interface to the user package, used to test the rate limiting functionality from the mail package.

The reason a new table was created for this, instead of just adding this data to the user object, was to avoid race conditions on database calls, since our user database currently does not support transactions, and email notifications run in a separate go routine. This will not be the case once the user layer gets rewritten.

closes #1398

@thi4go thi4go closed this Jul 2, 2021
@thi4go thi4go reopened this Jul 2, 2021
@thi4go thi4go marked this pull request as draft July 2, 2021 19:16
@thi4go thi4go changed the title [wip] www: email rate limit www: Email rate limit Jul 6, 2021
@thi4go thi4go marked this pull request as ready for review July 6, 2021 15:15
Copy link
Member

@lukebp lukebp left a comment

Choose a reason for hiding this comment

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

The code looks pretty good. Its clean and readable. I think you've go an issue with one of your design decisions though.

I gave this a high level review and will take a deeper look once the design issue has been sorted out.

politeiawww/mail/client.go Show resolved Hide resolved
politeiawww/user/user.go Outdated Show resolved Hide resolved
politeiawww/mail/mail.go Show resolved Hide resolved
politeiawww/user/user.go Outdated Show resolved Hide resolved
politeiawww/user/user.go Outdated Show resolved Hide resolved
@thi4go
Copy link
Member Author

thi4go commented Jul 7, 2021

This is ready for another look mate @lukebp fixed the design issue.

Copy link
Member

@lukebp lukebp left a comment

Choose a reason for hiding this comment

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

This still isn't right. Why do you have two different mail clients?

@thi4go
Copy link
Member Author

thi4go commented Jul 9, 2021

@lukebp my bad, nuked limiter client and extended our existing client with the limiting functionality. Also created the user.MailerDB for a cleaner separation of concerns like we talked about, so that Mailer would not have unnecessary exposure to other userdb methods. Things look even more cleaner now 👍

politeiawww/user/user.go Outdated Show resolved Hide resolved
politeiawww/user/user.go Outdated Show resolved Hide resolved
politeiawww/user/user.go Outdated Show resolved Hide resolved
politeiawww/user/user.go Outdated Show resolved Hide resolved
politeiawww/mail/client.go Show resolved Hide resolved
politeiawww/user/mysql/mysql.go Show resolved Hide resolved
politeiawww/user/mysql/mysql.go Show resolved Hide resolved
politeiawww/user/mysql/mysql.go Outdated Show resolved Hide resolved
politeiawww/mail/client.go Outdated Show resolved Hide resolved
politeiawww/mail/client.go Outdated Show resolved Hide resolved
…okups, return concrete types intead of interface. & misc fixes. TODO: fixing tests
…tations (cdb,mysql) and reorder functions to match the interface declaration
Copy link
Member

@lukebp lukebp left a comment

Choose a reason for hiding this comment

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

Overall design looks good. Most of my comments are related to formatting or documentation.

politeiawww/mail/client.go Outdated Show resolved Hide resolved
politeiawww/mail/client.go Outdated Show resolved Hide resolved
politeiawww/mail/client.go Outdated Show resolved Hide resolved
politeiawww/mail/mail_test.go Outdated Show resolved Hide resolved
politeiawww/mail/mail.go Outdated Show resolved Hide resolved
politeiawww/mail/mail.go Show resolved Hide resolved
politeiawww/user/mail.go Show resolved Hide resolved
politeiawww/user/mail.go Show resolved Hide resolved
politeiawww/user/mail.go Outdated Show resolved Hide resolved
politeiawww/user/localdb/localdb.go Outdated Show resolved Hide resolved
Copy link
Member

@lukebp lukebp left a comment

Choose a reason for hiding this comment

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

This is looking much better. The code is clean and readable. I think the overall design looks good. There are still a couple parts I need to look at more carefully, but I've got to run. Remove the CMS bits and fix the comments. I'll give it a another review and test it once that's done.

politeiawww/mail/mail_test.go Outdated Show resolved Hide resolved
politeiawww/mail/client.go Outdated Show resolved Hide resolved
politeiawww/mail/mail_test.go Outdated Show resolved Hide resolved
politeiawww/pi/events.go Outdated Show resolved Hide resolved
politeiawww/pi/events.go Outdated Show resolved Hide resolved
politeiawww/user.go Outdated Show resolved Hide resolved
politeiawww/user/localdb/localdb.go Show resolved Hide resolved
politeiawww/user/mysql/mysql.go Outdated Show resolved Hide resolved
@lukebp
Copy link
Member

lukebp commented Jul 16, 2021

Update the original pull request message too with a more in depth breakdown. It should describe from a high level the interfaces that you added, the fact that this is a new databaes table instead of adding it to the user object, and why we chose to do that.

@thi4go
Copy link
Member Author

thi4go commented Jul 17, 2021

Done @lukebp, removed cms bits, addressed the remaining comments and updated the PR message 👍

politeiawww/mail/client.go Outdated Show resolved Hide resolved
Copy link
Member

@lukebp lukebp left a comment

Choose a reason for hiding this comment

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

ACK

politeiawww/mail/client.go Show resolved Hide resolved
politeiawww/mail/client.go Outdated Show resolved Hide resolved
@lukebp lukebp merged commit 84a52cf into decred:master Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

politeiawww: Rate limit email.
3 participants