search

canonical / identity-platform-login-ui

Login UI for the Canonical identity broker and identity provider solution
Apache License 2.0
9 stars 6 forks source link

Update device code page #226

Closed nsklikas closed 3 months ago

nsklikas commented 3 months ago

IAM-736:

There are a couple problems with this PR:

lukasSerelis commented 3 months ago

Addressed the no error state for incorrect device code here

Regarding the split of text field per character is low priority, also we don't have front-end components to support this atm. The placeholder text sort-of helps this, the goal is just to match the placeholder formatting to the console formatting

nsklikas commented 3 months ago

Added error handling in the UI

the code is invalid image

hydra is down, so we get an unexpected error image

nsklikas commented 3 months ago

@lukasSerelis @edlerd

Not sure if this makes sense, but there are 2 case when you end up in the device_code page, you either get there :

Currently in the second case you will be unable to edit the user code (you can't change type a different code), not sure if this makes sense and I am not sure why this happens (probably because of https://github.com/canonical/identity-platform-login-ui/pull/226/files#diff-90614cb17aaa983f34c429b732b82f94716e1f4fa61c25c752f7c43d0d702537R63 @edlerd ?)

My question is: is this wrong? do we want to allow the users to edit the code? (probably yes I would say)

edlerd commented 3 months ago

@lukasSerelis @edlerd

Not sure if this makes sense, but there are 2 case when you end up in the device_code page, you either get there :

  • with a URL without a user_code param, in which case the box will simply show a placeholder image
  • with a URL with a user_code param, in which case the input box will be pre-populated with the value of the user_code.

Currently in the second case you will be unable to edit the user code (you can't change type a different code), not sure if this makes sense and I am not sure why this happens (probably because of https://github.com/canonical/identity-platform-login-ui/pull/226/files#diff-90614cb17aaa983f34c429b732b82f94716e1f4fa61c25c752f7c43d0d702537R63 @edlerd ?)

My question is: is this wrong? do we want to allow the users to edit the code? (probably yes I would say)

Shouldn't we auto submit the form if the user comes with a code already in the URL?

If we want the user to be able to change the code, better use defaultValue={code} instead of value={code}.

nsklikas commented 3 months ago

Shouldn't we auto submit the form if the user comes with a code already in the URL?

That was my initial thought as well, but the RFC says:

If the user starts the user interaction by navigating to "verification_uri_complete", then the user interaction described in Section 3.3 is still followed, with the optimization that the user does not need to type in the "user_code". The server SHOULD display the "user_code" to the user and ask them to verify that it matches the "user_code" being displayed on the device to confirm they are authorizing the correct device. As before, in addition to taking steps to confirm the identity of the device, the user should also be afforded the choice to approve or deny the authorization request.

And it makes sense from a security perspective, so that you are sure that you are validating your device and that you are not being scammed

lukasSerelis commented 3 months ago

@lukasSerelis @edlerd

Not sure if this makes sense, but there are 2 case when you end up in the device_code page, you either get there :

* with a URL without a `user_code` param, in which case the box will simply show a placeholder
  ![image](https://private-user-images.githubusercontent.com/19745916/319672045-1e597a9e-1bb0-44fa-9982-378395c5567c.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTIzMTE1ODksIm5iZiI6MTcxMjMxMTI4OSwicGF0aCI6Ii8xOTc0NTkxNi8zMTk2NzIwNDUtMWU1OTdhOWUtMWJiMC00NGZhLTk5ODItMzc4Mzk1YzU1NjdjLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA0MDUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNDA1VDEwMDEyOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQxYzBhMDY0MGI5ZmFmZjkxMTMwMTFlYjA4YTBmODEyMjdhOTI1NmVmZTkxMzZkM2U1Y2Y2ZWI5NTk4NzY0MWQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.XNyZnuohXvIP9p6xgi1nOzlR2KrGq-gBy_eY0AnOtus)

* with a URL with a `user_code` param, in which case the input box will be pre-populated with the value of the user_code.

Currently in the second case you will be unable to edit the user code (you can't change type a different code), not sure if this makes sense and I am not sure why this happens (probably because of https://github.com/canonical/identity-platform-login-ui/pull/226/files#diff-90614cb17aaa983f34c429b732b82f94716e1f4fa61c25c752f7c43d0d702537R63 @edlerd ?)

My question is: is this wrong? do we want to allow the users to edit the code? (probably yes I would say)

I think having the code non editable is okay, but some surrounding copy would need to change. Instead of "Enter the code to access ..." it should say "Verify the code to access ..." or something between those lines. Ideally text field should signify that it's not editable as well, to not cause any confusion, and to avoid any edge edge edge cases we should add a way to go back to the empty state where they could enter a code manually. image

lukasSerelis commented 3 months ago

RE single input per character, if we can bend vanilla a little bit, we could make something like this, which would improve the usability as @nsklikas pointed out, just need to make sure the behavior is smooth (nicely hoping from field to field after entering a character, pasting works nice, deleting works nice and moves cursor through fields logically) image image

nsklikas commented 3 months ago

Instead of "Enter the code to access ..." it should say "Verify the code to access ..." or something between those lines

@lukasSerelis unfortunately we don't know what application the user is trying to access at this stage. The user_code is used to connect the login session with the device. This is why the rfc states that the user code verification should be followed by a consent for the device (which I think we are planning not to implement, at least for now)

Ideally text field should signify that it's not editable as well, to not cause any confusion, and to avoid any edge edge edge cases we should add a way to go back to the empty state where they could enter a code manually.

I would like to merge this PR with out changing the UI design too much and to polish the UI on a later stage, preferably someone else with more UI knowledge should do it. Currently I made it editable and we can iterate on it.

if we can bend vanilla a little bit, we could make something like this, which would improve the usability

After talking with Massi, I think that the vanilla framework will add this component. So we can change the UI when that happens