Feat/auth improvement : refresh-token are now saved if not repeated by the backend refresh endpoint #593

alain-charles · 2018-07-31T07:15:16Z

Feature/Auth : improvement on refreshable tokens

Short description of what this resolves:

At log on, some backends send Oauth2 token with a refresh-token value.
But some of them do no repeat the refresh token value when you then hit the refresh token endpoint.
So up to now, we were losing the refresh-token so that we were unable to send more than once a refresh token request

This PR corrects this problem: if necessary , it gets the existing refresh-token value in the current token and injects it in the refreshed token before storing it.

Hope it helps.

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.

Tokens can now be asked for their creation date via createdAt attribute or getCreatedAt() NbAuthOAuth2Token.getTokenExpDate() now returns a correct expDate (Issue #564) Unit tests updated nnixaa first review nnixaa second review nnixaa third review (am i so bad ? :-p)

A new class of token has been defined. NbAuthOAuth2JWTToken : class to use when your auth backend sends Oauth tokens encapsulating a jwt access token.

# Conflicts: # src/framework/auth/services/token/token.ts Optimized token.ts code

Bug Refresh token value is now reinjected in the token before storage, in case the backend did not repeat it.

…icRefreshToken

nnixaa · 2018-07-31T10:00:57Z

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

@@ -16,11 +16,19 @@ export abstract class NbAuthToken {

 export interface NbAuthRefreshableToken {
  getRefreshToken(): string;
+  // Needed when refresh-token response does not repeat refresh_token value


@alain-charles so it is possible that after refreshing the token server may not return a new refresh token? Just looked through the RFC and this part looks vague.

In any case, I would propose to move this functionality into the refreshToken() method of the strategy. In this case, we have the current token and can extract the refresh part right there with no need for all additional checks.

What do you think?

I think that you are a amazing code architect :)

reverted token, token-service and corresponding spec.ts files

codecov · 2018-07-31T13:15:52Z

Codecov Report

Merging #593 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
+ Coverage    72.8%   72.85%   +0.04%     
==========================================
  Files         153      153              
  Lines        4207     4214       +7     
  Branches      317      318       +1     
==========================================
+ Hits         3063     3070       +7     
  Misses       1087     1087              
  Partials       57       57

Impacted Files	Coverage Δ
src/framework/auth/services/token/token.ts	`95.23% <100%> (+0.09%)`	⬆️
src/framework/auth/services/auth.service.ts	`89.58% <100%> (ø)`	⬆️
src/framework/auth/auth.options.ts	`100% <100%> (ø)`	⬆️
...ramework/auth/strategies/oauth2/oauth2-strategy.ts	`87.4% <100%> (+0.51%)`	⬆️
src/framework/auth/strategies/auth-strategy.ts	`95.23% <100%> (ø)`	⬆️

nnixaa

@alain-charles one last change

nnixaa · 2018-08-01T09:02:25Z

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

@@ -199,13 +199,17 @@ export class NbOAuth2AuthStrategy extends NbAuthStrategy {
    return this.http.post(url, this.buildRefreshRequestData(token), this.buildAuthHeader())
      .pipe(
        map((res) => {
+          const refreshedToken = <NbAuthOAuth2Token>this.createToken(res);


let's just move this into a new method:

createRefreshedToken(res): NbAuthRefreshableToken { ... }

and then call it almost as before:

return new NbAuthResult( true, res, this.getOption('redirect.success'), [], this.getOption('defaultMessages'), this.createRefreshedToken(res));

@nnixaa
I think you mean

createRefreshedToken(res, existingToken): NbAuthOAuth2Token { ... }

since we need the existing token for extracting the existing refresh_token

And NbAuthOAuth2Token instead of NbAuthRefreshableToken so that we return something assignable to NbAuthResult.token i.e NbAuthToken

@alain-charles yes, of course, you are right.
But in the second part - return NbAuthOAuth2Token - is this always true? as by design we can assign any token to any strategy, so it is not necessary NbAuthOAuth2Token, but rather NbAuthToken.

@nnixaa here is the best implementation i have found

protected createRefreshedToken(res, existingToken: NbAuthRefreshableToken): NbAuthToken { const refreshedToken = this.createToken(res); if (isNbAuthRefreshableToken(refreshedToken) && !refreshedToken.getRefreshToken() && existingToken.getRefreshToken()) { refreshedToken.setRefreshToken(existingToken.getRefreshToken()); } return refreshedToken; }

i think this is the one that is the most in the spirit of your architecture

authService : corrected the strategy.refreshToken() call : we were forgettong to pass data as argument ?! token : added isNbAuthRefreshableToken(token) method

nnixaa · 2018-08-01T12:44:00Z

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

@@ -376,6 +377,16 @@ export class NbOAuth2AuthStrategy extends NbAuthStrategy {
    }, {}) : {};
  }

+  protected createRefreshedToken(res, existingToken: NbAuthRefreshableToken): NbAuthToken {


I believe we can leave this check as:

if (!refreshedToken.getRefreshToken() && existingToken.getRefreshToken()) {

as we can only have the NbAuthRefreshableToken token here, which presumes that both required get and set method exist because this method could only be called from the refreshToken one, which cannot be called for a non-refreshable token.
In case when the developer forces a call of refreshToken method for a non-refreshble token it will be a typescript level error, which is completely fine in this case.

!refreshedToken.getRefreshToken() does not compile as NbAuthToken (the return type) does not implement this method.
That is the reason why i added the isNbAuthRefreshableToken(refreshedToken) test

So what do you prefer :

should we return NbAuthOauth2Token ?

or just cast in the body of the method the refreshedToken to NbAuth0Auth2Token ?

or leave the code like in #10d0b31 commit ?

@alain-charles can we do this:

protected createRefreshedToken(res, existingToken: NbAuthToken & NbAuthRefreshableToken)

Sorry not able to check it myself.
If we can't, let's leave it as your current version.

@alain-charles sorry for the delay, wrote the comment last Thursday but forgot to submit.

@nnixaa with the intersection type you propose above, the problem remains on refreshedToken

const refreshedToken = this.createToken(res);

this.createToken(res) returns NbAuthToken that does not implement getRefreshToken().

Any other idea ?

Alain

@alain-charles something like this probably? https://github.com/alain-charles/nebular/pull/1/files

…freshToken refactor(token): generic type for `createToken` method

nnixaa · 2018-08-07T12:50:54Z

@alain-charles cool!

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 and others added 11 commits July 26, 2018 08:11

Merge branch 'master' into master

ee706d3

feature/auth

5eb5821

Tokens can now be asked for their creation date via createdAt attribute or getCreatedAt() NbAuthOAuth2Token.getTokenExpDate() now returns a correct expDate (Issue #564) Unit tests updated nnixaa first review nnixaa second review nnixaa third review (am i so bad ? :-p)

Merge remote-tracking branch 'origin/master'

1a08225

feature/auth

1d697bc

A new class of token has been defined. NbAuthOAuth2JWTToken : class to use when your auth backend sends Oauth tokens encapsulating a jwt access token.

Merge remote-tracking branch 'upstream/master'

1c25956

# Conflicts: # src/framework/auth/services/token/token.ts Optimized token.ts code

Merge remote-tracking branch 'upstream/master'

ff0e8d3

# Conflicts: # src/framework/auth/services/token/token.ts Optimized token.ts code

Optimized getAccessTokenPayload() calls

e7af99e

Optimized getAccessTokenPayload() calls

95632b1

Merge branch 'master' into master

bc4001f

Feat/auth

e06a45c

Bug Refresh token value is now reinjected in the token before storage, in case the backend did not repeat it.

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

7c6fc43

…icRefreshToken

alain-charles changed the title ~~Feat/auth automatic refresh token~~ Feat/auth improvement : refresh-token are now kept if not repeated by the backent refres endpoint Jul 31, 2018

alain-charles changed the title ~~Feat/auth improvement : refresh-token are now kept if not repeated by the backent refres endpoint~~ Feat/auth improvement : refresh-token are now kept if not repeated by the backend refresh endpoint Jul 31, 2018

alain-charles changed the title ~~Feat/auth improvement : refresh-token are now kept if not repeated by the backend refresh endpoint~~ Feat/auth improvement : refresh-token are now saved if not repeated by the backend refresh endpoint Jul 31, 2018

nnixaa requested changes Jul 31, 2018

View reviewed changes

alain-charles added 2 commits July 31, 2018 15:11

Moved code from tokenservice to strategy.

88c7106

reverted token, token-service and corresponding spec.ts files

Added setRefreshToken(token)

76dda02

nnixaa requested changes Aug 1, 2018

View reviewed changes

oAuth2Strategy : Move refreshToken creation into independant method

10d0b31

authService : corrected the strategy.refreshToken() call : we were forgettong to pass data as argument ?! token : added isNbAuthRefreshableToken(token) method

nnixaa reviewed Aug 1, 2018

View reviewed changes

nnixaa and others added 5 commits August 6, 2018 17:49

Merge branch 'master' into feat/auth-AutomaticRefreshToken

f6ba089

Merge branch 'master' into feat/auth-AutomaticRefreshToken

c88b96e

refactor(token): generic type for createToken method

b3eb264

Merge pull request #1 from nnixaa/alain-charles-feat/auth-AutomaticRe…

a491f21

…freshToken refactor(token): generic type for `createToken` method

added parameter type = NbAuthToken in exported const NB_AUTH_TOKENS.

0ae51f6

nnixaa approved these changes Aug 7, 2018

View reviewed changes

nnixaa merged commit dffbd59 into akveo:master Aug 7, 2018

alain-charles deleted the feat/auth-AutomaticRefreshToken branch August 14, 2018 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/auth improvement : refresh-token are now saved if not repeated by the backend refresh endpoint #593

Feat/auth improvement : refresh-token are now saved if not repeated by the backend refresh endpoint #593

alain-charles commented Jul 31, 2018 •

edited

Loading

nnixaa Jul 31, 2018

alain-charles Jul 31, 2018

codecov bot commented Jul 31, 2018 •

edited

Loading

nnixaa left a comment

nnixaa Aug 1, 2018

alain-charles Aug 1, 2018

nnixaa Aug 1, 2018

alain-charles Aug 1, 2018

nnixaa Aug 1, 2018

alain-charles Aug 1, 2018

nnixaa Aug 1, 2018

nnixaa Aug 6, 2018

alain-charles Aug 7, 2018

nnixaa Aug 7, 2018

nnixaa commented Aug 7, 2018

Feat/auth improvement : refresh-token are now saved if not repeated by the backend refresh endpoint #593

Feat/auth improvement : refresh-token are now saved if not repeated by the backend refresh endpoint #593

Conversation

alain-charles commented Jul 31, 2018 • edited Loading

Short description of what this resolves:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 31, 2018 • edited Loading

Codecov Report

nnixaa left a 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

Choose a reason for hiding this comment

nnixaa commented Aug 7, 2018

alain-charles commented Jul 31, 2018 •

edited

Loading

codecov bot commented Jul 31, 2018 •

edited

Loading