create_with_otp destroys existing session

[borgand]

The DeviseOtpAuthenticatable::Hooks::Sessions#create_with_otp method destroys previous data in warden.session when OTP challenge is required. See sessions.rb:23

This conflicts with many important features of Devise and related plugins, such as:

• return_to functionality is ignored - user is redirected to root URL instead of requested resource when authentication is complete
• password_expired information is lost in the Devise Security Extensions plugin, meaning that the users are not required to change their passwords, regardless that they are actually expired.
• probably many other features too, that I did not test yet.

I tried to tweak the code to get around this, but failed as I'm not familiar with the concepts of Devise-OTP.

Can the Devise-OTP functionality be altered so that previous session information is persisted? This seems reasonable, taking into account that OTP most often is used together with another authentication mechanism to form 2FA and that other mechanism can expect session to persist once created.

[strzibny]

@borgand is this still relevant? what would be the acceptable behaviour?

[borgand]

Hi! We are migrating away from this gem due to this, so this is so relevant, but probably we complete migration before any fixes.

Two main aspects:

• UX: any user using either a direct link or working on a page when session express will be redirected to root page, loosing context and needing to navigate again.
• Security: allowing to continue login with expired password is a potential vulnerability.

Therefore I still suggest this to be fixed.

Acceptable behaviour would be that the session data is persisted during an OTP challenge.

😄

[strzibny]

Thanks for a quick reply. I'll look into it.

[strzibny]

Btw this commit probably fixed the location at least (but I understand there are other possible issues) https://github.com/wmlele/devise-otp/commit/7c2fbe9a1efd4db52e383d9495b3728f0cdf14b7

[strzibny]

Security: allowing to continue login with expired password is a potential vulnerability.

Do I understand it correctly that this is only due to use of Devise Security Extensions plugin? Like due to logging out this is not compatible?

[strouptl]

@strzibny, I have a solution for this issue in PR #80.