-
Notifications
You must be signed in to change notification settings - Fork 208
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
[POC] add refreshtoken to extend accessToken #537
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ import {UserProfile, securityId, SecurityBindings} from '@loopback/security'; | |
import { | ||
CredentialsRequestBody, | ||
UserProfileSchema, | ||
RefreshTokenRequestBody, | ||
} from './specs/user-controller.specs'; | ||
import {Credentials} from '../repositories/user.repository'; | ||
import {PasswordHasher} from '../services/hash.password.bcryptjs'; | ||
|
@@ -36,10 +37,12 @@ import { | |
TokenServiceBindings, | ||
PasswordHasherBindings, | ||
UserServiceBindings, | ||
RefreshtokenServiceBindings, | ||
} from '../keys'; | ||
import _ from 'lodash'; | ||
import {OPERATION_SECURITY_SPEC} from '../utils/security-spec'; | ||
import {basicAuthorization} from '../services/basic.authorizor'; | ||
import {RefreshtokenService} from '../services/refreshtoken.service'; | ||
|
||
@model() | ||
export class NewUserRequest extends User { | ||
|
@@ -59,6 +62,8 @@ export class UserController { | |
public passwordHasher: PasswordHasher, | ||
@inject(TokenServiceBindings.TOKEN_SERVICE) | ||
public jwtService: TokenService, | ||
@inject(RefreshtokenServiceBindings.REFRESHTOKEN_SERVICE) | ||
public refreshtokenService: RefreshtokenService, | ||
@inject(UserServiceBindings.USER_SERVICE) | ||
public userService: UserService<User, Credentials>, | ||
) {} | ||
|
@@ -250,7 +255,7 @@ export class UserController { | |
}) | ||
async login( | ||
@requestBody(CredentialsRequestBody) credentials: Credentials, | ||
): Promise<{token: string}> { | ||
): Promise<{token: string; refreshtoken: string}> { | ||
// ensure the user exists, and the password is correct | ||
const user = await this.userService.verifyCredentials(credentials); | ||
|
||
|
@@ -260,6 +265,52 @@ export class UserController { | |
// create a JSON Web Token based on the user profile | ||
const token = await this.jwtService.generateToken(userProfile); | ||
|
||
// create a refreshtoken | ||
const refreshtoken = await this.refreshtokenService.generateToken( | ||
userProfile, | ||
); | ||
|
||
return { | ||
token, | ||
refreshtoken, | ||
}; | ||
} | ||
|
||
@post('/users/newtoken', { | ||
security: OPERATION_SECURITY_SPEC, | ||
responses: { | ||
'200': { | ||
description: 'Refreshing the token', | ||
content: { | ||
'application/json': { | ||
schema: { | ||
type: 'object', | ||
properties: { | ||
token: { | ||
type: 'string', | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}) | ||
@authenticate('jwt') | ||
// TODO(derdeka) find out why @authorize.allowAuthenticated() is not working | ||
async refresh( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When would the refresh function get called? I am trying to illustrate the flow of how a refresh token works:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In a |
||
@inject(SecurityBindings.USER) currentUserProfile: UserProfile, | ||
@requestBody(RefreshTokenRequestBody) body: {refreshtoken: string}, | ||
): Promise<{token: string}> { | ||
// check if the provided refreshtoken is valid, throws error if invalid | ||
await this.refreshtokenService.verifyToken( | ||
body.refreshtoken, | ||
currentUserProfile, | ||
); | ||
|
||
// create a new JSON Web Token based on the user profile | ||
const token = await this.jwtService.generateToken(currentUserProfile); | ||
|
||
return {token}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
// Copyright IBM Corp. 2020. All Rights Reserved. | ||
// Node module: loopback4-example-shopping | ||
// This file is licensed under the MIT License. | ||
// License text available at https://opensource.org/licenses/MIT | ||
|
||
import {Entity, model, property} from '@loopback/repository'; | ||
|
||
@model() | ||
export class UserRefreshtoken extends Entity { | ||
@property({ | ||
type: 'string', | ||
id: true, | ||
}) | ||
id: string; | ||
|
||
@property({ | ||
type: 'number', | ||
required: true, | ||
}) | ||
ttl: number; | ||
|
||
@property({ | ||
type: 'date', | ||
}) | ||
creation: Date; | ||
|
||
@property({ | ||
type: 'string', | ||
required: true, | ||
mongodb: {dataType: 'ObjectID'}, | ||
}) | ||
userId: string; | ||
|
||
constructor(data?: Partial<UserRefreshtoken>) { | ||
super(data); | ||
} | ||
} | ||
|
||
export interface UserRefreshtokenRelations { | ||
// describe navigational properties here | ||
} | ||
|
||
export type UserRefreshtokenWithRelations = UserRefreshtoken & | ||
UserRefreshtokenRelations; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
import {Entity, model, property, hasMany, hasOne} from '@loopback/repository'; | ||
import {Order} from './order.model'; | ||
import {UserCredentials} from './user-credentials.model'; | ||
import {UserRefreshtoken} from './user-refreshtoken.model'; | ||
import {ShoppingCart} from './shopping-cart.model'; | ||
|
||
@model({ | ||
|
@@ -51,6 +52,9 @@ export class User extends Entity { | |
@hasOne(() => UserCredentials) | ||
userCredentials: UserCredentials; | ||
|
||
@hasMany(() => UserRefreshtoken) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not hasOne? Can user have multiple? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently the Alternatively we could share the WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For simplicity, sharing a single token across devices would have a single point of failure, any detected breach would log out all devices. Could be annoying to user. Your solution is more flexible/complex, but might think about identifying the authorised device linked to the refresh token maybe? Identifying devices, allows the token to be individually invalidated (i.e. Lost device can have access revoked, although any serious security will remote wipe devices). If one token is compromised, is there any practical difference in access compromised? To get updated refresh token we need to log in regardless don't we? Hmm will need to think more. Both solutions allow:
Multiple refresh token adds:
Do what you think fits the scenario best. I'm just offering thoughts for consideration. |
||
userRefreshtokens: UserRefreshtoken[]; | ||
|
||
@hasOne(() => ShoppingCart) | ||
shoppingCart: ShoppingCart; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Copyright IBM Corp. 2020. All Rights Reserved. | ||
// Node module: loopback4-example-shopping | ||
// This file is licensed under the MIT License. | ||
// License text available at https://opensource.org/licenses/MIT | ||
|
||
import {inject} from '@loopback/core'; | ||
import {DefaultCrudRepository, juggler} from '@loopback/repository'; | ||
import {UserRefreshtoken, UserRefreshtokenRelations} from '../models'; | ||
|
||
export class UserRefreshtokenRepository extends DefaultCrudRepository< | ||
UserRefreshtoken, | ||
typeof UserRefreshtoken.prototype.id, | ||
UserRefreshtokenRelations | ||
> { | ||
constructor(@inject('datasources.mongo') dataSource: juggler.DataSource) { | ||
super(UserRefreshtoken, dataSource); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
// Copyright IBM Corp. 2020. All Rights Reserved. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @derdeka , cool stuff. I was going to just increase the token lifetime from 10 minutes to 6 hours in my last PR ;). I am wondering :
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @emonddr All good questions. Above @derdeka said he'd add tests after initial feedback. I think the general idea is for the refresh token to hold the session open on a device for a period of time(say 6 hours, two days or even longer) without the need to log back in. When your access token runs out after 10 minutes, then the refresh token would be used to get new access token without the need to log in again. This would also need to be designed so that the refresh token can be revoked and force a login. Is that correct @derdeka? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'll add some tests.
On a
Good catch, i have currently no concept for this. My use case is a regular user on a
I did not have a deep look how the frontend is currently working. I guess a |
||
// Node module: @loopback/authentication | ||
// This file is licensed under the MIT License. | ||
// License text available at https://opensource.org/licenses/MIT | ||
import {TokenService} from '@loopback/authentication'; | ||
import {inject} from '@loopback/core'; | ||
import {repository} from '@loopback/repository'; | ||
import {HttpErrors} from '@loopback/rest'; | ||
import {UserProfile, securityId} from '@loopback/security'; | ||
import {UserRefreshtokenRepository} from '../repositories/user-refreshtoken.repository'; | ||
import {RefreshtokenServiceBindings} from '../keys'; | ||
|
||
export interface RefreshtokenService extends TokenService { | ||
/** | ||
* Verifies the validity of a token string and returns a user profile | ||
* | ||
* TODO(derdeka) move optional parameter userProfile to TokenService? | ||
*/ | ||
verifyToken(token: string, userProfile?: UserProfile): Promise<UserProfile>; | ||
/** | ||
* Revokes a given token (if supported by token system) | ||
*/ | ||
revokeToken(token: string, userProfile?: UserProfile): Promise<void>; | ||
} | ||
|
||
export class MyRefreshtokenService implements RefreshtokenService { | ||
constructor( | ||
@repository(UserRefreshtokenRepository) | ||
public userRefreshtokenRepository: UserRefreshtokenRepository, | ||
@inject(RefreshtokenServiceBindings.REFRESHTOKEN_ETERNAL_ALLOWED, { | ||
optional: true, | ||
}) | ||
private refreshtokenEternalAllowed: boolean = false, | ||
@inject(RefreshtokenServiceBindings.REFRESHTOKEN_EXPIRES_IN, { | ||
optional: true, | ||
}) | ||
private refreshtokenExpiresIn: number = 60 * 60 * 24, | ||
) {} | ||
|
||
async generateToken(userProfile: UserProfile): Promise<string> { | ||
// TODO(derdeka) objectId as refreshtoken is a bad idea | ||
const userRefreshtoken = await this.userRefreshtokenRepository.create({ | ||
creation: new Date(), | ||
ttl: this.refreshtokenExpiresIn, | ||
userId: userProfile[securityId], | ||
}); | ||
return userRefreshtoken.id; | ||
} | ||
|
||
async verifyToken( | ||
refreshtoken: string, | ||
userProfile?: UserProfile, | ||
): Promise<UserProfile> { | ||
try { | ||
if (!userProfile || !userProfile[securityId]) { | ||
throw new HttpErrors.Unauthorized('Invalid refreshToken'); | ||
} | ||
const {creation, ttl} = await this.userRefreshtokenRepository.findById( | ||
refreshtoken, | ||
{ | ||
where: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. usually findById should return only one result, any reason adding an additional where filter here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea is to check if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recently learned from @raymondfeng that findById does not respect the where condition. I have to do the check differently. |
||
userId: userProfile[securityId], | ||
}, | ||
}, | ||
); | ||
const isEternalToken = ttl === -1; | ||
const elapsedSeconds = (Date.now() - creation.getTime()) / 1000; | ||
const isValid = isEternalToken | ||
? this.refreshtokenEternalAllowed | ||
: elapsedSeconds < ttl; | ||
if (!isValid) { | ||
throw new HttpErrors.Unauthorized('Invalid refreshToken'); | ||
} | ||
return userProfile; | ||
} catch (e) { | ||
throw new HttpErrors.Unauthorized('Invalid refreshToken'); | ||
} | ||
} | ||
|
||
async revokeToken( | ||
refreshtoken: string, | ||
userProfile: UserProfile, | ||
): Promise<void> { | ||
try { | ||
await this.userRefreshtokenRepository.deleteById(refreshtoken, { | ||
where: { | ||
userId: userProfile[securityId], | ||
}, | ||
}); | ||
} catch (e) { | ||
// ignore | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually the jwt tokens we generate are very long. What is the reasoning for 8? Why not 1? Or why specify this at all? Just curious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emonddr If you look at the
generateToken
function inhttps://github.com/strongloop/loopback4-example-shopping/pull/537/files#diff-90f4ee64422c3d610fc44a185e6d0781R47
A refresh token is the id of a
UserRefreshToken
record, not a token generated by jwt service.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a pattern check to the token, but i'm still not sure what is the best way to generate a
refreshToken
. Currently it's aobjectId
which might not be the best practice. On amysql
/postgresql
database i would just use a UUID asrefreshToken
and use a regular expression for validation.Do you have any other ideas how to generate a revokeable token?