Skip to content

Add YubiKey keyboard support#248

Merged
kspearrin merged 3 commits intobitwarden:masterfrom
alistair23:alistair/yubikey
Jan 18, 2018
Merged

Add YubiKey keyboard support#248
kspearrin merged 3 commits intobitwarden:masterfrom
alistair23:alistair/yubikey

Conversation

@alistair23
Copy link

No description provided.

@alistair23 alistair23 force-pushed the alistair/yubikey branch 2 times, most recently from 8531f4b to 8536695 Compare January 9, 2018 16:16
Copy link
Member

Choose a reason for hiding this comment

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

Other platforms do not need this cell since they listen to NFC.

Also, no need for the placeholder string. UI should follow how it is in the browser extension.

image

Copy link
Author

Choose a reason for hiding this comment

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

It is also possible to use the USB entry on Android, if you don't have NFC support, which is why I was adding the option. I will update the interface though.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Author

Choose a reason for hiding this comment

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

GitHub wants me to comment here as well. Same as above, USB entry does work on Android so I think it makes sense to leave it in. If you really want I can remove it on Android though.

Copy link
Member

Choose a reason for hiding this comment

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

You dont need parens around your UWP check. Also dont you need some kind of "Has USB Port" check?

@alistair23 alistair23 force-pushed the alistair/yubikey branch 2 times, most recently from a2728ed to b3e2298 Compare January 12, 2018 07:35
@alistair23
Copy link
Author

alistair23 commented Jan 12, 2018

I updated it, I tried to detect USB support, but it seems really hard to do. USB is a mess. It also seems unlikely that a Windows 10 computer doesn't have a USB port. This is my dev branch: https://github.com/alistair23/bytewarden-windows/tree/alistair/yubikey-wip

@kspearrin
Copy link
Member

Yes, but UWP is for mobile devices too. Do all windows mobile devices support USB?

@alistair23
Copy link
Author

This app doesn't support Windows for phones, so that isn't a huge issue.

I was thinking about this more though. So this app does support Xbox and maybe some future phone thing that Microsoft does. So there is merit in checking for USB. In saying that it is really hard to do. For example on my Surface Pro 2 the built-in camera is connected via USB, so the check will be a false positive there. I don't think it's unreasonable to assume that a device has a USB port, that isn't too unrealistic. If the user can't use it then they can change the second factor option.

@alistair23
Copy link
Author

It also occurred to me that the Xbox One does has USB ports and I'm pretty sure it supports USB keyboards. So there really isn't much that doesn't support USB input.

@kspearrin
Copy link
Member

Wait, so UWP apps don't run on any type of mobile phones?

@alistair23
Copy link
Author

There is no Windows Phone OS that runs with .NET standard 2.0. The latest and last Windows Phone OS is the fall creators update, which doesn't run .NET standard 2.0. This is why I had to create a fork, because Microsoft dropped the platform.

@alistair23
Copy link
Author

Unless some unannounced OS comes up this can't support a Windows Phone

@kspearrin
Copy link
Member

But aren't there mobile phones that run windows 10? If not, I am struggling to understand why we should even support UWP through the mobile project going forward. We will have a desktop app in a month or two.

@alistair23
Copy link
Author

There are no mobile phones that run a Windows OS that bitwarden can run on. UWP is good for Windows S and Xbox as well as regular Windows.

@alistair23
Copy link
Author

My fork can run on Windows Phones though

Copy link
Member

Choose a reason for hiding this comment

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

Should read:

To continue, hold your YubiKey NEO against the back of the device or insert your YubiKey into your device's USB port, then touch its button.

Copy link
Author

Choose a reason for hiding this comment

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

Updated

Copy link
Member

Choose a reason for hiding this comment

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

AppResources.YubiKeyTextEnter This text can go away to be consistent with the browser extension. It is blank.

Copy link
Member

Choose a reason for hiding this comment

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

Also, is this field auto-focused?

Copy link
Author

Choose a reason for hiding this comment

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

Ok, fixed

Signed-off-by: Alistair Francis <alistair@alistair23.me>
On Android and UWP it's possible to use the YubiKey to enter text
instead of using the NFC. Allow people to do that.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
Windows platforms should always have YubiKey support.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
@alistair23
Copy link
Author

All updated again

@kspearrin kspearrin merged commit 3f31d78 into bitwarden:master Jan 18, 2018
@alistair23 alistair23 deleted the alistair/yubikey branch January 18, 2018 17:10
alistair23 added a commit to alistair23/bytewarden-windows that referenced this pull request Jan 19, 2018
* App: Pages: Fix the YubiKey image source

Signed-off-by: Alistair Francis <alistair@alistair23.me>

* App: Allow YubiKey keyboard output on apps

On Android and UWP it's possible to use the YubiKey to enter text
instead of using the NFC. Allow people to do that.

Signed-off-by: Alistair Francis <alistair@alistair23.me>

* App: Pages: Enable YubiKey support on Windows platforms

Windows platforms should always have YubiKey support.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
@kumarharsh
Copy link

Kinda bummed that phones won't be supported ☹ But it'd be a huge help if XBox does get the app - as inputting auto-generated passwords with a controller are a major PITA!

Also, just to be clear regarding Windows support:

  1. The official Bitwarden app can't run on Windows 10 Mobile (which is stuck on Creators Update more or less).
  2. There is a fork maintained by @alistair23 which does run on Windows 10 Mobile.
  3. The official Bitwarden app can run on Windows 10 S and XBox.
  4. The official Bitwarden app will run on any future phone-like devices which run Windows 10/Andromeda.

Am I right in these assumptions?

@alistair23
Copy link
Author

  1. Correct
  2. Correct. This does run on Xbox, although the Xbox has no copy/paste support so it isn't super useful.
  3. At the moment yes.
  4. In theory yes

vvolkgang pushed a commit that referenced this pull request Jun 20, 2024
UCan927 pushed a commit to UCan927/BitWarden-Android that referenced this pull request Jun 22, 2024
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.

3 participants