Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Force username & email to lowerCase PLUS Remove sensitive data before… #837

Closed
wants to merge 0 commits into from

Conversation

almegdad
Copy link
Contributor

… return authenticated user in password reset

@lirantal
Copy link
Member

@rhutchison @codydaig @ilanbiala @mleanos any thoughts?

@lirantal lirantal self-assigned this Aug 23, 2015
@@ -6,7 +6,7 @@ <h3 class="col-md-12 text-center">Or with your account</h3>
<div class="form-group">
<label for="username">Username</label>
<input type="text" id="username" name="username" class="form-control"
data-ng-model="credentials.username" placeholder="Username">
data-ng-model="credentials.username" data-ng-change="credentials.username=credentials.username.toLowerCase();" placeholder="Username">
Copy link
Member

Choose a reason for hiding this comment

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

We seem to have decided not to use the data- prefix for angular attributes so please change this through-out the PR to just ng-change and so on..

@rhutchison
Copy link
Contributor

can we use css text-transform: lowercase; instead of ng-change, and then on save (client or server) transform to lower?

@mleanos
Copy link
Member

mleanos commented Aug 25, 2015

I agree with @rhutchison that this should be done with the save method, either on the client or server side. I'm not a fan of using ng-change for these types of scenarios.

I don't see a need to transform with css. The real need here is to change the username to lowercase, sometime, before authentication; in order to decrease the chance of user error.

However, if the view must be updated to reflect the lowercase change, then I'd say that's an argument to do the conversion, in the client controller save method; thus, updating the view.

@mleanos
Copy link
Member

mleanos commented Aug 25, 2015

After looking at these changes a bit further, I think the lowercase conversion should be done at the very front-end with the user input; most likely in the client controller method that initiates any API calls. If these changes are made in the middleware, they can get lost & the SOC gets weaker.

To ensure the username & email always get converted to lowercase, the pre-save hook can be used on the User model.

@rhutchison
Copy link
Contributor

I don't see a need to transform with css. The real need here is to change the username to lowercase, sometime, before authentication; in order to decrease the chance of user error.

However, if the view must be updated to reflect the lowercase change, then I'd say that's an argument to do the conversion, in the client controller save method; thus, updating the view.

@mleanos Why wouldn't you just have text-transform? the user doesn't need to know when it's transformed, just that it is being transformed.

@mleanos
Copy link
Member

mleanos commented Aug 26, 2015

@rhutchison I don't like using css as a means to transform data based on business logic/needs. The client/server controller would be sufficient in most cases. If someone wants to update the view to show the transformation, then adding the css text-transform would be an enhancement, IMO.

@ilanbiala
Copy link
Member

.toLowerCase is good, but I'd do it in the JS and not in the HTML.

@simison
Copy link
Member

simison commented Aug 27, 2015

Those ng-change bits could be removed; after that this LGTM and pretty much how I'm doing it already at my copy.

+1 @mleanos - CSS/angular frontend tweaking seems hacky.

I understand the sentiment tho; without it changing case sensitive input to lowercase at the backend comes as a surprise after hitting save. Changing text on the go while typing might be confusing as well.

Personally I'm saving displayUsername separately, much like displayName works, so users can keep their CamelCaseUsernames and no confusing UX.

@almegdad
Copy link
Contributor Author

reopened #866

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants