[PM-9388] Add new device type for DuckDuckGo browser #14708
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #14708 +/- ##
==========================================
+ Coverage 36.34% 36.50% +0.16%
==========================================
Files 3190 3200 +10
Lines 92150 92680 +530
Branches 16524 16691 +167
==========================================
+ Hits 33491 33833 +342
- Misses 56268 56392 +124
- Partials 2391 2455 +64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (7)Checkmarx found the following issues in this Pull Request
Fixed Issues (1)Great job! The following issues were fixed in this Pull Request
|
trmartin4
changed the title
trmartin4
requested review from
tangowithfoxtrot and
justindbaur
and removed request for
tangowithfoxtrot
| @@ -221,6 +225,10 @@ export abstract class BrowserPlatformUtilsService implements PlatformUtilsServic | |||
| return true; | |||
| } | |||
|
|
|||
| supportsSyncDomains(): boolean { | |||
There was a problem hiding this comment.
I think a better name for this is supportsAutofill my dream would be that this would live on something autofill owned as well but this needs to be used pretty low level and I don't know of a great place it right now so we can leave it.
| @@ -83,6 +88,10 @@ export class WebPlatformUtilsService implements PlatformUtilsService { | |||
| return this.getDevice() === DeviceType.SafariBrowser; | |||
| } | |||
|
|
|||
| isDuckDuckGo(): boolean { | |||
There was a problem hiding this comment.
I don't think we should add these anymore. The ideal is that no team needs to do specific client checks and even if they do the getDevice() check already exists.
|
|
||
| test("returns DuckDuckGo browser with example User-Agent", () => { | ||
| setUserAgent( | ||
| "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/18.3.1 Safari/605.1.15 Ddg/18.3.1", |
There was a problem hiding this comment.
I think a generalization of this test would be useful so we can define a lot of inputs and the expected browser a little easier.
const testData: { userAgent: string, expectedDevice: DeviceType }[] = [
{
userAgent: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/18.3.1 Safari/605.1.15 Ddg/18.3.1",
expectedDevice: DeviceType.DuckDuckGoBrowser,
}
];Also some comments about what the user agent was taken from version, OS, browser type for example. One of the test cases could be Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/135.0.0.0 Safari/537.36 Edg/135.0.0.0 which I just grabbed from Duck Duck Go's windows browser v0.109.7 which I'm pretty sure will report Edge. I'd have it include a comment saying that it is expected to be Edge right now but if something changes we'd be happy to have the expected device type become more accurate later on.
There was a problem hiding this comment.
Added additional tests for all the different browsers, based on current user-agent strings.
| @@ -12,7 +12,7 @@ export class WebFileDownloadService implements FileDownloadService { | |||
| download(request: FileDownloadRequest): void { | |||
| const builder = new FileDownloadBuilder(request); | |||
| const a = window.document.createElement("a"); | |||
| if (!this.platformUtilsService.isSafari()) { | |||
| if (!(this.platformUtilsService.isSafari() || this.platformUtilsService.isDuckDuckGo())) { | |||
There was a problem hiding this comment.
If we ever accurately identified DuckDuckGo on windows this logic might break down. I think a lot of our isSafari checks should actually be isWebkit checks and I think this one is too. Since DDG on windows is Edge based this logic would become wrong. It should be fine for now but I think we should get a ticket on the backlog for transitioning isSafari checks to capability based checks.
There was a problem hiding this comment.
I've created a backlog ticket for converting isSafari() to capability-based checks. For this particular check, I converted this to supportsFileDownload(), as that is what we're trying to do here, I believe; my research tells me that Safari's respect of the download property for file downloads is flaky, and so we have to have the backup of opening in a new tab. That's what I tried to express here.
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-9388
📔 Objective
We currently determine the Safari browser type based on the presence of
Safari /in theUser-Agent. Due to the fact that DDG is built on WebKit, it also hasSafari /and is thus identified as such when viewing your devices.This change adds a new
DeviceTypeand uses the presence ofDdgin theUser-Agentfor the DuckDuckGo brower to identify it.See bitwarden/server#5799 for the corresponding
serverchanges.isSafari()checks will no longer returntruefor DuckDuckGo (as it's now classified as DuckDuckGo).I opted to clean up these reference in a few different ways, to hopefully get rid of some debt:
isSafari()in which the methods do not appear to be used:add-edit.component.tssecrets-list.component.tsApiServicehad logic to determine the type of client that was independent of thePlatformUtilsService. That felt not great, and I didn't want to just addDuckDuckGoto the list. So I pulled the check into a feature -supportsAutofill()- on thePlatformUtilsServiceand set tofalsefor any platform except the browser extension. This reproduces the behavior of the check previously in theApiService.One additional note:
The prompt for browser extension install was left as-is, meaning that DuckDuckGo will no longer link to the Mac App Store, which I believe is correct. I've tagged Vault on this PR to make sure this is right.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes