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

Added registration email blacklist #2352

Merged
merged 6 commits into from Feb 8, 2017

Conversation

woogerboy21
Copy link
Contributor

Fix #1862
Added the ability to define email blacklist for user registration. Now server operators can prevent users from registering accounts that contain providers they do not wish users to use. This also adds a new response protocol item with new client popup response indicating the reason for denied registration.

image

Added the ability to define email blacklist for user registration.  Now
server operators can prevent users from registering accounts that
contain providers they do not wish users to use.
; You can prevent users from using certain mail domains for registration. This setting contains a
; comma-seperated list of email provider domains that you would like to prevent users from using
; during registration. Example: "10minutemail.com,gmail.com,mail.com"
emailblacklist=""
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change the name of the config value to emailproviderblacklist so it's a little more clear

woogerboy21 and others added 2 commits January 13, 2017 13:01
@woogerboy21
Copy link
Contributor Author

@ZeldaZach Updated...

Copy link
Member

@ZeldaZach ZeldaZach left a comment

Choose a reason for hiding this comment

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

I reviewed the code base and it looks fine to me, but i'd like to see it tested. Can you set up the test server with this PR in and I'll give it a shot?

@woogerboy21
Copy link
Contributor Author

Sure, give me a bit and Ill see if I can stand something up this afternoon people can bounce tests off of.

@ctrlaltca
Copy link
Contributor

Code looks fine, the only note is about the use of "contains" when checking the provided email address against the blacklist: eg. if i blacklist "mail.com", it will implicitly block also "gmail.com", "whatevermail.com", "mail.computers.biz", etc..

I don't see this as a real problem as long as it's stated clearly in the comment before the setting, but the current comment mentions "10minutemail.com,gmail.com,mail.com" that is a misleading example as it contains "mail.com".

@ZeldaZach
Copy link
Member

@ctrlaltca Nice spotting on that. I don't like the wildcard effect of the blanket blocks (mail.com blocks gmail.com, etc). Can this PR be redone so it blocks only that specific host @woogerboy21 ?

@woogerboy21
Copy link
Contributor Author

@ZeldaZach I would prefer to keep the wildcard affect myself. Updating the description would be more of a solution that what your suggesting.

Added implicit explination
@tooomm
Copy link
Member

tooomm commented Jan 15, 2017

Haven't we something like that working here? #2200
Maybe grab the idea/concept?

@woogerboy21
Copy link
Contributor Author

woogerboy21 commented Jan 15, 2017

@tooomm That is for the actual username, we could apply it to the email component of registration as well. Regex always seems like overkill to me though. I've used it in other project (primarily for string text finding card pricing from websites) and though it allows for very granular definition of search criteria, it also adds a pretty large complexity component to those that don't really understand how to use it.

@woogerboy21
Copy link
Contributor Author

woogerboy21 commented Jan 16, 2017

@ZeldaZach I have a new testing server up with this rolled in. You can get to it by going to:

host: testing.woogerworks.com
port: 4748

The websocket server is disabled and the regular client is running on the port listed above.
The email provider 10minutemail.com has been blacklisted for testing. I can add or change it something else if you like just let me know.

@woogerboy21 woogerboy21 added the App - Servatrice Tickets relating to the servatrice application label Jan 19, 2017
@woogerboy21
Copy link
Contributor Author

@ZeldaZach Just an FYI on this one. I have email capabilities setup as well as changed the testing server information on the last post. So when you get around to testing this make sure you have things correct.

Copy link
Member

@ZeldaZach ZeldaZach left a comment

Choose a reason for hiding this comment

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

Works as expected. Nice job

@woogerboy21 woogerboy21 merged commit 0fdb9b7 into Cockatrice:master Feb 8, 2017
@woogerboy21 woogerboy21 deleted the emailblacklist branch July 24, 2017 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App - Servatrice Tickets relating to the servatrice application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants