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

Token manager Implemented #222

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Token manager Implemented #222

wants to merge 9 commits into from

Conversation

Aetherall
Copy link
Member

Here is the PR to implement the token manager with the rest of the suite

I had to open a new one because jest broke when I tried to merge with master

@codecov-io
Copy link

codecov-io commented Apr 3, 2018

Codecov Report

Merging #222 into master will decrease coverage by 1.28%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #222      +/-   ##
========================================
- Coverage   95.29%    94%   -1.29%     
========================================
  Files          55     56       +1     
  Lines        1274   1334      +60     
  Branches      188    180       -8     
========================================
+ Hits         1214   1254      +40     
- Misses         55     74      +19     
- Partials        5      6       +1
Impacted Files Coverage Δ
packages/server/src/index.ts 100% <ø> (ø) ⬆️
packages/server/src/accounts-server.ts 90.96% <100%> (+1.01%) ⬆️
packages/password/src/accounts-password.ts 93.37% <100%> (+2.41%) ⬆️
packages/token-manager/src/token-manager.ts 100% <100%> (ø)
packages/token-manager/src/index.ts 100% <100%> (ø)
packages/server/src/utils/get-first-user-email.ts 33.33% <0%> (-66.67%) ⬇️
packages/client/src/accounts-client.ts 88% <0%> (-10.69%) ⬇️
packages/server/src/utils/email.ts 81.81% <0%> (-3.9%) ⬇️
packages/rest-express/src/user-loader.ts 100% <0%> (ø) ⬆️
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11bb4f9...7542144. Read the comment docs.

@Aetherall Aetherall mentioned this pull request Apr 3, 2018
12 tasks
@@ -44,6 +32,7 @@ const defaultOptions = {

export class AccountsServer {
public options: AccountsServerOptions;
public tokenManager: any;
Copy link
Member

Choose a reason for hiding this comment

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

if you can add real typings there would be great

Copy link
Member Author

Choose a reason for hiding this comment

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

sure ! Should be done now


private validateConfiguration(config: Configuration): void {
if (!config) {
throw new Error('[ Accounts - TokenManager ] configuration : A configuration object is needed');
Copy link
Member

Choose a reason for hiding this comment

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

can you use the error module?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure ! Should be done now

@pradel
Copy link
Member

pradel commented Apr 3, 2018

Can you add an example about how do you instantiate the server class with the token manager?

@Aetherall
Copy link
Member Author

import TokenManager from '@accounts/token-manager'

const tokenManager = new TokenManager({
	secret: 'secret',
	emailTokenExpiration: 1000 * 60 * 60 * 2,
	access: {
		expiresIn: '1h'
	},
	refresh: {
		algorithm: 'HS256'
	}
})

new AccountsServer({
	db,
	tokenManager
}, {
	password: new Password(passwordConfig)
})

@pradel
Copy link
Member

pradel commented Apr 3, 2018

So the user will have to install a new package manually, can't we just make the initialisation internal?

@Aetherall
Copy link
Member Author

I think we should keep the packages simple and make the user-friendly in the preset packages ? The relations between packages would become too complicated in my opinion

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

Successfully merging this pull request may close these issues.

None yet

3 participants