Simplify refresh_credential functionality

[strouptl]

@strzibny, I am requesting to simplify the refresh_credential functionality in preparation for some other Pull Requests.

This Pull request updates the needs_credentials_refresh? method to consider the current_sign_in_at so that we don't have to call "refresh_otp_credentials_for(resource)" artificially when signing in.

Details:

• Update needs_credentials_refresh? method to reference current_sign_in_at when present;
• Break apart needs_credential_refresh? method, and set return URL within otp_tokens controller for simplicity;
• Remove 'refresh_otp_credentials_for(resource)' from create_otp_session method (no longer needed);

Current tests are sufficient and passing for this change.

[strouptl]

NOTE: This would assume the trackable module is included. I think it is worth the tradeoff, though, versus the complexity of calling refresh_otp_credentials during sign in.

Happy to update the README for this. Let me know if you have any thoughts.

[strzibny]

I personally use trackable but it's certainly a big assumption for others. I wonder if there is a way to "depend" on that module somehow

[strouptl]

I was wondering that too. I don't see anything online for this (cf. https://stackoverflow.com/questions/23965646/how-to-configure-devise-to-re-authenticate-user), or any other pre-built solutions for re-authenticating users for sensitive areas (checked devise-security gem).

However, I may have another solution. Let me give it a quick shot, and I will push it back up for you.

[strouptl]

OK, I have added a dedicated field (credentials_refreshed_at) and hook for refreshing credentials (versus depending on trackable). Take a look, and let me know what you think.

NOTE: We will definitely need to add this to the changelog, as it requires adding a new database field.

[strouptl]

One more step might be extracting the Credential Refresh functionality to a dedicated controller, and adding an "refresh_credentials!" helper method so that you can perform this in any controller you like (not just the OTP tokens controller).

If you are interested, I can work on extracting that to a dedicated gem to further simplify things.

[strouptl]

(Sorry, I posted this comment to the wrong PR. Here is the updated comment).

@strzibny , I went ahead and extracted the check/redirection to a dedicated refresh_credentials! helper, and it is working great. With that, I think we are ready to merge. Let me know if you have any other comments.

[strouptl]

@strzibny, I realized that moving the refreshed credentials functionality to the database does not account for other browser usage. For example, if you leave a browser logged in on a public computer, and then login again later on a personal device, then the public computer will reference the new credentials_refreshed_at datetime.

Doing this via a dedicated hook is still a good step if possible (to get away from overwriting the "create_session" method), but we most likely need to revert to storing this and calculating it at the browser layer.

I will update this branch with the changes above. Please hold off on merging until then.

Thanks!

[strzibny]

Okay, thanks for the updates. I'll wait with the review. Can I also ask what changes you want to later based on this?

[strouptl]

Sure. The main thing I am trying to accomplish is to ensure that the mandatory OTP is working correctly (Issue #71 ). To do this, we need to enforce the redirection throughout the application, rather than just in the "create_otp_session" method. As a result, I am trying to get away from overusing the Sessions hook as follows:

1. OTP Challenge: Update Database Authenticatable strategy to redirect to challenge via native Warden "redirect!" method. This is in PR Native Warden Authentication #77 .

2. Credential Refresh: Add Refreshable hook to refresh credentials after successful sign_in (including OTP authentication). This is in the current PR.

3. Mandatory OTP: Add ensure_mandatory_otp! helper method like authenticate_user! that will ensure that an user is forced to enable OTP before proceeding to other parts of the application. This is still in process, but needs to be completed before merging PR Native Warden Authentication #77 so that it does not take away existing functionality.

Does that make sense? Please let me know if you have any thoughts. I would be happy to meet sometime too, if that would help.

[strouptl]

Closing PR. Will open a new PR with a simpler solution.