Skip to content

LG 11453 Remove platform device nickname#9657

Merged
kevinsmaster5 merged 34 commits intomainfrom
kmas-lg-11453-remove-platform-device-nickname
Dec 11, 2023
Merged

LG 11453 Remove platform device nickname#9657
kevinsmaster5 merged 34 commits intomainfrom
kmas-lg-11453-remove-platform-device-nickname

Conversation

@kevinsmaster5
Copy link
Contributor

@kevinsmaster5 kevinsmaster5 commented Nov 27, 2023

🎫 Ticket

LG-11453

🛠 Summary of changes

On Platform authenticator setup Nickname field is now a hidden field with the decorated device browser name as its value.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Go to http://localhost:3000
  • Create a new account and add Face or touch unlock
  • Note the absence of a nickname field. See that browser info is in the device nickname box.
  • In some edge cases a user might have another device with the same OS and browser. Changes should allow for that. Testing can be done by way of modifying a user in the rails console

User.find_with_email('//user email//').webauthn_configurations.create(name: "//browser nice_name//", platform_authenticator: true, transports: ["internal", "hybrid"], credential_id: "123xyz", credential_public_key: "pdq456")

For browser nice name use the name that would be injected from your testing system like 'Chrome 120 on macOS 10'

Screenshot 2023-11-27 at 10 31 45 AM (2)

Nickname field should still be present when adding a security key

Screenshot 2023-11-27 at 10 33 44 AM (2)

Incremented number after the automatically generated nickname for duplicate device environment.

Screenshot 2023-12-06 at 4 53 58 PM (2)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to pass in the device name from the constructor? I think it'd be preferable since the device name from the user agent would be more direct compared to querying the database (and also saves a database query).

Is there concern that the existing unique requirement on name could create issues if it can't be overridden?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that suggestion. I'm still reworking how best to handle that but I'm no longer hitting recent_devices.

There is a method in WebauthnSetupForm that checks if the name is unique. I can try to handle appending to a duplicate name there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will need to be handled, yeah

@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review November 29, 2023 14:28
@kevinsmaster5 kevinsmaster5 marked this pull request as draft December 4, 2023 19:02
@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review December 4, 2023 22:40
@kevinsmaster5 kevinsmaster5 force-pushed the kmas-lg-11453-remove-platform-device-nickname branch from 4cf89d3 to da16b91 Compare December 4, 2023 22:41
Comment on lines +68 to +71
incrementer = 1
while WebauthnConfiguration.exists?(user_id: @user.id, name: @name)
@name = "#{@name} (#{++incrementer})"
end
Copy link
Contributor

@zachmargolis zachmargolis Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Ruby has no ++ operator (source link)

  2. Since we're re-assigning @name this would have like "My Device (1) (2) (3)"

I would also consider counting the number of devices that exist and just appending? We can do a prefix match on the name to see how many of that device already exist

Suggested change
incrementer = 1
while WebauthnConfiguration.exists?(user_id: @user.id, name: @name)
@name = "#{@name} (#{++incrementer})"
end
if WebauthnConfiguration.exists?(user_id: @user.id, name: @name)
num_existing_devices = WebauthnConfiguration.
where(user_id: @user.id).
where('name LIKE ?', "#{@name}%").
count
@name = "#{@name} (#{num_existing_devices + 1})"
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zachmargolis thank you for that! It was starting with (2) when I tested locally, if I remove the + 1 that should start with (1) and go up from there, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! It's very naive, so for example if a user has "Device", "Device 1", "Device 2" and deletes "Device 1", the next would probably be "Device 2"? it might be fine?

@aduth aduth requested a review from a team December 11, 2023 16:14
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this locally and it worked as I expected, both in terms of generating the expected automatic nickname, as well as adding an incrementing suffix (I tested with 3 of the same generated name).

Added a comment about a previous review nitpick, but otherwise LGTM 👍

Correct use of class member labeling

Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
@kevinsmaster5 kevinsmaster5 merged commit d95fb90 into main Dec 11, 2023
@kevinsmaster5 kevinsmaster5 deleted the kmas-lg-11453-remove-platform-device-nickname branch December 11, 2023 21:00
@jmhooper jmhooper mentioned this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants