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

feat: add support for jwt on api #896

Merged
merged 27 commits into from Aug 21, 2018
Merged

feat: add support for jwt on api #896

merged 27 commits into from Aug 21, 2018

Conversation

juanpicado
Copy link
Member

@juanpicado juanpicado commented Aug 5, 2018

Type: feat
Description:

This PR aims to deprecate AES token generator but without replacing it and allow to use JWT as an alternative on API. (#168 (comment))

Provides customize JWT signature and verification based on
https://github.com/auth0/node-jsonwebtoken#usage

Default Security Config

security:
  api:
    jwt:
      sign:
        expiresIn: 60d
        notBefore: 1
  web:
    sign:
      expiresIn: 7d

Legacy

Legacy refers to the default authentification (#168 (comment)) used by 2.x and 3.x. If you don't define the security block, legacy is being enabled by default.

security:
  api:
    legacy: true
  web:
    sign:
      expiresIn: 7d

.npmrc

//localhost:4873/:_authToken=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoianVhbjExIiwiZ3JvdXBzIjpbImp1YW4xMSIsIiRhbGwiLCIkYXV0aGVudGljYXRlZCIsIkBhbGwiLCJAYXV0aGVudGljYXRlZCIsImFsbCJdLCJyZWFsX2dyb3VwcyI6WyJqdWFuMTEiXSwiZ3JvdXAiOlsianVhbjExIl0sImlhdCI6MTUzNDAwOTkzOSwibmJmIjoxNTM0MDA5OTM5LCJleHAiOjE1MzQ2MTQ3Mzl9.FPQ__wb-CIT2Gnti0fMyBw6XK20Zv8xuuZBqVVaCr7s

Resolves #168 #729

🆕 enable JWT by default on API.

https://verdaccio.org/blog/2019/04/19/diving-into-jwt-support-for-verdaccio-4

@juanpicado juanpicado added the WIP label Aug 5, 2018
@juanpicado juanpicado added this to the 4.0.0 milestone Aug 5, 2018
@juanpicado juanpicado changed the base branch from master to 4.x August 5, 2018 13:19
add multiple scenarios with configuration file
@@ -15,7 +15,7 @@ const spliceURL = require('../../utils/string').spliceURL;
module.exports = function(config, auth, storage) {
Search.configureStorage(storage);

router.use(auth.webUIJWTmiddleware());
// router.use(auth.webUIJWTmiddleware());
Copy link
Member Author

Choose a reason for hiding this comment

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

reminder: this is odd

@juanpicado juanpicado added this to In Progress in Verdaccio 4 Aug 15, 2018
@juanpicado juanpicado mentioned this pull request Aug 15, 2018

return credentials;
} else {
return;
Copy link
Member

Choose a reason for hiding this comment

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

No need of return

Copy link
Member

Choose a reason for hiding this comment

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

may be you want it more readable..

Copy link
Member

@ayusharma ayusharma left a comment

Choose a reason for hiding this comment

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

Can you replace err with error. I'll run it locally later and give one more review.

return jwt.sign(payload, secretOrPrivateKey, {
notBefore: '1000', // Make sure the time will not rollback :)
...options,
}, (err, token) => {
Copy link
Member

Choose a reason for hiding this comment

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

arrow ===> function expression

@@ -76,7 +76,7 @@ export default class VerdaccioProcess implements IServerProcess {
});

this.childFork.on('error', (err) => {
console.log('error process', err);
// console.log('error process', err);
Copy link
Member

Choose a reason for hiding this comment

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

can we remove it ?

});
} else {
// i am wiling to use here _.isNil but flow does not like it yet.
if (typeof security.api.jwt !== 'undefined' &&
Copy link
Member

Choose a reason for hiding this comment

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

Destructure...

const {jwt} = security.api;

export function createSessionToken(): CookieSessionToken {
return {
// npmjs.org sets 10h expire
expires: new Date(Date.now() + 10 * 60 * 60 * 1000),
Copy link
Member

Choose a reason for hiding this comment

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

can we have this as a constant somewhere?

}

export function getAuthenticatedMessage(user: string): string {
return `you are authenticated as '${user}'`;
Copy link
Member

Choose a reason for hiding this comment

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

Just check ' this in $user

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's intended. Don't ask me why :-) ... it was there already and unless I know what it does i cannot remove it.

}

export function isAESLegacy(security: Security): boolean {
return _.isNil(security.api.legacy) === false &&
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: destructures..


return credentials;
} else {
return;
Copy link
Member

Choose a reason for hiding this comment

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

may be you want it more readable..

if (_.isString(token) && scheme.toUpperCase() === TOKEN_BEARER.toUpperCase()) {
return verifyJWTPayload(token, secret);
} else {
return;
Copy link
Member

Choose a reason for hiding this comment

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

same here.. return

Copy link
Member Author

Choose a reason for hiding this comment

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

it was mostly for debugging. Removed

Copy link
Contributor

@priscilawebdev priscilawebdev left a comment

Choose a reason for hiding this comment

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

Great! Looks fine 👍

@juanpicado juanpicado merged commit a68d247 into 4.x Aug 21, 2018
Verdaccio 4 automation moved this from In Progress to Done Aug 21, 2018
@delete-merged-branch delete-merged-branch bot deleted the feature-168 branch August 21, 2018 06:05
@juanpicado
Copy link
Member Author

Thanks @priscilawebdev @ayusharma for CR, this is a nice feature.

priscilawebdev pushed a commit that referenced this pull request Sep 16, 2018
* feat: add support for jwt on api

* test: add unit test for sign token with jwt

add multiple scenarios with configuration file

* chore: add JWT verification on middleware

* chore: restore headless

* chore: restore middleware header validation

* refactor: fix login whether user exists

* refactor: JWT is signed asynchronously

* refactor: better structure and new naming convention

* test: add unit test for token signature

* test: add unit test for creating user with JWT enabled

#168

* docs: add security section jwt

* refactor: renable  web auth middleware

* test(auth): add legacy disabled scenario

* chore: update gitignore

* chore: add some es6 sugar

* feat: enable JWT token signature for new installations

* chore: add yaml files to git

I forgot add this before 😷

* chore: trace log on auth

in case we want more output
priscilawebdev pushed a commit that referenced this pull request Sep 19, 2018
* feat: add support for jwt on api

* test: add unit test for sign token with jwt

add multiple scenarios with configuration file

* chore: add JWT verification on middleware

* chore: restore headless

* chore: restore middleware header validation

* refactor: fix login whether user exists

* refactor: JWT is signed asynchronously

* refactor: better structure and new naming convention

* test: add unit test for token signature

* test: add unit test for creating user with JWT enabled

#168

* docs: add security section jwt

* refactor: renable  web auth middleware

* test(auth): add legacy disabled scenario

* chore: update gitignore

* chore: add some es6 sugar

* feat: enable JWT token signature for new installations

* chore: add yaml files to git

I forgot add this before 😷

* chore: trace log on auth

in case we want more output
@lock
Copy link

lock bot commented Jan 8, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Verdaccio 4
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants