Expire sessions #10

mftaff · 2018-01-14T13:39:54Z

Relevant to issue #8

codecov-io · 2018-01-14T13:41:10Z

Codecov Report

Merging #10 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage    95.8%   95.88%   +0.07%     
==========================================
  Files          32       32              
  Lines         262      267       +5     
==========================================
+ Hits          251      256       +5     
  Misses         11       11

Impacted Files	Coverage Δ
test/models/passwordless/session_test.rb	`100% <ø> (ø)`	⬆️
app/models/passwordless/session.rb	`100% <100%> (ø)`	⬆️
...pp/controllers/passwordless/sessions_controller.rb	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51c1993...5873005. Read the comment docs.

mftaff · 2018-01-14T13:45:14Z

So all this would do is block sign in if the session is expired. I wasn't sure how to handle the message part. (Your session has expired, please sign in again.). Do you use flash messaging?

mikker

This is a good start! I left some notes – please ask questions if something doesn't make sense 😄

mikker · 2018-01-14T19:45:41Z

app/controllers/passwordless/sessions_controller.rb

@@ -96,5 +96,9 @@ def find_session
 token: params[:token]
 )
 end
+
+ def session_expired(session)


I think we should move the responsibility of knowing whether the session is expired to the session itself? Let's move this to a method Session#expired?

mikker · 2018-01-14T19:50:25Z

app/controllers/passwordless/sessions_controller.rb

@@ -42,7 +42,7 @@ def show
 BCrypt::Password.create(params[:token])

 session = find_session
- sign_in session.authenticatable
+ sign_in session.authenticatable unless session_expired session


I don't think we want to continue this method if the session is expired. We could either a) do a regular old (boring) if statement and return early or b) create an Error class, raise that and rescue it at the bottom.

mikker · 2018-01-14T19:53:39Z

app/controllers/passwordless/sessions_controller.rb

@@ -42,7 +42,7 @@ def show
 BCrypt::Password.create(params[:token])

 session = find_session
- sign_in session.authenticatable
+ sign_in session.authenticatable unless session_expired session

 redirect_enabled = Passwordless.redirect_back_after_sign_in


We don't want to redirect back if the authentication wasn't successful. In that case the user is going to be turned away again and again in an infinite loop.

Instead we would probably want to always redirect to root_path and set a flash[:error] message about the session being expired.

mftaff · 2018-01-15T10:16:49Z

Thanks for the feedback! How does it look now? (I am more than happy to make infinite changes 😄)

mikker

Getting there!

mikker · 2018-01-15T10:21:48Z

app/controllers/passwordless/sessions_controller.rb

- else
- redirect_to main_app.root_path
- end
+ redirect_to(redirect_enabled && target ? target : main_app.root_path)


I like the old way better :)

I did too, I did that to try and satisfy rubocop saying the show method was too long... Should I ignore rubocop? or is there a better solution that I missed?

Feel free to add ignores to rubocop checks – they're only there for guidance 👍
The method length one is called Metrics/MethodLength

# rubocop:disable Metrics/MethodLength

and then # rubocop:enable ... after the method

mikker · 2018-01-15T10:22:42Z

app/controllers/passwordless/sessions_controller.rb

- end
+ redirect_to(redirect_enabled && target ? target : main_app.root_path)
+ rescue ExpiredSessionError
+ session_expired


I'd prefer to have the handling logic here instead of its own method.

mikker · 2018-01-15T10:28:24Z

lib/passwordless/exceptions.rb

+ # Abstract passwordless errors from StandardError.
+ class PasswordlessError < StandardError; end
+ # Exception raised when a session is expired.
+ class ExpiredSessionError < PasswordlessError; end


While this is not a bad idea, I think I'd prefer to just have the error be a subclass to the controller. Like:

module Passwordless class SessionsController < ApplicationController class ExpiredSessionError < StandardError; end # ...

As this is our only error so far and the controller is the only place it'll be raised, I don't think we need to centralize the errors just yet :)

