Configurable Username Policies by 0xEAB · Pull Request #13 · rejectedsoftware/userman

0xEAB · 2017-07-09T01:55:10Z

My changes add the possibility to configure what username policies are applied.

The main reason

The year is 2017. Homosexual people in the Netherlands have been allowed to marry for over 16 years by now; more and more countries have been or are following. Almost all European countries have abandoned the death penalty.

Nevertheless, people wanting to register with usernames that start with a number (like mine does 😊) cannot signup on several pages. This includes practically all services relying on userman (and thus also the dub-registry).

It's a shame that people still have to use umpteen fancy usernames on different sites because of weird and incomprehensible policies all over the WWW.

I wonder if this is really necessary ...
Why can't I simply have one single username that I can use everywhere? Because it's a hex number? Because it starts with a number? Because it has only 5 characters? Because it's easy to remember as it's based on my name's initials?
Whatever it is, it's always just ridiculous. Well, I'm a bit disappointed.

Findings

As you might guess, userman's new default setting for noNumberStart is, of course, false. Because it would be simply wrong to enable it. If someone really needs to enforce this (and I doubt there will ever be one), she/he can enable it explicitly on purpose.

By the way

Facebook, Twitter, VK, GitHub, GitLab, Discord, and so on, all those allow usernames starting with a number.

Note

I don't want to accuse anyone of anything. In this case, the problem simply comes from a design flaw in the defaults of vibe.d's username validation and userman was only affected because it lazily relied on those.

Conclusion

I would be very happy if these changes would find their way into the dub-registry one day.

- length: min + max - characters: whitelist + firstCharNumber? - improved consistency (member naming) - added options in web admin

Error: function vibe.core.core.CoreTask.state cannot override final function core.thread.Fiber.state

Many of them are caused by an alias in 'userman.d' and seem to happen in 'vibe.d'. Don't get confused! The problem actually here.

This reverts commit a9109dc.

0xEAB · 2017-07-09T02:02:59Z

TL;DR

Username policies applied can now be configured.
Usernames starting with a number (e.g. mine @0xEAB) are allowed by default.
There is no reason not to do so.
The dub-registry "suffers" from this problem, too.

0xEAB · 2017-07-09T02:04:26Z

StillTL;DR

I like my nick (@0xEAB) and I want to be able to use it.

s-ludwig · 2017-07-09T09:47:41Z

I didn't yet look at the implementation, but I fully agree to the change. There is usually no need to enforce programming language identifier rules here! The default for validateUserName simply uses the wrong default due to lack of thought (simply forwarding the same default that validateIndentifier uses).

s-ludwig · 2017-07-09T09:55:22Z

source/userman/userman.d

+/**
+	Settings also used by the API
+ */
+struct UserManCommonSettings {


There is an inconsistency with other vibe.d related libraries that use settings types here. Usually all of them use class for this, but for some reason APISettings, which should also really be UserManAPISettings, uses struct. So this should really use class and inheritance instead.

s-ludwig · 2017-07-09T09:55:28Z

source/userman/userman.d

 		mailSettings = new SMTPClientSettings;
 	}
