donotcache ignored on forceauthn
Description
Environment
Attachments
- 23 Aug 2017, 07:49 AM
Activity
Scott Cantor August 30, 2017 at 12:35 PM
I didn't use the same fix you did because it wasn't right. Checking for a session isn't the right test.
The test I added determines if there's already a cached result of this type. If there is, then all that's going to happen is that the old cached result gets overwritten with the updated one, which feels like the most reasonable default. It mainly just updates the timestamps for the most part.
There is no unambiguously "right" outcome. It's whatever you want the outcome to be.
The original example was clearly wrong. It would show the box even if the result had been cached already, giving the illusion that the option could stop that, when it can't undo what's already been done previously. My small change prevents that much, and that's all that I can unambiguously say it should do.
Manuel Haim August 30, 2017 at 6:50 AM
Hi Scott,
did you just remove the checkbox within the velocity template, or also do some magic within the IdP's Java classes?
In my tests, the hidden field in 3.3.1 was required within the template. If I did not add a hidden field, the result was the same as if there was an unchecked checkbox (which means "start to cache now"). As you see, this would overwrite the user's former decision of "do not cache".
Manuel
Scott Cantor August 29, 2017 at 3:42 PM
Defaulted it to checking for an authn/Password result. This would vary based on configuration, it would often be authn/MFA, but it illustrates the point.
Setting a hidden field seems unnecessary because of the box were not shown, it would mean the result would be to update the result from before, which is likely desirable.
This is very much not a one solution problem, it's entirely subjective.
Scott Cantor August 29, 2017 at 3:17 PM
I'm fairly certain the most appropriate criteria for this is really "is there a cached result for this authentication flow already stored in a session?"
The existence of a session in and of itself doesn't preclude displaying it necessarily. It really just determines whether this particular impending result is going to be cached or not. I'll have to consider how best to detect that, it may just be checking the AuthenticationContext.
Manuel Haim August 23, 2017 at 7:51 AM
Patch updated (look only for not-yet-existing idpSession, set donotcache=1 by hidden input otherwise to prevent enabling cache on second request)
This bug affects logins with username and password.
If a user did not check the "Don't Remember Login" checkbox during first login, any later checking of the "Don't Remember Login" checkbox on subsequent forceauthn login requests will be ignored.
Thus, a user cannot change behaviour from "Remember Login" to "Don't Remember Login".