mikker · 2018-01-15T10:30:15Z

app/controllers/passwordless/sessions_controller.rb

@@ -96,5 +96,10 @@ def find_session
 token: params[:token]
 )
 end
+
+ def session_expired
+ flash[:error] = 'Your session has expired, please sign in again.'


I think we found our next issue: Adding I18n locale support 😄

mikker · 2018-01-15T10:30:37Z

test/models/passwordless/session_test.rb

@@ -16,7 +16,7 @@ def create_session(attrs = {})

 test 'scope: valid' do
 valid = create_session
- _expired = create_session expires_at: 1.hour.ago
+ # _expired = create_session expires_at: 1.hour.ago


You can delete this 👍

mikker · 2018-01-15T12:01:45Z

app/controllers/passwordless/sessions_controller.rb

@@ -5,6 +5,8 @@
 module Passwordless
 # Controller for managing Passwordless sessions
 class SessionsController < ApplicationController
+ # Raise this exception when a session is expired.
+ class ExpiredSessionError < StandardError; end


Add a new line under this and then we’re good to go!

mikker · 2018-01-15T12:02:25Z

app/controllers/passwordless/sessions_controller.rb

@@ -37,11 +39,13 @@ def create
 # or _root_path_
 # @see ControllerHelpers#sign_in
 # @see ControllerHelpers#save_passwordless_redirect_location!
- def show
+ def show # rubocop:disable Metrics/MethodLength, Metrics/AbcSize


Oh, and move the comment above the method (or above the docs if necessary)

mikker · 2018-01-15T12:53:13Z

This is good thank you!
💙💚💛💜❤️

mikker · 2018-01-15T12:54:00Z

Oh, I forgot: Can you squash these commits into a single one? Then I'll happily merge it 😄

mftaff · 2018-01-15T13:20:25Z

Uh oh, it looks like I just did something wrong... Something to do with rebasing? Do you know how to fix this?

mikker · 2018-01-15T13:48:16Z

😀 Not sure but you are right that it looks weird.

Be sure that a) your master is up to date with my master then b) git rebase -i master this branch on it and fixup all your commits into one.

Sorry, can't do much when I don't have it in front of me.

mftaff · 2018-01-15T14:12:51Z

Wow thanks I followed those commands exactly and it did the trick

mikker · 2018-01-15T15:40:38Z

You can even do it on those two Merge mikker/master commits :)

mftaff · 2018-01-15T17:23:35Z

Here are the results of running git log -3:

commit f364d454427e643305ecbd8ecc939e0cccafc78e (HEAD -> expire-sessions, origin/expire-sessions)
Author: mftaff
Date:   Sun Jan 14 13:37:29 2018 +0000

    Implement expiring sessions.

commit 343352dac833f72bd68c476b76eb18f7c16a3508 (origin/master, origin/HEAD, master)
Merge: 26bbc76 51c1993
Author: mftaff
Date:   Sun Jan 14 12:13:33 2018 +0000

    Merge branch 'master' of https://github.com/mikker/passwordless

commit 51c1993ad4d293ec21938e06f54deb4f78fa142e (tag: v0.4.4, upstream/master)
Author: Mikkel Malmberg
Date:   Tue Jan 2 10:17:08 2018 +0100

    0.4.4

When I try running git rebase -i master it is only taking me back to that top commit. Is this because the other commits are from my master branch? (When I started today I ran git pull upstream master which created that merge commit) Do you know how to resolve this?

(sorry for the technical troubles ;-) hope it isn't too much of a bother)

mikker · 2018-01-15T19:50:21Z

No trouble. Git can be trouble but it's usually because I mess it up myself 😁

Do you have my fork as a remote in your local repo? Then you can reference its master like

git rebase -i mikker/master # or origin/master, but that could be your own fork

If not you can add it with

git remote add mikker https://github.com/mikker/passwordless

Then the first command should give you the entire history divergence from mine.

