Feat/auth : Rejects malformed JWTToken and requireValidToken Option #597

alain-charles · 2018-08-02T13:35:27Z

Short description

The framework now :

rejects every malformed JWT tokens (i.e empty, not presenting 3 parts, not base64 encoded) whatever the value of requireValidToken,
proposes a new requireValidToken at the endpoint level that is set to false by default,
keeps on accepting to authenticate/register/refresh/token if the backend does not return any token, but only if requireValidToken is set to false (rejects otherwise)
rejects every !isValid() token if requireValidToken is set to true,

important

requireValidToken option is set to false by default so that there is no breaking changes.
failWhenNoTokenhas been removed from password strategy as it was still not released and becoming redundant

issue resolved

Resolves the #517 issue

Now : If existing, NbAuthResult contains backend error description other Changes requested by Dmitry (first review)

Now : If existing, NbAuthResult contains backend error description other Changes requested by Dmitry (first review) +tslint missing trailing comma arghhh

…erted in the doc

…hService

…strategy used to create the token (future use)

The token now contains ownerStrategyName, with is a back link to the strategy used to create the token (future use). BREAKING CHANGE : NbAuthCreateToken (token.ts) now takes a third parameter, which is the ownerStrategyName Tokens are created by strategies that are called from services, so it is *potentially* a breaking change.

The token now contains ownerStrategyName, with is a back link to the strategy used to create the token (future use). updated unit tests files and oauth2-password-login.component (breaking change below) BREAKING CHANGE : NbAuthCreateToken (token.ts) now takes a third parameter, which is the ownerStrategyName Tokens are created by strategies that are called from services, so it is *potentially* a breaking change.

removed useless code and cleaned one unit test file BREAKING CHANGE : NbAuthCreateToken (token.ts) now takes a third parameter, which is the ownerStrategyName Tokens are created by strategies that are called from services, so it is *potentially* a breaking change.

…ValidToken

The framework now accepts empty jwt token (it is the NbPasswordAuthStrategy failWhenNoToken oprion) but it rejects every non empty malformed JWT tokens A new option requireValidToken has been added for accepting or rejecting inValid (!isValid) tokend. Option set to false by default so no breaking changes cleaned code Resolves the #517 issue

…ValidToken

…eValidtoken is now present for all strategies. Factorized code in password strategy for error response handling Updated unit tests No breaking change since : - requireValidToken is set to false by default - failWhenNoToken was not released.

…ValidToken # Conflicts: # src/framework/auth/strategies/auth-strategy.ts

alain-charles · 2018-08-07T13:47:22Z

@nnixaa the last commit resolves merging conflict and implements failWhenNoToken removal.

codecov · 2018-08-07T15:41:55Z

Codecov Report

Merging #597 into master will decrease coverage by 0.22%.
The diff coverage is 83.08%.

@@            Coverage Diff             @@
##           master     #597      +/-   ##
==========================================
- Coverage   72.71%   72.49%   -0.23%     
==========================================
  Files         153      153              
  Lines        4222     4264      +42     
  Branches      323      330       +7     
==========================================
+ Hits         3070     3091      +21     
- Misses       1095     1109      +14     
- Partials       57       64       +7

Impacted Files	Coverage Δ
...rk/auth/strategies/dummy/dummy-strategy-options.ts	`100% <ø> (ø)`	⬆️
...h/strategies/password/password-strategy-options.ts	`100% <ø> (ø)`	⬆️
.../framework/auth/strategies/dummy/dummy-strategy.ts	`61.29% <0%> (-6.57%)`	⬇️
src/framework/auth/strategies/auth-strategy.ts	`95.83% <100%> (+0.59%)`	⬆️
.../auth/strategies/oauth2/oauth2-strategy.options.ts	`100% <100%> (ø)`	⬆️
...work/auth/strategies/password/password-strategy.ts	`95.65% <100%> (-0.58%)`	⬇️
...ramework/auth/strategies/oauth2/oauth2-strategy.ts	`83.8% <61.9%> (-3.6%)`	⬇️
src/framework/auth/services/token/token.ts	`88.48% <81.35%> (-6.75%)`	⬇️

