Actually use Device Type for mails

[dfunkt]

• match Bitwarden behaviour
• remove upcase_first, no longer needed and it would also interfere with device types like iOS, macOS

Examples:

Chrome Extension
Before -> New Device Logged In From/Incomplete Two-Step Login From/Device Type: Chrome
After -> New Device Logged In From/Incomplete Two-Step Login From/Device Type: Chrome Extension

Poco F3 (shows up as M2012K11AG)
Before -> New Device Logged In From/Incomplete Two-Step Login From/Device Type: M2012K11AG
After -> New Device Logged In From/Incomplete Two-Step Login From/Device Type: Android

[jjlin]

If the device type is Android or iOS, the mobile client should include a device model as the name, so the notification email should ideally include that info for those device types. I'm not sure if any other device types include useful model/name info.

[dfunkt]

Let's not forget that the email template says "Device Type", not "Device Type and Name/Model" or anything like that.
Example:

vaultwarden/src/static/templates/email/new_device_logged_in.html.hbs

Line 22 in 248e561

<b>Device Type:</b> {{device}}

Ideally I try to stick as close as possible to Bitwarden's behaviour when implementing these changes, and the emails from them don't specify the device name/model number either.

Nonetheless, let's see what others have to say about this change.

[jjlin]

Vaultwarden does various things differently from Bitwarden, as long as they are compatible improvements. For example, Bitwarden does not even have a 2FA incomplete notification AFAIK.

[BlackDex]

I agree with @jjlin.
For all the client functionality we should stay as close as possible.
Server-side specific changes should be looked at if it has no impact client side or might alter a way of working.

But for something like emails shouldn't be an issue.
I actually love it shows we the device name and not only type.
So both would be better.

I even was thinking about adding basic location detection based upon IP.

[dfunkt]

New variant with device name and device type included.
Examples:

[dfunkt]

I chose atype on purpose to match how it was in device.rs:

vaultwarden/src/db/models/device.rs

Line 19 in 248e561

pub atype: i32, // https://github.com/bitwarden/server/blob/dcc199bcce4aa2d5621f6fab80f1b49d8b143418/src/Core/Enums/DeviceType.cs

Which one do you prefer @BlackDex? atype or just type?

[BlackDex]

type is the correct name. The only reason there is an a in front is because you can't use that as a variable name.

[jjlin]

I chose atype on purpose to match how it was in device.rs:

vaultwarden/src/db/models/device.rs

Line 19 in 248e561

pub atype: i32, // https://github.com/bitwarden/server/blob/dcc199bcce4aa2d5621f6fab80f1b49d8b143418/src/Core/Enums/DeviceType.cs

This is only called atype because you can't have an identifier that's just called type. There are many other existing instances of device_type in the code.

[dfunkt]

Hopefully things are good now

[dfunkt]

@BlackDex Not sure if this is what you meant, but that's all I could come up with. I'm not particularly familiar with Rust (or programming in general tbh).

[BlackDex]

Yes, that is what i mend indeed.

I just noticed something else too. Not that important, but no need to create new variables for those items actually.

[gbe0]

With this merged I get the following on startup:

[2024-09-19 10:17:36.721][panic][ERROR] thread 'main' panicked at 'Error running migrations: QueryError(DieselMigrationName { name: "2024-09-04-091351_use_device_type_for_mails", version: MigrationVersion("20240904091351") }, DatabaseError(Unknown, "syntax error at or near \"`\""))': src/db/mod.rs:495
   0: vaultwarden::init_logging::{{closure}}
   1: std::panicking::rust_panic_with_hook
   2: std::panicking::begin_panic_handler::{{closure}}
   3: std::sys::backtrace::__rust_end_short_backtrace
   4: rust_begin_unwind
   5: core::panicking::panic_fmt
   6: core::result::unwrap_failed
   7: vaultwarden::db::DbPool::from_config
   8: vaultwarden::main::{{closure}}
   9: vaultwarden::main
  10: std::sys::backtrace::__rust_begin_short_backtrace
  11: main
  12: <unknown>
  13: __libc_start_main
  14: _start

My DB is PostgreSQL 16.4 on x86_64-pc-linux-gnu, compiled by gcc (Debian 12.2.0-14) 12.2.0, 64-bit.

I rolled back and there are no issues.

[Gerardv514]

@dfunkt i just checked this out. I have an iOS device and just tried to login to the web vault using Google chrome iOS app, i closed out on the 2fa page which prompted the 2fa incomplete notification. Device type and device name both say safari, is that normal?

[dfunkt]

I think that might be due to Chrome using the same browser engine as Safari on iOS. I expect this happens with Bitwarden as well, but I don't have any iOS devices in order to test my theory.

TL;DR: I think this is expected

[Gerardv514]

I also attempted the iOS bitwarden app, and device name was iPhone 15,2 and device type was iOS.

So as you said probably normal due to browser not passing the info along.