+
+	deprecated("Consistency: Use .serviceURL instead.") @property {


Any reason not to put this into UserManCommonSettings as an alias instead?

Basically, I simply forgot to do so.

s-ludwig · 2017-07-09T09:56:22Z

source/userman/web.d


 	@noAuth @errorDisplay!getRegister
-	void postRegister(ValidEmail email, Nullable!ValidUsername name, string fullName, ValidPassword password, Confirm!"password" passwordConfirmation)
+	void postRegister(ValidEmail email, string name, string fullName, ValidPassword password, Confirm!"password" passwordConfirmation)


I need to put in some more thought into this. It would really be nice to have a facility that allows configuring the validation types instead of having to resort to manual validation here.

One advantage of my changes is that the user will see a reasonable error message now. At the moment, when I try to sign up at the dub-registry, all I get is Missing user name field., though vibe.d actually receives 0xEAB.

The wrong error message is actually a bug in the handling of Nullable!T. I'll push a fix for that.

There is one more issue that I missed last time. It needs to be Nullable!string, or omitting the name field will fail before checking m_settings.useUserNames below. Other than that, I think we can keep the manual validation for now.

- consistency - fixed deprecation warnings caused by previous changes

A parameter that failed validation was erroneously treated as unset. See also rejectedsoftware/userman#13.

s-ludwig · 2017-07-15T19:14:20Z

source/userman/web.d


 	@noAuth @errorDisplay!getRegister
-	void postRegister(ValidEmail email, Nullable!ValidUsername name, string fullName, ValidPassword password, Confirm!"password" passwordConfirmation)
+	void postRegister(ValidEmail email, string name, string fullName, ValidPassword password, Confirm!"password" passwordConfirmation)


There is one more issue that I missed last time. It needs to be Nullable!string, or omitting the name field will fail before checking m_settings.useUserNames below. Other than that, I think we can keep the manual validation for now.

s-ludwig · 2017-07-15T19:16:35Z

source/userman/userman.d

+	string additionalChars = "-_";
+	bool noNumberStart = false; // it's always a good idea to keep this option *disabled* 
+
+	public bool validateUserName(R)(ref R error_sink, string userName)


I'd leave this declared as package. If a use case for this occurs outside of userman itself, we can still add it to the public API. But if, for example, this gets switched back to ValidUserName, this will have to be deprecated again, or will stay as an unused function.

»It needs to be Nullable!string, or omitting the name field will fail before checking m_settings.useUserNames below.« - Sönke Ludwig

0xEAB · 2017-07-15T23:49:28Z

Is there anything else to do?

s-ludwig · 2017-07-16T08:54:59Z

Looks good now, thanks!

Configurable Username Policies Conflicts: example/source/app.d source/userman/api.d source/userman/web.d source/userman/webadmin.d views/userman.admin.settings.dt

See #13.

s-ludwig · 2017-07-16T09:48:05Z

A new version of the registry with the changes in is now deployed!

0xEAB · 2017-07-16T15:33:18Z

I have no idea, what exactly went wrong, but, unfortunately, signing up at the dub-registry now leads to an error.

500 - Internal Server Error

Internal Server Error

s-ludwig · 2017-07-16T16:42:56Z

Hm, seems like I uploaded an older binary. The error was already fixed by 560e4b7
I've re-uploaded the current version now.

0xEAB added 5 commits July 9, 2017 01:12

Added configurable username policies

503bed0

- length: min + max - characters: whitelist + firstCharNumber? - improved consistency (member naming) - added options in web admin

Fixed build

a9109dc

Error: function vibe.core.core.CoreTask.state cannot override final function core.thread.Fiber.state

Fixed some deprecation warnings

fa3fe91

Many of them are caused by an alias in 'userman.d' and seem to happen in 'vibe.d'. Don't get confused! The problem actually here.

Username policies now also apply to webadmin

0dc5653

Revert "Fixed build"

87da607

This reverts commit a9109dc.

s-ludwig requested changes Jul 9, 2017

View reviewed changes

Revised 'Configurable Username Policies'

101de67

- consistency - fixed deprecation warnings caused by previous changes

s-ludwig added a commit to vibe-d/vibe.d that referenced this pull request Jul 9, 2017

Fix error message for Nullable!T parameters in web interfaces.

5ef298f

A parameter that failed validation was erroneously treated as unset. See also rejectedsoftware/userman#13.

s-ludwig requested changes Jul 15, 2017

View reviewed changes

Fixed postRegister() params

9696cc0

»It needs to be Nullable!string, or omitting the name field will fail before checking m_settings.useUserNames below.« - Sönke Ludwig

Fix condition.

62bfcf6

s-ludwig approved these changes Jul 16, 2017

View reviewed changes

s-ludwig merged commit 5139198 into rejectedsoftware:master Jul 16, 2017

s-ludwig added a commit that referenced this pull request Jul 16, 2017

Merge pull request #13 from 0xEAB/master

741bcbc

Configurable Username Policies Conflicts: example/source/app.d source/userman/api.d source/userman/web.d source/userman/webadmin.d views/userman.admin.settings.dt

s-ludwig added a commit that referenced this pull request Jul 16, 2017

Fix null pointer access for default UserManSettings.

6e37ef9

See #13.

s-ludwig added a commit that referenced this pull request Jul 16, 2017

Fix null pointer access for default UserManSettings.

560e4b7

See #13.

Conversation

0xEAB commented Jul 9, 2017

The main reason

Findings

By the way

Note

Conclusion

Uh oh!

0xEAB commented Jul 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Uh oh!

0xEAB commented Jul 9, 2017

StillTL;DR

Uh oh!

s-ludwig commented Jul 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xEAB commented Jul 15, 2017

Uh oh!

s-ludwig commented Jul 16, 2017

Uh oh!

s-ludwig commented Jul 16, 2017

Uh oh!

0xEAB commented Jul 16, 2017

Uh oh!

s-ludwig commented Jul 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

0xEAB commented Jul 9, 2017 •

edited

Loading