IF we end up not getting it to work then it's totally fine. I can remove the two commits with a little force on my end – but it's more fun to see if we can do it the right way™.

mftaff · 2018-01-16T11:40:02Z

Yes! it is one commit now 🎆

mikker · 2018-01-16T12:03:18Z

Nice! Ok, LAST thing. I like to keep commit messages in the style of "Add thing" or "Change things". Basically "VERB description of what the app now does differently".

In this case maybe something like "Redirect user with error when session has expired". Makes sense? I read a pretty good blog post on this a while back. Trying if I can find it 😄

mikker · 2018-01-16T12:05:11Z

Still good! https://chris.beams.io/posts/git-commit/

mikker · 2018-01-16T12:06:16Z

"Imperative mood" is the stuff – https://chris.beams.io/posts/git-commit/#imperative

mftaff · 2018-01-16T12:09:21Z

👍 I will give it a read

mikker · 2018-01-16T12:20:44Z

Nice work! Thank you.

mftaff · 2018-01-16T12:25:47Z

😺
Again if there is anything else to do I am keen.
Either way though thanks, I've learned loads from these contributions!!

mikker · 2018-01-16T12:35:06Z

I hope you have! You are very welcome to look into the I18n stuff. You don't have to translate anything but rather lay the ground work for users to translate it themselves.

mftaff closed this Jan 14, 2018

mftaff reopened this Jan 14, 2018

mikker requested changes Jan 14, 2018

View reviewed changes

mikker requested changes Jan 15, 2018

View reviewed changes

mftaff force-pushed the expire-sessions branch from abdaf5d to 505e306 Compare January 15, 2018 12:22

mikker approved these changes Jan 15, 2018

View reviewed changes

mftaff force-pushed the expire-sessions branch from 505e306 to 43c6d89 Compare January 15, 2018 13:19

mftaff force-pushed the expire-sessions branch from 43c6d89 to f364d45 Compare January 15, 2018 14:11

mftaff force-pushed the expire-sessions branch from f364d45 to bb2c53d Compare January 16, 2018 11:39

mftaff force-pushed the expire-sessions branch from bb2c53d to dda3549 Compare January 16, 2018 12:08

Redirect user with error when session has expired

5873005

mftaff force-pushed the expire-sessions branch from dda3549 to 5873005 Compare January 16, 2018 12:13

mikker merged commit 43916ba into mikker:master Jan 16, 2018

mikker mentioned this pull request Jan 16, 2018

Make sessions (actually) expire #8

Closed

Expire sessions #10

Expire sessions #10

Conversation

mftaff commented Jan 14, 2018 • edited Loading

codecov-io commented Jan 14, 2018 • edited by codecov bot Loading

Codecov Report

mftaff commented Jan 14, 2018

mikker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikker Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mftaff commented Jan 15, 2018

mikker 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

mikker commented Jan 15, 2018

mikker commented Jan 15, 2018

mftaff commented Jan 15, 2018 • edited Loading

mikker commented Jan 15, 2018

mftaff commented Jan 15, 2018

mikker commented Jan 15, 2018

mftaff commented Jan 15, 2018 • edited Loading

mikker commented Jan 15, 2018 • edited Loading

mftaff commented Jan 16, 2018

mikker commented Jan 16, 2018

mikker commented Jan 16, 2018

mikker commented Jan 16, 2018 • edited Loading

mftaff commented Jan 16, 2018

mikker commented Jan 16, 2018

mftaff commented Jan 16, 2018

mikker commented Jan 16, 2018

mftaff commented Jan 14, 2018 •

edited

Loading

codecov-io commented Jan 14, 2018 •

edited by codecov bot

Loading

mikker Jan 14, 2018 •

edited

Loading

mftaff commented Jan 15, 2018 •

edited

Loading

mftaff commented Jan 15, 2018 •

edited

Loading

mikker commented Jan 15, 2018 •

edited

Loading

mikker commented Jan 16, 2018 •

edited

Loading