kanidm / kanidm

Kanidm: A simple, secure and fast identity management platform
Mozilla Public License 2.0
2.66k stars 179 forks source link

[UX] Make sure users submit their credential changes before leaving #2504

Closed tumbl3w33d closed 1 week ago

tumbl3w33d commented 8 months ago

Is your feature request related to a problem? Please describe.

I've noticed a few users struggle with the necessity to submit changes to passkeys twice, i.e.:

At this point some users close the tab and leave. Then they wonder why they cannot login with the passkey they just added. What happened? After the procedure described above you need to submit the entire authentication settings form to actually store the changes. I think that's not optimal from UX perspective.

It's the same behavior when deleting passkeys and should be adjusted as well. There already is an extra confirmation dialog so you don't accidentally delete passkeys.

Describe the solution you'd like

When I submit the name of the passkey, it should really be stored. Maybe the other submit button of the entire form is pointless? I don't know all the use cases.

Describe alternatives you've considered

You give affected people another onboarding token and tell them to submit harder.

Additional context

Can add more info if it's not clear from the description.

Another minor thing (but edge case) is that the "Updating Credentials" view doesn't update the list of passkeys while it's opened and you refresh the tab. That means if you change the set of credentials on another device (e.g., mobile via QR code) you won't see it in this opened tab on your workstation and I wouldn't know what happens if you submit the entire form without your tab being aware of that change. The entire problem would probably disappear if this surrounding form would go away and be replaced with direct submits as suggested above.

Firstyear commented 8 months ago

What you are hitting is that credential updates are done in transactions. That's why there is an extra submit, and in-flight changes aren't reflected to other devices. This is so that the "state of the credentials at the start of the transaction" is a precise known state, and we can then apply account policy and such within that transaction.

In addition, all the clicks to "start" the passkey registration, while annoying, are needed because iOS/safari have bugs around what is classed as user interaction. Without the "begin" click, it actually won't prompt to enroll anything. It's a pain in the butt, but it's what we have to do.

Without this, it would be possible for someone to do things like setting a totp on on browser, while deleting the password on another - what are we meant to do here?

By doing everything in a transaction, we have a precise, whole state of the accounts credentials before and after the operation.

Another part of this is replication. What if the user on their browser is connected to Kani server A, but their phone to Kani server B? Changes on B may be submitted but they can take time to arrive at A. So even without transactions, there is always a possible consistency delay.

Also things like the reset tokens - a reset token can only be used once. While it's possible to potentially start multiple credential update transactions with a reset token, only one of them will apply, and the rest will be rejected. This is so that if an attacker gets the reset token at the same time, provided the user submits first, their changes win. If the attacker submits first, the reset token is "invalid" and the user can then raise the alarm (or ask for a new one).

I'm sure it's a bit annoying, but all these elements exist for a reason.

I think we can however, make the UI better for the user to make it clearer that they have to "save their changes". This is common on other workflows for updating personal info or similar.

Instead of being "submit" should the button say "save changes" and we warn that changes aren't saved until they click that?

tumbl3w33d commented 8 months ago

I'm not surprised there's a good reason for it. 😁

How about catching the tab/browser close event then and inform the user about unsaved changes?

Firstyear commented 8 months ago

That's also a really good idea!

yaleman commented 8 months ago

Yeah, we should be able to flag the whole "hey you're not done" part.

ethindp commented 6 months ago

I don't know if you can do this when intercepting tab/window close events, but perhaps we could ask the user "Hey, we see you want to close this tab/browser window but you haven't saved your changes, so want us to do that for you?" User clicks yes, changes are saved and then the browser/tab finishes closing. But I don't know if the JS interface would allow us to do that.

yaleman commented 6 months ago

There's the window.beforeunload event which can catch it.

joostrijneveld commented 3 months ago

I thought I understood the reason when you described the transactions, but I'm no longer convinced I do. At the end of the passkey enrolment and after filling in a name, the user clicks a 'submit' button in the pop-over dialog. Why could that submit button not submit the transaction (and trigger page reload), like the green 'Submit Changes' button currently does on the main page? We might as well color it green at that point :-)

In case of adding a new password, I can see that simply adding a password may not be sufficient for the transaction to pass account policy (i.e., when a second factor is required), and prematurely submitting will always result in transactions that fail to meet the requirements. This could even be something we could check for, though: if the account policy is any, a green submit button in the dialog could complete the transaction, and in case of mfa, it could exhibit the current behaviour.

I assume most users are unlikely to ever encounter both scenarios (by being exposed to multiple environments, or by using multiple accounts within the same environment), so I actually don't see much opportunity for confusion because of inconsistency.

Firstyear commented 3 months ago

Because they still might be adding more passkeys. They might add the passkey then remove their password and totp.

And if "any submit" committed the txn, then we'd need a way to restart the txn all the time because the user may not be finished. So it becomes a complete pain server side.

We are currently working on the web side of things right now though, so I agree there are ways we can improve this to make it a better user experience, but the transaciton model isn't going away. It exists on so many sites so I think it's not unreasonable we have one, we just need to improve the ux around it.

joostrijneveld commented 3 months ago

Right, and I assume you'll want to have at most one transaction per reset token; I suppose that makes sense. Or is there a reason I'm overlooking why it's a pain to start a new transaction?

Firstyear commented 1 month ago

@ToxicMushroom @weijiangan I think this relates to your current work on the credential update stuff?

ToxicMushroom commented 1 month ago

I don't feel like it's necessary to reinit the transaction and have intermediary commits, then the cancel button at the bottom becomes a gambling machine.

I would rather make the buttons sticky and floating so it's always in view, discord does this too. I personally don't prefer this but I think it'd be easier for new users. image

Perhaps the button texts could also be clearer, I'd personally prefer add for all the modals that don't commit and save for the commit. Currently we use submit for everything which feels like a save.

I'll also add the window.beforeunload event listener suggestion

Let me know your thoughts :)

Firstyear commented 1 month ago

We can also strip out the modals as they exist now. I think the floating "careful" is good.

And yes, renaming to "add" probably will help. Similar save changes as the text over commit.

I think these are all good ways to improve things.

ToxicMushroom commented 1 month ago

Hrm, beforeunload also gets triggered by reloads. Reloads don't really affect the ability to save on our page and may get triggered programmatically (although rare).

weijiangan commented 1 month ago

this is a problem we try to solve numerous times at work, trying to interrupt users from leaving an unfinished flow. It's quite unsolvable because browsers don't allow it because you can see how easily ad-infested sites can abuse this. We ended up implementing saving unfinished flows or doing partial commits, making flows resumable.

I understand it's a different scenario here as there is a time limit for credential update, so I think trying to fix it by showing reminders or showing a floating button here is the best solution. You cannot customize beforeunload and it is not guaranteed to fire.