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

Commit

Permalink
feat(users): change username to usernameOrEmail in signin (#1545)
Browse files Browse the repository at this point in the history
* feat(users): change username to usernameOrEmail in signin

* fix(users): toLowerCase at email in local strategy
  • Loading branch information
itelo authored and lirantal committed Oct 6, 2016
1 parent 73a7c2c commit 6a6b630
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe('Article Admin CRUD tests', function () {
beforeEach(function (done) {
// Create user credentials
credentials = {
username: 'username',
usernameOrEmail: 'username',
password: 'M3@n.jsI$Aw3$0m3'
};

Expand All @@ -43,7 +43,7 @@ describe('Article Admin CRUD tests', function () {
displayName: 'Full Name',
email: 'test@test.com',
roles: ['user', 'admin'],
username: credentials.username,
username: credentials.usernameOrEmail,
password: credentials.password,
provider: 'local'
});
Expand Down
12 changes: 6 additions & 6 deletions modules/articles/tests/server/article.server.routes.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('Article CRUD tests', function () {
beforeEach(function (done) {
// Create user credentials
credentials = {
username: 'username',
usernameOrEmail: 'username',
password: 'M3@n.jsI$Aw3$0m3'
};

Expand All @@ -43,7 +43,7 @@ describe('Article CRUD tests', function () {
lastName: 'Name',
displayName: 'Full Name',
email: 'test@test.com',
username: credentials.username,
username: credentials.usernameOrEmail,
password: credentials.password,
provider: 'local'
});
Expand Down Expand Up @@ -216,7 +216,7 @@ describe('Article CRUD tests', function () {
it('should be able to get a single article that has an orphaned user reference', function (done) {
// Create orphan user creds
var _creds = {
username: 'orphan',
usernameOrEmail: 'orphan',
password: 'M3@n.jsI$Aw3$0m3'
};

Expand All @@ -226,7 +226,7 @@ describe('Article CRUD tests', function () {
lastName: 'Name',
displayName: 'Full Name',
email: 'orphan@test.com',
username: _creds.username,
username: _creds.usernameOrEmail,
password: _creds.password,
provider: 'local',
roles: ['admin']
Expand Down Expand Up @@ -322,7 +322,7 @@ describe('Article CRUD tests', function () {
it('should be able to get single article, that a different user created, if logged in & verify the "isCurrentUserOwner" field is set to "false"', function (done) {
// Create temporary user creds
var _creds = {
username: 'articleowner',
usernameOrEmail: 'articleowner',
password: 'M3@n.jsI$Aw3$0m3'
};

Expand All @@ -332,7 +332,7 @@ describe('Article CRUD tests', function () {
lastName: 'Name',
displayName: 'Full Name',
email: 'temp@test.com',
username: _creds.username,
username: _creds.usernameOrEmail,
password: _creds.password,
provider: 'local',
roles: ['admin', 'user']
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ <h3 class="col-xs-12 text-center">Or with your account</h3>
<uib-alert type="danger" ng-show="vm.error" class="text-center text-danger">
<span ng-bind="vm.error"></span>
</uib-alert>
<label for="username">Username</label>
<input type="text" id="username" name="username" class="form-control" ng-model="vm.credentials.username" placeholder="Username" lowercase required autofocus>
<div ng-messages="vm.userForm.username.$error" role="alert">
<p class="help-block error-text" ng-message="required">Username is required.</p>
<label for="usernameOrEmail">Username or Email</label>
<input type="text" id="usernameOrEmail" name="usernameOrEmail" class="form-control" ng-model="vm.credentials.usernameOrEmail" placeholder="Username or Email" required autofocus>
<div ng-messages="vm.userForm.usernameOrEmail.$error" role="alert">
<p class="help-block error-text" ng-message="required">Username or Email is required.</p>
</div>
</div>
<div class="form-group" show-errors>
Expand Down
10 changes: 7 additions & 3 deletions modules/users/server/config/strategies/local.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,16 @@ var passport = require('passport'),
module.exports = function () {
// Use local strategy
passport.use(new LocalStrategy({
usernameField: 'username',
usernameField: 'usernameOrEmail',
passwordField: 'password'
},
function (username, password, done) {
function (usernameOrEmail, password, done) {
User.findOne({
username: username.toLowerCase()
$or: [{
username: usernameOrEmail.toLowerCase()
}, {
email: usernameOrEmail.toLowerCase()
}]
}, function (err, user) {
if (err) {
return done(err);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
}));

describe('$scope.signin()', function () {
it('should login with a correct user and password', function () {
it('should login with a correct username and password', function () {
// Test expected GET request
$httpBackend.when('POST', 'api/auth/signin').respond(200, { username: 'Fred' });

Expand All @@ -60,6 +60,18 @@
expect($location.url()).toEqual('/');
});

it('should login with a correct email and password', function () {
// Test expected GET request
$httpBackend.when('POST', 'api/auth/signin').respond(200, { email: 'Fred@email.com' });

scope.vm.signin(true);
$httpBackend.flush();

// Test scope value
expect(scope.vm.authentication.user.email).toEqual('Fred@email.com');
expect($location.url()).toEqual('/');
});

it('should be redirected to previous state after successful login',
inject(function (_$state_) {
$state = _$state_;
Expand Down Expand Up @@ -101,7 +113,7 @@

it('should fail to log in with wrong credentials', function () {
// Foo/Bar combo assumed to not exist
scope.vm.authentication.user = { usersname: 'Foo' };
scope.vm.authentication.user = { username: 'Foo' };
scope.vm.credentials = 'Bar';

// Test expected POST request
Expand Down
6 changes: 3 additions & 3 deletions modules/users/tests/e2e/users.e2e.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ describe('Users E2E Tests:', function () {
expect(element.all(by.css('.error-text')).get(0).getText()).toBe('Email address is invalid.');
});

it('Should report missing username', function () {
it('Should report missing username or email', function () {

This comment has been minimized.

Copy link
@PierreBrisorgueil

PierreBrisorgueil Oct 18, 2016

Contributor

Hello @itelo , it's signup, so we have two inputs for me, email, and username,

it('Should report missing username or email', function () {
should be

it('Should report missing username', function () {

This comment has been minimized.

Copy link
@itelo

itelo Oct 18, 2016

Author Contributor

You're correct, should be username only

browser.get('http://localhost:3001/authentication/signup');
// Enter First Name
element(by.model('vm.credentials.firstName')).sendKeys(user1.firstName);
Expand Down Expand Up @@ -306,7 +306,7 @@ describe('Users E2E Tests:', function () {
// Click Submit button
element(by.css('button[type="submit"]')).click();
// Username Error
expect(element.all(by.css('.error-text')).get(0).getText()).toBe('Username is required.');
expect(element.all(by.css('.error-text')).get(0).getText()).toBe('Username or Email is required.');
// Password Error
expect(element.all(by.css('.error-text')).get(1).getText()).toBe('Password is required.');
});
Expand All @@ -317,7 +317,7 @@ describe('Users E2E Tests:', function () {
// Sign in
browser.get('http://localhost:3001/authentication/signin');
// Enter UserName
element(by.model('vm.credentials.username')).sendKeys(user1.username);
element(by.model('vm.credentials.usernameOrEmail')).sendKeys(user1.username);
// Enter Password
element(by.model('vm.credentials.password')).sendKeys(user1.password);
// Click Submit button
Expand Down
37 changes: 28 additions & 9 deletions modules/users/tests/server/user.server.routes.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ var semver = require('semver'),
var app,
agent,
credentials,
credentialsEmail,
user,
_user,
admin;
Expand All @@ -32,9 +33,15 @@ describe('User CRUD tests', function () {
});

beforeEach(function (done) {
// Create user credentials
// Create user credentials with username
credentials = {
username: 'username',
usernameOrEmail: 'username',
password: 'M3@n.jsI$Aw3$0m3'
};

// Create user credentials with email
credentialsEmail = {
usernameOrEmail: 'test@test.com',
password: 'M3@n.jsI$Aw3$0m3'
};

Expand All @@ -44,7 +51,7 @@ describe('User CRUD tests', function () {
lastName: 'Name',
displayName: 'Full Name',
email: 'test@test.com',
username: credentials.username,
username: credentials.usernameOrEmail,
password: credentials.password,
provider: 'local'
};
Expand Down Expand Up @@ -83,7 +90,8 @@ describe('User CRUD tests', function () {
});
});

it('should be able to login successfully and logout successfully', function (done) {
it('should be able to login with username and email successfully and logout successfully', function (done) {
// Login with username
agent.post('/api/auth/signin')
.send(credentials)
.expect(200)
Expand Down Expand Up @@ -111,7 +119,18 @@ describe('User CRUD tests', function () {
signoutRes.text.should.equal('Moved Temporarily. Redirecting to /');
}

return done();
// Login with username
agent.post('/api/auth/signin')
.send(credentials)
.expect(200)
.end(function (signinErr, signinRes) {
// Handle signin error
if (signinErr) {
return done(signinErr);
}

return done();
});
});
});
});
Expand Down Expand Up @@ -711,11 +730,11 @@ describe('User CRUD tests', function () {
_user2.email = 'user2_email@test.com';

var credentials2 = {
username: 'username2',
usernameOrEmail: 'username2',
password: 'M3@n.jsI$Aw3$0m3'
};

_user2.username = credentials2.username;
_user2.username = credentials2.usernameOrEmail;
_user2.password = credentials2.password;

var user2 = new User(_user2);
Expand Down Expand Up @@ -763,11 +782,11 @@ describe('User CRUD tests', function () {
_user2.email = 'user2_email@test.com';

var credentials2 = {
username: 'username2',
usernameOrEmail: 'username2',
password: 'M3@n.jsI$Aw3$0m3'
};

_user2.username = credentials2.username;
_user2.username = credentials2.usernameOrEmail;
_user2.password = credentials2.password;

var user2 = new User(_user2);
Expand Down

7 comments on commit 6a6b630

@PierreBrisorgueil
Copy link
Contributor

Choose a reason for hiding this comment

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

@lirantal Hello, in modules/users/tests/server/user.server.routes.tests.js

should be able to login with username and email successfully and logout successfully

you make two agent.post('/api/auth/signin') with credentials, credentialsEmail is not used

a oversights if I'm not mistaken? :)

@mleanos
Copy link
Member

Choose a reason for hiding this comment

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

@PierreBrisorgueil I took a look at this, and you're correct. The second login should be using credentialsEmail.

@PierreBrisorgueil
Copy link
Contributor

Choose a reason for hiding this comment

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

@mleanos that's not more clear to make two tests in this case ?

  it('should be able to login with username  successfully and logout successfully', function (done)
  it('should be able to login with  email successfully and logout successfully', function (done)

@itelo
Copy link
Contributor Author

@itelo itelo commented on 6a6b630 Oct 18, 2016

Choose a reason for hiding this comment

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

@PierreBrisorgueil I agree with you, make two test is more clear

@mleanos
Copy link
Member

Choose a reason for hiding this comment

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

Yes. We should have two separate tests.

@lirantal
Copy link
Member

Choose a reason for hiding this comment

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

@itelo @PierreBrisorgueil could you please submit a PR with enhancements on this?

@PierreBrisorgueil
Copy link
Contributor

Choose a reason for hiding this comment

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

Please sign in to comment.