nnixaa · 2018-08-07T15:45:02Z

src/framework/auth/services/token/token.ts

+export class InvalidJWTTokenError extends Error {
+  constructor(message: string) {
+    super(message);
+    const actualPrototype = new.target.prototype;


no need in additional variable here, just Object.setPrototypeOf(this, new.target.prototype);

nnixaa · 2018-08-07T15:45:33Z

src/framework/auth/services/token/token.ts

@@ -14,6 +14,14 @@ export abstract class NbAuthToken {
  }
 }

+export class InvalidJWTTokenError extends Error {


InvalidJWTTokenError -> NbAuthInvalidJWTTokenError would be better

nnixaa · 2018-08-07T15:46:52Z

src/framework/auth/services/token/token.ts

+    Object.setPrototypeOf(this, actualPrototype);
+  }
+}
+


we also need another error then, as we cannot use InvalidJWTTokenError everywhere, since not every token is JWT token.
probably like this
NbAuthInvalidTokenError
NbAuthInvalidJWTTokenError

nnixaa · 2018-08-07T15:50:07Z

src/framework/auth/services/token/token.ts

@@ -221,7 +237,7 @@ export class NbAuthOAuth2Token extends NbAuthSimpleToken {
   */
  getPayload(): any {
    if (!this.token || !Object.keys(this.token).length) {
-      throw new Error('Cannot extract payload from an empty token.');
+      throw new InvalidJWTTokenError('Cannot extract payload from an empty token.');


this isn't a JWT token error I suppose as we are in NbAuthOAuth2Token

nnixaa · 2018-08-07T15:50:39Z

src/framework/auth/strategies/auth-strategy.ts

+    const requireValidToken: boolean = this.getOption('requireValidToken');
+    const token = nbAuthCreateToken<T>(this.getOption('token.class'), value, this.getName());
+    if (requireValidToken && !token.isValid()) {
+      throw new InvalidJWTTokenError('Token is empty or invalid.');


same here, we can only throw a more generic error here

nnixaa · 2018-08-07T15:51:03Z

src/framework/auth/strategies/dummy/dummy-strategy-options.ts

@@ -7,7 +7,6 @@ import { NbAuthStrategyOptions } from '../auth-strategy-options';
 import { NbAuthSimpleToken } from '../../services/';

 export class NbDummyAuthStrategyOptions extends NbAuthStrategyOptions {
-  name: string;


why do we need to remove this name?

@nnixaa
As for the moment requireValidToken was set at strategy level, i put it in the ancestor NbAuthStrategyOption.
I then realized that only NbPasswordStrategyOption was extending NbAuthStrategyOption.
I guess it would be better (even if requireValidToken finally goes at endpoint level) to make OAuth2 and Dummy options also extend NbAuthStrategyOption.

In this case name (and probably more attributes) can be declared in NbAuthStrategyOption.

What do you think ?

@alain-charles I'm bit confused, as far as I can see, all strategies' options (oauth2, dummy, password) are extended from NbAuthStrategyOptions. Am I missing something?

@nnixaa Have a look at NbOAuth2AuthStrategyOptions in the master branch

nnixaa · 2018-08-07T15:53:30Z

src/playground/auth/auth.module.ts

@@ -74,7 +74,7 @@ import { NbAuthGuard } from './auth-guard.service';

        NbPasswordAuthStrategy.setup({
          name: 'email',
-
+          requireValidToken: false,


I would say we need to introduce this not on a strategy level, but on an endpoint level.
For instance, in a case when login endpoint requires to have a valid token, but register endpoint doesn't.

nnixaa · 2018-08-07T15:53:51Z

src/playground/oauth2-password/oauth2-password.module.ts

@@ -16,13 +16,11 @@ import {
 } from '@nebular/theme';

 import {
-  NbAuthModule,
+  NbAuthModule, NbAuthOAuth2JWTToken,


I guess this change should not be a part of this PR?

alain-charles · 2018-08-08T07:13:20Z

@nnixaa i'm still working on this.
Issue #517 i said

If the jwt token is malformed, i don't think we shoud accept it and store it, because it is really useless. we won't be able to use it for authenticating against backend.

If the token is malformed, maybe it is a misconfiguration of the strategy, maybe a backend problem.
I propose that instead of crashing, we send NbAuthResult with success=false, and the error description "invalid jwt token".

Do you still agree with that, i.e. even if requireValidToken is set to false on the endpoint, we reject the token if it is malformed ?

I still think we shoud reject malformed JWT tokens, whatever the value of requireValidToken. For me requireValidToken must be used to stop the process when we got no token at all or when it is expired.

What do you think ?

nnixaa · 2018-08-08T09:34:57Z

@alain-charles yes, let's do this way. But then we need to parse the token at the creation time I guess, not at the getPayload time?

alain-charles · 2018-08-08T11:05:27Z

@nnixaa it is already the case in what i've written and i'll push
JWT token payload is parsed at creation time, the parsing is launched by prepareCreatetAt called in every constructor.
=> We have to fail if token id returned by backend but is invalid,
=> and we have to create the token if no token is given because of requireValidToken than can be false

nnixaa · 2018-08-08T16:02:02Z

@alain-charles yes, I agree, but since we parse it here already, moreover we have to parse it (to validate and fail) there is no need to parse it all over again on each getPayload() call. It would be better to parse/validate it in the constructor once and then use the parsed value in the consequential calls. What do you think?

alain-charles · 2018-08-08T16:38:07Z

@nnixaa concretely you wanna store the decoded payload as a token attribute ?

nnixaa · 2018-08-08T20:09:01Z

@alain-charles yes, something like this:

   constructor(protected readonly token: any,
               protected readonly ownerStrategyName: string,
               protected createdAt?: Date) {
     super();
	 this.payload = this.parsePayload();
     this.createdAt = this.prepareCreatedAt(createdAt);
   }

and then the getPayload() method is just a getter:

   getPayload(): string {
     return this.payload;
   }

Does it make sense?

if you want present and valid token, use requireValidToken = true at the endpoint level if jwt malformed, token are rejected.

…ValidToken

…o feat/auth_requireValidToken

alain-charles · 2018-08-09T13:52:16Z

@nnixaa finally i suuceeded in pushing something that makes sense.
It was quite hard to have something working well in every case, but i think now we are (almost?) good.

nnixaa · 2018-08-09T15:25:00Z

@alain-charles awesome, checking

nnixaa · 2018-08-09T15:34:31Z

src/framework/auth/strategies/password/password-strategy.ts

+    let errors = [];
+    if (res instanceof HttpErrorResponse) {
+      errors = this.getOption('errors.getter')(module, res, this.options);
+    } else if (res instanceof NbAuthIllegalTokenError) {


so in case when the token is empty any of our token classes will throw the NbAuthTokenNotFoundError exception, right? Which won't be caught by this if and will go to something went wrong part?

In this case the statement It MAY be created even if backend did not return any token, in this case, it is !Valid is not valid and empty token will never be created.
If my reasoning is correct, then we just need to decide, what empty means - malformed or invalid.

@nnixaa here is what i have coded (or what i think i've coded 🤣 ) :

NbAuthTokenNotFoundError is thrown if we did not find the token key in the response payload (i.e. no access_token key in OAuth2 token response, no token key in simple token response...). That is what i call an empty token.
=> the error is not catched during token construction, because we want to allow some endpoints to create such tokens if requireValidToken is false.
=> But after token construction, if requireValidToken is set to true, then the isValid() will return false so that the createToken() method will throw an NbInvalidTokenError saying the token is empty or not valid. NbInvalidTokenError will then be catched by the strategy that will display the error message.

if (failWhenInvalidToken && !token.isValid()) { // If we require a valid token (i.e. isValid), then we MUST throw NbAuthIllegalTokenError so that the strategies // intercept it throw new NbAuthIllegalTokenError('Token is empty or invalid.'); }

If we find something in the token key in the response payload (i.e access_token is present, token is present), then i consider that
1/ we have a token (even if it is '' or {})
2/ the token we have must respect some rules (JWT format for instance) and if not we throw NbIllegalTokenError that will be catched in every case by the strategies.

To conclude

empty means the key was not found in the response.

And emptytokens are never valid (isValid()) but can be created if requireValidToken is set to false

If the key is found, the token is not empty even if its value is''or {} and then it must pass the "wellFormed" control otherwise it is declared Illegal (NbIllegalTokenError) and rejected.

Hope i am clear.

I think we can decide that '' or {} are empty tokens....

Ah okay, so we catch the empty token error during this check, right?

try { this.parsePayload(); } catch (err) { if (err instanceof NbAuthIllegalTokenError) { // token is present but illegal (empty or malformed), we reject it throw err; } }

so if it is illegal - throw next, and if it's just invalid (empty) - do nothing, right?

Exactly!
If empty the constructor does not throw any error .
Then the createToken() method (that launched the constructor) :

do nothing if requireValidToken is false so that we have an empty token created, stored and the login/register/refresh succeeds

throw NbIllegalTokenError (we could create NbRequireValidTokenError) if requireValidToken is true so that stratégies can return the message in NBAuthResult and login/refresh/register fail with the message

Got it, this is cool, but I'd suggest we inverse this check so that we don't block any other possible exception. So we specifically block the NbAuthTokenNotFoundError and don't touche other expectations including NbAuthIllegalTokenError:

try { this.parsePayload(); } catch (err) { if (!(err instanceof NbAuthTokenNotFoundError)) { throw err; } }

Or, another option is to not throw the NbAuthTokenNotFoundError expectation at all, just create the token.

Yes I agree .
I’ m gonna push very soon

@nnixaa What do you think now and what else can we do to be ready ?

…ValidToken

…o feat/auth_requireValidToken

alain-charles and others added 30 commits June 29, 2018 08:33

Added password grant_type option

cef7a1d

Added password grant_type option implementation

9f0dbab

Playground sample for oAuth2 password grant-type

44d7d2e

Added route to oAuth2 password grant-type sample

f57b737

Addes angular-jwt for decoding jwt token (used in playground only)

ed4628e

Added /api/auth/token endpoint (oAuth2 password grant-type playground)

d7f01df

Patched code for passing ci

af52e88

code optimization for passing ci

554f7dc

Merge branch 'master' into master

fa0548f

Changes request by Dmitry (first review)

b72adae

Generalized code in oauth2-strategy

733efc0

Now : If existing, NbAuthResult contains backend error description other Changes requested by Dmitry (first review)

Merge remote-tracking branch 'origin/master'

57fab69

Generalized code in oauth2-strategy

c50df3a

Now : If existing, NbAuthResult contains backend error description other Changes requested by Dmitry (first review) +tslint missing trailing comma arghhh

Added unit tests for oAuth-strategy password authentication

dda5436

Merge branch 'master' into master

ef0be25

Merge branch 'master' into master

e180dc3

Cleaned code according to nnixaa second review

6aa1fdb

Merge branch 'master' of https://github.com/alain-charles/nebular

96c7344

Removed package-lock.json from git repo

cf82c42

package-lock.json to revert

dcff930

reverted package-lock.json

315662f

Added new grant_type 'PASSWORD' in the block comment for it to be ins…

57a0230

…erted in the doc

Merge remote-tracking branch 'upstream/master'

d69d633

Add the refreshtoken request management in NbJwtInterceptor and NbAut…

1be8d91

…hService

Merge branch 'master' of https://github.com/alain-charles/nebular

4b67825

Merge branch 'master' into temp

deea3e2

The token now contains ownerStrategyName, with is a back link to the …

acd0241

…strategy used to create the token (future use)

feature/auth:

bd564b3

removed useless code and cleaned one unit test file BREAKING CHANGE : NbAuthCreateToken (token.ts) now takes a third parameter, which is the ownerStrategyName Tokens are created by strategies that are called from services, so it is *potentially* a breaking change.

alain-charles added 6 commits August 2, 2018 14:53

Merge remote-tracking branch 'upstream/master' into feat/auth_require…

cfe6b87

…ValidToken

Merge remote-tracking branch 'upstream/master' into feat/auth_require…

66cf701

…ValidToken

Corrected import paths

432b2cc

Merge remote-tracking branch 'upstream/master' into feat/auth_require…

01e2c2f

…ValidToken # Conflicts: # src/framework/auth/strategies/auth-strategy.ts

Merge branch 'master' into feat/auth_requireValidToken

a22d3ef

nnixaa requested changes Aug 7, 2018

View reviewed changes

Merge branch 'master' into feat/auth_requireValidToken

ffef510

alain-charles added 3 commits August 9, 2018 15:37

Payload(s) are now parsed during token construction

03e1fae

if you want present and valid token, use requireValidToken = true at the endpoint level if jwt malformed, token are rejected.

Merge remote-tracking branch 'upstream/master' into feat/auth_require…

cb1bfed

…ValidToken

Merge remote-tracking branch 'origin/feat/auth_requireValidToken' int…

b6a9db6

…o feat/auth_requireValidToken

nnixaa reviewed Aug 9, 2018

View reviewed changes

alain-charles and others added 4 commits August 10, 2018 19:55

Inverted error test in parsePayload

4b57287

Merge branch 'master' into feat/auth_requireValidToken

27d74a7

Merge remote-tracking branch 'upstream/master' into feat/auth_require…

9a8d41f

…ValidToken

Merge remote-tracking branch 'origin/feat/auth_requireValidToken' int…

ae46531

…o feat/auth_requireValidToken

nnixaa approved these changes Aug 14, 2018

View reviewed changes

nnixaa merged commit 127b5d2 into akveo:master Aug 14, 2018

alain-charles deleted the feat/auth_requireValidToken branch August 26, 2018 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/auth : Rejects malformed JWTToken and requireValidToken Option #597

Feat/auth : Rejects malformed JWTToken and requireValidToken Option #597

alain-charles commented Aug 2, 2018 •

edited

Loading

alain-charles commented Aug 7, 2018

codecov bot commented Aug 7, 2018 •

edited

Loading

nnixaa Aug 7, 2018

nnixaa Aug 7, 2018

nnixaa Aug 7, 2018

nnixaa Aug 7, 2018

nnixaa Aug 7, 2018

nnixaa Aug 7, 2018

alain-charles Aug 7, 2018

nnixaa Aug 8, 2018

alain-charles Aug 8, 2018

nnixaa Aug 7, 2018

nnixaa Aug 7, 2018

alain-charles commented Aug 8, 2018

nnixaa commented Aug 8, 2018

alain-charles commented Aug 8, 2018 •

edited

Loading

nnixaa commented Aug 8, 2018

alain-charles commented Aug 8, 2018

nnixaa commented Aug 8, 2018

alain-charles commented Aug 9, 2018

nnixaa commented Aug 9, 2018

nnixaa Aug 9, 2018

alain-charles Aug 9, 2018

alain-charles Aug 9, 2018

nnixaa Aug 10, 2018

alain-charles Aug 10, 2018 •

edited

Loading

nnixaa Aug 10, 2018

alain-charles Aug 10, 2018

alain-charles Aug 14, 2018

Feat/auth : Rejects malformed JWTToken and requireValidToken Option #597

Feat/auth : Rejects malformed JWTToken and requireValidToken Option #597

Conversation

alain-charles commented Aug 2, 2018 • edited Loading

Short description

important

issue resolved

alain-charles commented Aug 7, 2018

codecov bot commented Aug 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alain-charles commented Aug 8, 2018

nnixaa commented Aug 8, 2018

alain-charles commented Aug 8, 2018 • edited Loading

nnixaa commented Aug 8, 2018

alain-charles commented Aug 8, 2018

nnixaa commented Aug 8, 2018

alain-charles commented Aug 9, 2018

nnixaa commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alain-charles Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alain-charles commented Aug 2, 2018 •

edited

Loading

codecov bot commented Aug 7, 2018 •

edited

Loading

alain-charles commented Aug 8, 2018 •

edited

Loading

alain-charles Aug 10, 2018 •

edited

Loading