feat(sms): Add support for rate-limiting sms actions #161
Conversation
shane-tomlinson
added this to the
FxA-53: Email Confirmation Flow - SMS (Phase 2) milestone
|
@philbooth @rfk This provides the rate limiting functionality for sms requests. r? |
|
@vbudhram this looks solid at a first glance, hopefully I can find the time for a thorough review soon. But you're right that it also really highlights the need for some serious refactoring in this repo! Let's chat next week about putting a plan in place to untangle the web of logic we've woven here over the years. |
| format: 'nat', | ||
| env: 'SMS_RATE_LIMIT_INTERVAL_SECONDS' | ||
| }, | ||
| maxSMSs: { |
philbooth
Feb 2, 2017
Contributor
I'm not a fan of the capitalisation here, especially as it's all lower case above. I suspect you did it because the pluralised double-s looks funny in lower case, but maybe that's a reason to ditch the plural. maxSms?
I'm not a fan of the capitalisation here, especially as it's all lower case above. I suspect you did it because the pluralised double-s looks funny in lower case, but maybe that's a reason to ditch the plural. maxSms?
vbudhram
Feb 2, 2017
Author
Contributor
Heh guess a pattern that I thought made sense to me was to capitalize acronyms only if in between words. No issues changing to maxSmss.
Heh guess a pattern that I thought made sense to me was to capitalize acronyms only if in between words. No issues changing to maxSmss.
|
|
||
| isSMSSendingAction: function isSMSSendingAction(action) { |
philbooth
Feb 2, 2017
•
Contributor
If you switched to ES6 style here, we could end the senseless tyranny of double-named functions that's blighting this module.
If you switched to ES6 style here, we could end the senseless tyranny of double-named functions that's blighting this module.
| @@ -123,6 +124,32 @@ module.exports = function (limits, now) { | |||
| this.as = this._trim(now, this.as, limits.maxAccountStatusCheck) | |||
| } | |||
|
|
|||
| IpRecord.prototype.isOverSMSLimit = function () { | |||
| this.trimSMSRequests(now()) | |||
philbooth
Feb 2, 2017
Contributor
...oh you've capitalised them here as well. I don't know, feel free to ignore what I said above in that case, just as long as it's upper-cased everywhere. A sometimes-uppercase-but-sometimes-lowercase mish-mash would keep me awake at night.
...oh you've capitalised them here as well. I don't know, feel free to ignore what I said above in that case, just as long as it's upper-cased everywhere. A sometimes-uppercase-but-sometimes-lowercase mish-mash would keep me awake at night.
| // Limit based on number of unique sms request sent by this IP | ||
| var count = 0 | ||
| var seen = {} | ||
| this.sms.forEach(function(info) { |
philbooth
Feb 2, 2017
Contributor
...but then I see this little guy and I think, no, all-caps is madness.
...but then I see this little guy and I think, no, all-caps is madness.
| var count = 0 | ||
| var seen = {} | ||
| this.sms.forEach(function(info) { | ||
| if (!(info.u in seen)) { |
philbooth
Feb 2, 2017
Contributor
I can see they're used elsewhere here but, for the record, I'm not a fan of these one-character (and two-character) property names. It's like we're explicitly trying to make the code harder to follow. Using t instead of time at least has an element of logic, but why u for the phone number? Am I missing something?
I can see they're used elsewhere here but, for the record, I'm not a fan of these one-character (and two-character) property names. It's like we're explicitly trying to make the code harder to follow. Using t instead of time at least has an element of logic, but why u for the phone number? Am I missing something?
rfk
Feb 8, 2017
Member
FWIW the u in other places here was for "user".
FWIW the u in other places here was for "user".
| // Check against SMS records to make sure that this request | ||
| // can send SMS | ||
| if (smsNumber) { | ||
| promises.push(mc.getAsync(smsNumber).then(smsRecord.parse, smsRecord.parse)) |
philbooth
Feb 2, 2017
•
Contributor
Scrolls up to the top of the file to look for what mc means. Scrolls back down.
Scrolls up to the top of the file to look for what mc means. Scrolls back down.
| var rec = new SMSRecord() | ||
| object = object || {} | ||
| rec.rl = object.rl // timestamp when the account was rate-limited | ||
| rec.xs = object.xs || [] // timestamps when sms were sent |
philbooth
Feb 2, 2017
Contributor
Sigh
Sigh
vbudhram
Feb 2, 2017
Author
Contributor
@rfk Looking back at code, these records get read/written to memcache. Is that why the code is using as small a possible var names? This pattern was before my time.
@rfk Looking back at code, these records get read/written to memcache. Is that why the code is using as small a possible var names? This pattern was before my time.
rfk
Feb 2, 2017
Member
Yes, IIRC exactly this, those properties go straight into memcache as JSON so this was designed to keep the memory pressure down.
Yes, IIRC exactly this, those properties go straight into memcache as JSON so this was designed to keep the memory pressure down.
rfk
Feb 2, 2017
Member
(which is not to argue that it's the right tradeoff today, just to explain why it's there)
(which is not to argue that it's the right tradeoff today, just to explain why it's there)
rfk
Feb 8, 2017
Member
OK, I'll admit, I have no idea what "xs" is supposes to stand for in the email_record class, nor here. But at least it's consistent between the two I suppose...
OK, I'll admit, I have no idea what "xs" is supposes to stand for in the email_record class, nor here. But at least it's consistent between the two I suppose...
|
Looks fine to me @vbudhram, although I'm not very familiar with this repo so that probably doesn't qualify as an official r+. This is not aimed at you, but the variable/property names in this repo are completely mad, aren't they? |
| // Actions that can send sms, and could make us | ||
| // very annoying to a user if abused. | ||
| const SMS_SENDING_ACTION = { | ||
| inviteUserSms: true |
vladikoff
Feb 2, 2017
Contributor
what's the logic behind this inviteUserSms name?
what's the logic behind this inviteUserSms name?
vbudhram
Feb 3, 2017
Author
Contributor
I should have made a note about this. Since the auth-server sms endpoint will be generic, I needed some way to differentiate the type of sms being sent. Currently, this should only be for inviting users, however I could see us reusing the sms endpoint for 2FA etc. I am open for other another name.
I should have made a note about this. Since the auth-server sms endpoint will be generic, I needed some way to differentiate the type of sms being sent. Currently, this should only be for inviting users, however I could see us reusing the sms endpoint for 2FA etc. I am open for other another name.
vbudhram
Feb 3, 2017
Author
Contributor
@vladikoff Thought about it some more and really what is happening is the user is sending an sms to connect a device, not inviting a user. Renamed to connectDeviceSms.
@vladikoff Thought about it some more and really what is happening is the user is sending an sms to connect a device, not inviting a user. Renamed to connectDeviceSms.
|
Fixes #160 |
| var rec = new SMSRecord() | ||
| object = object || {} | ||
| rec.rl = object.rl // timestamp when the account was rate-limited | ||
| rec.xs = object.xs || [] // timestamps when sms were sent |
rfk
Feb 8, 2017
Member
OK, I'll admit, I have no idea what "xs" is supposes to stand for in the email_record class, nor here. But at least it's consistent between the two I suppose...
OK, I'll admit, I have no idea what "xs" is supposes to stand for in the email_record class, nor here. But at least it's consistent between the two I suppose...
|
r+, but with the caveat that we're going to get cracking on that long-overdue refactor in this repo, as discussed IRL earlier today :-) |
|
@rfk @philbooth Thanks guys! |
|
|
| module.exports = { | ||
|
|
||
| isPasswordCheckingAction: function isPasswordCheckingAction(action) { |
vladikoff
Feb 9, 2017
Contributor
this was here because it is useful during debugging / testing, but we can live without it I hope
this was here because it is useful during debugging / testing, but we can live without it I hope
vbudhram
Feb 9, 2017
Author
Contributor
I think it will be ok? I don't mind adding it back if we need it.
I think it will be ok? I don't mind adding it back if we need it.
Fixes #160
This PR adds the following support to the custom's server:
Connects with mozilla/fxa-features#53, mozilla/fxa-auth-server#1628