[DNCR-107] Add token refreshing #31

megawebmaster · 2016-10-16T14:54:48Z

This PR also resolves issues when token is not valid and the backend rejects it - app clears the token and redirects to login page.

tlegutko

Good job, what's most imporant is that token refreshing seems to work (after expiration token nicely refreshed and I had no trouble using app).

I have some code-style comments, mostly around auth-service class, other are super-minor.

tlegutko · 2016-10-17T08:55:47Z

frontend/src/app/homepage/login/login.component.ts

+          if (response.hasOwnProperty('error')) {
+            this.error = response.error;
+          } else {
+            this.error = 'Nieprawidłowy login lub hasło.';


isn't 'Nieprawidłowy login lub hasło.' just the case when auth is failed? Let's look at method at backend

try { if(!$token = \JWTAuth::attempt($credentials)) { return response()->json(['error' => \Lang::get('auth.failed')], 401); } } catch(JWTException $e) { return response()->json(['error' => \Lang::get('auth.could_not_create_token')], 500); } return response()->json(['token' => $token]); (...) 'failed' => 'Nieprawidłowy login lub hasło.',

So if it reaches this else condition if it means it's error not related to JWT and message should be something like Wystąpił nieoczekiwany błąd w komunikacji z serwerem.

tlegutko · 2016-10-17T09:04:02Z

frontend/src/app/homepage/homepage.component.ts

-        }
-      }
-    );
+    if (this.service.isLoggedIn()) {


I don't think it's a priority for now, but I was wondering if moving this logic to some guard executed in canActivate would prevent homepage from blinking before redirect takes place. I think it should, If I'm not logged I'm cleanly redirected to homepage, so it should work the same the other way around.

Yeah, it would work, but I will try to find more clever way ;)

tlegutko · 2016-10-17T09:16:30Z

frontend/src/app/course/courses.service.ts

-  courseCreated = this.courseCreatedSource.asObservable();
-  calendarItemsCreated = this.courseCreated.map(this.courseToCalendarItems);
-  recentlyClickedTime = this.recentlyClickedTimeSource.asObservable();
+  courseCreated: Observable<Course> = this.courseCreatedSource.asObservable();


you can leave it, but any particular reason for a change? I thought TS infers the type during typed declaration.

Well, I got some weird errors so I added it - does not cost us a thing and shows what are these types explicitly.

tlegutko · 2016-10-17T09:35:02Z

frontend/src/app/_commons/auth/auth.service.ts

