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

refactor: refactor to utilize latest @loopback/authentication module #120

Merged
merged 1 commit into from May 17, 2019

Conversation

Projects
None yet
5 participants
@emonddr
Copy link
Contributor

commented May 10, 2019

Refactor this example shopping cart application to utilize the latest
authentication strategy interface, action provider, and
strategy provider from @loopback/authentication

associated with : #79

@emonddr

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Waiting for code from strongloop/loopback-next#2312 to land in a new release of @loopback/authentication 2.0 .

@emonddr emonddr force-pushed the dremond_79_refactor branch from bd97c64 to 3b8d7dd May 10, 2019

@emonddr emonddr force-pushed the dremond_79_refactor branch from dd117ac to f43ca10 May 14, 2019

@emonddr emonddr marked this pull request as ready for review May 14, 2019

@emonddr emonddr force-pushed the dremond_79_refactor branch 4 times, most recently from 5112095 to 2da4c52 May 14, 2019

@emonddr emonddr requested review from jannyHou, raymondfeng and b-admike May 15, 2019

@emonddr emonddr self-assigned this May 15, 2019

@emonddr

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

This PR is based on @loopback/authentication 2.0.3 , which was released yesterday by @b-admike (thanks Biniam!)

@b-admike
Copy link
Member

left a comment

Looks great. I've left some comments/nitpicks.

//
// The strategy resolver throws a non-http error if it cannot
// resolve the strategy. When the strategy resolver obtains
// a strategy, it calls strategy.authentication(request) which

This comment has been minimized.

Copy link
@b-admike

b-admike May 15, 2019

Member

Typo: it calls strategy.authentication(request) -> it calls strategy.authenticate(request)

Show resolved Hide resolved packages/shopping/src/sequence.ts Outdated
);
}

import {PasswordHasher} from './services/hash.password.bcryptjs';

This comment has been minimized.

Copy link
@b-admike

b-admike May 15, 2019

Member

Let's move this import statement to the top of the file please.

This comment has been minimized.

Copy link
@emonddr

emonddr May 16, 2019

Author Contributor

@b-admike , why did me editor do that? weird. Good catch.

Show resolved Hide resolved ...shopping/src/__tests__/acceptance/shopping-cart.controller.acceptance.ts
return {id: user.id, name: userName};
}

