Page MenuHomekolab.org

Enhance protections in signup in two ways;
AbandonedPublic

Authored by vanmeeuwen on Dec 22 2021, 10:14 AM.

Details

Reviewers
machniak
Group Reviewers
Restricted Project
Summary
  • Ensure the short code is unique among all short codes that have not yet expired.
  • Allow protecting the signup controller against duplicate external email addresses, and duplicate source IP addresses.
Test Plan

None

Diff Detail

Repository
rK kolab
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 37439
Build 15017: arc lint + arc unit

Event Timeline

vanmeeuwen requested review of this revision.Dec 22 2021, 10:14 AM
vanmeeuwen created this revision.
mollekopf added inline comments.
src/app/SignupCode.php
107

Doesn't this check mean that if you accidentally generate a collision with an expired code it's reused?
So it seems to me you shouldn't check for isExpired and always generate again if there is a collision.

mollekopf added inline comments.Dec 22 2021, 10:54 AM
src/app/Rules/ExternalEmail.php
46

Perhaps add a comment that this is the same message on purpose, otherwise we're bound to "fix" this.

machniak requested changes to this revision.Dec 22 2021, 11:07 AM
machniak added a subscriber: machniak.
machniak added inline comments.
src/app/Rules/ExternalEmail.php
41

ExternalEmail rule is also used to validate group (distlist) members. And is not, but actually should be used to validate user external email. So, doing the signup related checks here is not right.

This revision now requires changes to proceed.Dec 22 2021, 11:07 AM
vanmeeuwen requested review of this revision.Dec 22 2021, 9:02 PM
vanmeeuwen marked an inline comment as done.
vanmeeuwen added inline comments.
src/app/Rules/ExternalEmail.php
41

Then where should they be?

46

This is apparently moving anyway.

src/app/SignupCode.php
107

No, but it at least prevents this;

MariaDB [kolabdev]> select count(*) as count, short_code from signup_codes where expires_at > NOW() group by short_code order by count desc limit 0,10;
+-------+------------+
| count | short_code |
+-------+------------+
|     3 | *****      |
|     3 | *****      |
|     3 | *****      |
|     2 | *****      |
(...)
10 rows in set (0.077 sec)
mollekopf added inline comments.Dec 23 2021, 9:11 AM
src/app/SignupCode.php
107

To me this code reads like:

  • Generate a supposedly unique shortcode
  • Check if it's unique
  • If it isn't unique and it's expired reuse it
  • If it isn't unique and not expired, generate a new one.

Maybe it's okay to reuse expired shortcodes because they're about to be purged anyways? In either case you shouldn't end up with duplicates, so at best this is an optimization (that may or may not be necessary) and at worst it's masking an error in the uniqueness guarantee.

machniak added inline comments.Dec 23 2021, 9:52 AM
src/app/Rules/ExternalEmail.php
41

Either in another rule class or in the SignupController.

46

emailinvalid does not make sense in this case, indeed.

vanmeeuwen abandoned this revision.Dec 24 2021, 9:44 AM
vanmeeuwen marked an inline comment as done.