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

[WIP] Handle session expiration gracefully #3492

Closed
wants to merge 1 commit into from
Closed

[WIP] Handle session expiration gracefully #3492

wants to merge 1 commit into from

Conversation

marcoow
Copy link
Contributor

@marcoow marcoow commented Jul 31, 2014

This will eventually fix #2092, however it's currently missing correct handling of some edge cases, tests etc. Whenever a 401 response is detected (which most likely means that the session is expired), a modal signin window is opened that lets the user sign in again to retry the action that triggered the 401 response.

That, together with Ember Simple Auth's automatic token refresh should resolve all problems with expired sessions and lost data.

Missing:

  • tests
  • when 401 is triggered by a transition the modal is opened before the redirect to the signing page which looks ugly
  • the modal itself needs to be styled

return user;
}, function() {
self.invalidate();
});

This comment was marked as abuse.

This comment was marked as abuse.

@ErisDS
Copy link
Member

ErisDS commented Aug 20, 2014

Hi @marcoow, are you planning on working on this any further? I'm really excited to get this into core :)

We can style the modal as a follow up task, so is there much else left to do?

@marcoow
Copy link
Contributor Author

marcoow commented Aug 21, 2014

I'd like wot finish this but didn't find any time at all recently - hopefully next week.

The problem currently is that the current solution does actually fix that one case but also introduces some other problems. For example it will always open the modal also if you are already on an error page where it really doesn't make sense. I'm not sure yet how a better approach would look.

Besides that some tests are the only thing that's missing (given the styling of the modal in done in a follow up issue).

@marcoow
Copy link
Contributor Author

marcoow commented Aug 26, 2014

I dropped the global approach where every invocation of authorizationFailed would open the signin modal and explicitly handle that case only for the editor.edit route now which seems to work quite well. The only 2 things that are missing are

  • tests
  • design for the signin modal

I definitely can't do the design and it'd also be nice if someone could step in and add a test as I don't have a lot of time currently - functionality should be good though.

@jaswilli
Copy link
Contributor

jaswilli commented Sep 9, 2014

So I pulled this PR down and gave it a run through but I don't ever see the sign-in modal popup when authorization fails, I only get the "are you sure you want to leave?" modal dialog.

@marcoow
Copy link
Contributor Author

marcoow commented Sep 9, 2014

You should see it when you're on the editor page, change sth., delete the token from your db and click save.

@jaswilli
Copy link
Contributor

jaswilli commented Sep 9, 2014

That's what I'm doing and I can't seem to make it happen.

Sign in -> edit existing post -> delete token -> click save.

Then I get the "are you sure you want to leave?" modal, and if I hit "leave" I get the 401 and redirected to sign in. If I hit "stay" I just get dumped back to the editor.

Maybe someone else can test it and see if it works for them.

@marcoow
Copy link
Contributor Author

marcoow commented Sep 9, 2014

Hm, you're right - not sure what's wrong - I'll check

@marcoow
Copy link
Contributor Author

marcoow commented Sep 9, 2014

...and now it works again for me - without (consciously) having changed anything...

@jaswilli
Copy link
Contributor

jaswilli commented Sep 9, 2014

I was having the exact same problems when I was working on #3873, which is why I eventually just put the authorizationFailed action handler on the Application route. It's the only place I could get it to fire reliably.

@marcoow
Copy link
Contributor Author

marcoow commented Sep 9, 2014

Very strange - guess we should find out what's going on though.

@ErisDS
Copy link
Member

ErisDS commented Sep 10, 2014

I was having exactly the same problem, sorry I should have commented but I assumed I was doing something wrong!

@ErisDS
Copy link
Member

ErisDS commented Sep 18, 2014

Any news?

@marcoow
Copy link
Contributor Author

marcoow commented Sep 20, 2014

I don't really understand why the action is sometimes triggered on the application route and sometimes (which I think is correct) on editor.edit. Maybe someone from the Ember team could help here... Not sure but I'd say this might as well be a bug in Ember itself. @rwjblue you seem to have been involved with Ghost a bit already?

Steps to reproduce for the working case:

  1. go to edit page for an entry (editor.edit route)
  2. reload the page
  3. delete the access token from the DB
  4. edit and save the entry -> authorizationFailed is triggered on editor.edit route (login modal shows)

Failing case:

  1. edit an entry, don't save
  2. delete the access token from the DB
  3. save the entry -> authorizationFailed is triggered on application route while it should be on editor.edit ("do you want to leave this page" shows)

@jaswilli
Copy link
Contributor

This should be unblocked when mainmatter/ember-simple-auth#367 lands.

@ErisDS
Copy link
Member

ErisDS commented Dec 1, 2014

I saw that mainmatter/ember-simple-auth#367 landed - would be awesome to get this in!

@marcoow
Copy link
Contributor Author

marcoow commented Dec 3, 2014

Ember Simple Auth 0.7.2 including the above fix was released earlier today.

@ErisDS
Copy link
Member

ErisDS commented Dec 7, 2014

Does this mean that if this PR is rebased it'll be good to merge?

@jaswilli
Copy link
Contributor

jaswilli commented Dec 7, 2014

I have it rebased and working locally. I can do the usual and open a new PR while preserving this commit so that the UX and other details can be worked out.

@ErisDS
Copy link
Member

ErisDS commented Dec 7, 2014

@jaswilli that would be awesome

@jaswilli
Copy link
Contributor

jaswilli commented Dec 8, 2014

I've moved this commit into #4602 so that we can iron out the details and merge all at once.

@jaswilli jaswilli closed this Dec 8, 2014
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.

Handle session expiry gracefully
4 participants