-        this.router.navigate(['/']);
+  public isLoggedIn(): boolean {
+    try {
+      if (!tokenNotExpired()){


hm, I need some explanation on this one. Why not just

public isLoggedIn(): boolean { return tokenNotExpired();

?

router navigation in this method returning boolean is definitely unexpected. especially that i.e. in AuthGuard there is similar redirection once this method returns and I think it's preferable approach to navigate in component calling this method.

what kind of exception do we expect? In code samples in tutorial for angular2-jwt they just return tokenNotExpired() (https://github.com/auth0-samples/auth0-angularjs2-systemjs-sample/search?p=1&q=tokenNotExpired&utf8=%E2%9C%93)

But I might be missing something.

There is a lot of places where we use or will use isLoggedIn(). Moreover there is also this requirement for URL check (otherwise app hangs on homepage). But maybe you are right and I should move it up.

tlegutko · 2016-10-17T10:13:22Z

frontend/src/app/_commons/auth/auth.service.ts

+      return;
+    }
+
+    let helper = new JwtHelper();


hm, took me a while, but I finally understood it. Why not just call this.jwtHelper.getTokenExpirationDate(token) and use Date / Moment for date operations? Getting barely documented exp field from token and then manual operations on milliseconds with some unit conversion along the way seems error-prone. I looked into end result and yes it looks like you didn't make a mistake, but cleaner code will allow us to modify it without fear in the future. And those time-unit operations could be a separate method taking token as argument (whole scheduleTokenRefreshing() could actually take token as parameter)

Because we need milliseconds for setTimeout() and Date operations in JS are horrible. The exp field is clearly described in JWT RFC so I don't think we need to explain it more, do you? :)

oh cmon, date -> moment

var day = new Date(2011, 9, 16); var dayWrapper = moment(day);

and moment seems to have nice manipulation operations. imo that'd make the code more readable, but I trust your judgement, it's up to you

And please, tell me how this would be more readable:

let expiration = moment(helper.getTokenExpirationDate(token)); let now = moment(); let difference = expiration.diff(now); let timeout = difference - 60000; // Subtract 1 minute to be sure token is still valid

imo sticking to moment / duration for all calculations makes it a bit better:

let expiration = moment(new JwtHelper().getTokenExpirationDate(token)); let timeout = moment.duration(expiration.diff(moment())) .subtract(1, 'minute') // to be sure token is still valid .asMilliseconds();

tlegutko · 2016-10-17T10:18:22Z

frontend/src/app/_commons/auth/auth.service.ts


 @Injectable()
 export class AuthService {
+  private static TOKEN = 'id_token';
+  private static refreshTimeout: any;


it's a number according to https://nodejs.org/docs/v0.6.1/api/timers.html#setTimeout and I think it shouldn't be static - it's related to lifecycle of this service.

token nad known user could be readonly (https://basarat.gitbooks.io/typescript/content/docs/types/readonly.html I recently learned TS2 introduced constants, nice) and known user should be private

It's not NodeJS :) I won't be so sure where it doesn't hurt us for it to be any. Please also see where it is cleared and as we have (and always will) have a single instance of AuthService. These statics are required to avoid dependency loop (AuthHttp => AuthService => AuthHttp).

Oh, I got confused because our PhpStorms says it returns a NodeJS.Timer when I checked. And it seems to be a common mistake mgechev/angular-seed#901. So I guess we're using https://developer.mozilla.org/en-US/docs/Web/API/WindowTimers/setTimeout and as in linked issue, if we do
refreshTimeout**Id** = window.setTimeout(), then it returns number, so we can add proper type.

We generally use types, so let's stick to it - if this field has Id in it's name and has number type the code will be more clear.

And I was unaware of this circular dependency, now that you mention it.. I think Angular should provide some nice way to make service a singleton (apart from defining it top-level in app and having it in all namespaces), but I didn't find a better way, so until I do I'm ok with static.. :P

Well, why should we call it an ID as it does not matter to us at all, it's even internal thing of AuthService. As for circular dependency there is no way of resolving it without manual factories with setters.

Well you have a point, it's just passed somewhere so it's not super important.

On the other hand in another place in this class, during calculations we name something timeout and it's a duration in milliseconds. So if it's something different (id of timer) and held as a class field and not just local variable then different name (and proper type, just to be consistent, especially when I got the wrong idea inspecting it because of some import / dependency bug) would improve code quality and readability.

But I doubt this class will be modified super-often and it is kind of internal, so if you still disagree I won't push any more, I won't argue about it any more here, we both have better things to do :)

tlegutko · 2016-10-17T10:20:51Z

app/Http/Controllers/Auth/AuthController.php

+      $token = \JWTAuth::refresh();
+    }
+    catch(JWTException $e)
+    {
      return response()->json(['error' => \Lang::get('auth.could_not_create_token')], 500);


Message to user is correct for both scenarios, but maybe error could be auth.token_error? seeing create in error with refresh made me go and check error msgs. if you think it's not important you can leave it as it is

This is message also used by JWT library, I don't want to change it in any way.

tlegutko · 2016-10-17T10:26:16Z

frontend/src/app/_commons/auth/http.ts

+          // TODO: Add notification about invalid response - logout
+          AuthService.clear();
+        }
+        if (response.status === 500) {


this! till now we didn't log some 500 errors and this'll be helpful. and it's done just in one place, brilliant :).

This commit also contains some minor changes to how being logged in is checked.

tlegutko requested changes Oct 17, 2016

View reviewed changes

megawebmaster force-pushed the DNCR-107 branch from ed396c7 to 4a9d9af Compare October 26, 2016 08:16

megawebmaster added 3 commits October 31, 2016 13:33

[DNCR-107] Add refreshing token before it's timeout

280aa33

[DNCR-107] Improve authentication error handling

41164ed

[DNCR-107] Improve homepage guarding and redirection

e73004b

This commit also contains some minor changes to how being logged in is checked.

tlegutko force-pushed the DNCR-107 branch from 4a9d9af to e73004b Compare October 31, 2016 12:34

tlegutko approved these changes Oct 31, 2016

View reviewed changes

tlegutko merged commit bc2c996 into develop Oct 31, 2016

tlegutko deleted the DNCR-107 branch October 31, 2016 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DNCR-107] Add token refreshing #31

[DNCR-107] Add token refreshing #31

megawebmaster commented Oct 16, 2016

tlegutko left a comment

tlegutko Oct 17, 2016

megawebmaster Oct 17, 2016

tlegutko Oct 17, 2016

megawebmaster Oct 17, 2016

tlegutko Oct 17, 2016

megawebmaster Oct 17, 2016

tlegutko Oct 17, 2016

megawebmaster Oct 17, 2016

tlegutko Oct 17, 2016

megawebmaster Oct 17, 2016

tlegutko Oct 17, 2016

megawebmaster Oct 18, 2016

tlegutko Oct 19, 2016

tlegutko Oct 17, 2016

megawebmaster Oct 17, 2016

tlegutko Oct 17, 2016

tlegutko Oct 17, 2016

megawebmaster Oct 18, 2016

tlegutko Oct 19, 2016

tlegutko Oct 17, 2016

megawebmaster Oct 17, 2016

tlegutko Oct 17, 2016

[DNCR-107] Add token refreshing #31

[DNCR-107] Add token refreshing #31

Conversation

megawebmaster commented Oct 16, 2016

tlegutko 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

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

Choose a reason for hiding this comment