async validateCredentials(credentials: Credentials) {

This comment has been minimized.

Copy link
@b-admike

b-admike May 15, 2019

Member

We don't have any async operations in this method, so we can remove the async keyword. You'd also have to change up the tests which expect it to behave in this manner.

const error = new JsonWebTokenError('jwt malformed');
return expect(jwtService.decodeAccessToken(token)).to.be.rejectedWith(
error,
it('user service convertToUserProfile() succeeds without optional fields : firstname, surname', async () => {

This comment has been minimized.

Copy link
@b-admike

b-admike May 15, 2019

Member

convertToUserProfile is not an async function, so let's remove the async keyword from the it block here.

@emonddr emonddr force-pushed the dremond_79_refactor branch from 2da4c52 to 4fdc4b2 May 15, 2019

"@loopback/build": "1.5.3",
"@loopback/testlab": "1.2.8",
"@loopback/build": "1.5.4",
"@loopback/testlab": "1.2.9",

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng May 15, 2019

Member

I wonder why the dependencies are declared with fixed versions instead of ^ range.

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 15, 2019

Contributor

I think it's because we don't have the shopping example as downstream test in loopback-next, so if new release breaks the example we won't know.

This comment has been minimized.

Copy link
@b-admike

b-admike May 15, 2019

Member

It's because of how we configured renovate-bot (see #93 (comment) and #104).

This comment has been minimized.

Copy link
@emonddr

emonddr May 15, 2019

Author Contributor

@raymondfeng , @b-admike mentioned to me that renovate-bot goes into package.json and removes the ^ . So I just changed the versions to the latest.

This comment has been minimized.

Copy link
@emonddr

emonddr May 15, 2019

Author Contributor

@b-admike did I paraphrase what you said properly?

This comment has been minimized.

Copy link
@b-admike

b-admike May 16, 2019

Member

Yes @emonddr, it'll remove them if we add the ^ back because of our settings.

@@ -3,8 +3,7 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {Client, expect, toJSON} from '@loopback/testlab';
import {Response} from 'superagent';
import {Client, expect /*, toJSON*/} from '@loopback/testlab';

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng May 15, 2019

Member

If toJSON is not used, we should just remove it.

This comment has been minimized.

Copy link
@emonddr

emonddr May 15, 2019

Author Contributor

@raymondfeng , good catch. Just a silly comment I meant to leave temporarily while I was refactoring.


const token = res.body.token;
expect(token).to.not.be.Null();
expect(token).to.be.String();

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng May 15, 2019

Member

This assertion covers expect(token).to.not.be.Null();

expect(token).to.not.be.empty();
const parts = token.split('.');
//token has 3 parts separated by '.'
expect(parts.length).to.equal(3);

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng May 15, 2019

Member

If we want to assert the format of the token, can we use a regular expression instead?

This comment has been minimized.

Copy link
@emonddr

emonddr May 15, 2019

Author Contributor

If we want to assert the format of the token, can we use a regular expression instead?

As per our conversation offline, I will remove the test for 3 parts of the jwt token string, since this is tested later on when a jwt token is created and then used to go to /users/me endpoint.


it('users/me returns an error when an expired token is provided', async () => {
const expiredToken =
'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpZCI6IjEiLCJuYW1lIjoiRXhhbXBsZSBVc2VyIiwiaWF0IjoxNTU3NTE1MTg1LCJleHAiOjE1NTc1MTUyNDV9.NZ31amMztvIYBth3SRY9fxiZTukObpgV2gSmJZ10pwY';

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng May 15, 2019

Member

Can we use a helper method to generate an expired token?

this.bind(PasswordHasherBindings.ROUNDS).to(10);
this.bind(PasswordHasherBindings.PASSWORD_HASHER).toClass(BcryptHasher);

this.bind(UserServiceBindings.USER_SERVICE).toClass(MyUserService);

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng May 15, 2019

Member

Maybe extract all bindings into a method?

const token = res.body.token;
expect(token).to.not.be.Null();
expect(token).to.be.String();
expect(token).to.not.be.empty();

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 15, 2019

Contributor

The token should always be a string I think, it's enforced by

});

it('returns product recommendations for a user', async () => {
const newUser = await userRepo.create(user);
it('login returns an error when invalid email is used', async () => {
await client

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 15, 2019

Contributor

making http request is slower, please create the user using const newUser = await userRepo.create(user); to save time :)

This comment has been minimized.

Copy link
@emonddr

emonddr May 16, 2019

Author Contributor

@jannyHou

making http request is slower, please create the user using const newUser = await userRepo.create(user); to save time :)

Hmm, I don't agree. The purpose is to test the REST application and its endpoints, not test a repository.
The server needs to start anyways. I don't see the benefit or point of by-passing POST /users.

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 16, 2019

Contributor

The purpose is to test the REST application and its endpoints, not test a repository.

Ah true, but this test is for verifying endpoint Users/login, therefore it doesn't care about how the user is created, by calling repository API or sending HTTP request, by "making http request is slower" I mean how the user is created.
Not a big deal though, I will leave it to you to decide how to create the user.

This comment has been minimized.

Copy link
@b-admike

b-admike May 16, 2019

Member

I see @jannyHou's point. IIUC, we want to create the user using the less expensive repository operation, then we can use the our http client to make the request to /users/login which is what the test aims for. To see this in action, I'd run this test in isolation with both ways and see how much time it took (might have to change the spec to checkmarks instead of dots).

This comment has been minimized.

Copy link
@emonddr

emonddr May 16, 2019

Author Contributor

@jannyHou , @b-admike explained to me the specific tests you wanted me to avoid using POST /users. I understand now. I thought you wanted me to avoid POST /users for every test.

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 17, 2019

Contributor

@b-admike Thank you for the explanation!

I thought you wanted me to avoid POST /users for every test.

Sorry I meant only those "specific tests", glad you guys figure it out, no misunderstanding now 👏

@jannyHou
Copy link
Contributor

left a comment

@emonddr Great job! I left some minor comments. The refactored services and the sequence LGTM.

});

it('login returns a valid token', async () => {
const newUser = await userRepo.create(user);
it('login returns an error when invalid password is used', async () => {
await client

This comment has been minimized.

Copy link
@emonddr

emonddr May 16, 2019

Author Contributor

@jannyHou , please see my response .

password: plainPassword,
});
it('users/me returns the current user profile when a valid token is provided', async () => {
let res = await client

This comment has been minimized.

Copy link
@emonddr

emonddr May 16, 2019

Author Contributor

@jannyHou , please see my response .

const credentials = {email: 'dom@example.com', password: 'p4ssw0rd'};
return expect(
userService.validateCredentials(credentials),
).to.be.fulfilled();

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 15, 2019

Contributor

I think expect(()=>userService.validateCredentials).to.not.throw() is better.

You may want to make sure there is no error thrown by userService.validateCredentials(credentials)

This comment has been minimized.

Copy link
@emonddr

emonddr May 15, 2019

Author Contributor

@jannyHou I had to change this expect statement to the one you are mentioning to address the issue biniam raised. :)

const userProfile = userService.convertToUserProfile(newUser);
const token = await jwtService.generateToken(userProfile);
expect(token).not.Null();
expect(token).to.be.String();

This comment has been minimized.

it('password encrypter hashPassword() succeeds', async () => {
const encrypedPassword = await bcryptHasher.hashPassword(user.password);
expect(encrypedPassword).to.not.be.Null();
expect(encrypedPassword).to.be.String();

This comment has been minimized.


constructor(
@inject(TokenServiceBindings.TOKEN_SERVICE)
public tokenService: JWTService,

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 15, 2019

Contributor

I think the type here should be TokenService

This comment has been minimized.

Copy link
@emonddr

emonddr May 16, 2019

Author Contributor

@jannyHou , It can be either, but I will change it to the interface instead of the implementation.

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 16, 2019

Contributor

@emonddr As we discussed earlier, feel free to ignore the comment about changing the interface :)

This comment has been minimized.

Copy link
@emonddr

emonddr May 16, 2019

Author Contributor

@jannyHou , I decided to go with the interfaces as you suggested.

@emonddr emonddr force-pushed the dremond_79_refactor branch from d0a3de9 to bc73e01 May 16, 2019

@b-admike
Copy link
Member

left a comment

Nice 🎉. Changes LGTM once @jannyHou and @raymondfeng's comments are addressed.

@nabdelgadir
Copy link
Contributor

left a comment

I have some minor comments, but ditto @b-admike 💯

Show resolved Hide resolved packages/shopping/src/__tests__/acceptance/user.controller.acceptance.ts Outdated
Show resolved Hide resolved packages/shopping/src/authentication-strategies/jwt-strategy.ts Outdated
Show resolved Hide resolved packages/shopping/src/sequence.ts Outdated
Show resolved Hide resolved packages/shopping/src/sequence.ts
convertToUserProfile(user: User): UserProfile {
// since first name and surname are optional, no error is thrown if not provided
let userName = '';
if (user.firstname && user.surname) {

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir May 16, 2019

Contributor

Question: are they allowed to provide only firstname or surname or does it have to be both to be used as the name?

This comment has been minimized.

Copy link
@emonddr

emonddr May 16, 2019

Author Contributor

@nabdelgadir . Just a personal decision at the time of writing. But you raise a good point.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng May 16, 2019

Member

I would prefer firstName and lastName.

This comment has been minimized.

Copy link
@emonddr

emonddr May 17, 2019

Author Contributor

@raymondfeng , I have addressed this in my latest commit

@@ -27,3 +27,7 @@ export async function setupApplication(): Promise<AppWithClient> {

return {app, client};
}

export function getExpiredToken(): string {
return 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpZCI6IjEiLCJuYW1lIjoiRXhhbXBsZSBVc2VyIiwiaWF0IjoxNTU3NTE1MTg1LCJleHAiOjE1NTc1MTUyNDV9.NZ31amMztvIYBth3SRY9fxiZTukObpgV2gSmJZ10pwY';

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng May 17, 2019

Member

Is there a way to use jsonwebtoken api to create an expired token? If not, at least document how it was created at the first place with the corresponding json value.

This comment has been minimized.

Copy link
@emonddr

emonddr May 17, 2019

Author Contributor

@raymondfeng , using the utility, I created a token that expires in 5 minutes and printed out its value in console.log. Then, every other time I use it, it is always expired.

packages/shopping/src/services/jwt-service.ts is used to create the jwt token:

  async generateToken(userProfile: UserProfile): Promise<string> {
    if (!userProfile) {
      throw new HttpErrors.Unauthorized(
        'Error generating token : userProfile is null',
      );
    }

    // Generate a JSON Web Token
    let token: string;
    try {
      token = await signAsync(userProfile, this.jwtSecret, {
        expiresIn: Number(this.jwtExpiresIn),
      });
    } catch (error) {
      throw new HttpErrors.Unauthorized(`Error encoding token : ${error}`);
    }

    return token;
  }

The import statement:

const signAsync = promisify(jwt.sign);

This comment has been minimized.

Copy link
@emonddr

emonddr May 17, 2019

Author Contributor

@raymondfeng , as per our conversation:

packages/shopping/src/tests/acceptance/user.controller.acceptance.ts now contains :
givenAnExpiredToken()

});

it('user service convertToUserProfile() succeeds without optional fields : firstName, lastName', () => {
const userWithoutFirstOrSurname = Object.assign({}, newUser);

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng May 17, 2019

Member

Use lastName for the var too.

This comment has been minimized.

Copy link
@emonddr

emonddr May 17, 2019

Author Contributor

Oops, nice catch! :)
@raymondfeng , I amended the latest commit.

@emonddr emonddr referenced this pull request May 17, 2019

Merged

refactor: fix typo in authentication package #2913

2 of 7 tasks complete
@jannyHou
Copy link
Contributor

left a comment

🚢 Great, LGTM! (after addressing Raymond's comments squashing commits into one)

@emonddr emonddr force-pushed the dremond_79_refactor branch 2 times, most recently from a45843e to a19bcb6 May 17, 2019

refactor: refactor to utilize latest @loopback/authentication module
Refactor this example shopping cart application to utilize the latest
authentication strategy interface, action provider, and
strategy provider from @loopback/authentication

Signed-off-by: Dominique Emond <dremond@ca.ibm.com>

@emonddr emonddr force-pushed the dremond_79_refactor branch from a19bcb6 to 72bb763 May 17, 2019

@emonddr

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

squashed commits

* Specifying a negative value for 'expiresIn' so the
* token is automatically expired
*/
async function givenAnExpiredToken() {

This comment has been minimized.

Copy link
@raymondfeng
@raymondfeng
Copy link
Member

left a comment

Great job!

@emonddr emonddr merged commit 2d8978d into master May 17, 2019

3 checks passed

DCO DCO
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.