Skip to content

Commit

Permalink
[Password Manager] Url validation for the add password dialog
Browse files Browse the repository at this point in the history
This CLs adds url validation to the add password dialog on the new UI.
The behaviour is copied from the old UI.

Bug: 1398560
Change-Id: I380259f815afea4948e2f3b892f2db579e639c7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4272613
Commit-Queue: Viktor Semeniuk <vsemeniuk@google.com>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108216}
  • Loading branch information
Viktor Semeniuk authored and Chromium LUCI CQ committed Feb 22, 2023
1 parent a41da22 commit 614d19f
Show file tree
Hide file tree
Showing 11 changed files with 151 additions and 6 deletions.
6 changes: 6 additions & 0 deletions chrome/app/password_manager_ui_strings.grdp
Original file line number Diff line number Diff line change
Expand Up @@ -306,4 +306,10 @@
<message name="IDS_PASSWORD_MANAGER_UI_SHOW_MORE" desc="Text in a link shown when the password note is too long. When clicked it shows the note fully.">
Show more
</message>
<message name="IDS_PASSWORD_MANAGER_UI_NOT_VALID_WEB_ADDRESS" desc="Text indicating that the Web address entered by the user is invalid." >
Not a valid web address
</message>
<message name="IDS_PASSWORD_MANAGER_UI_MISSING_TLD" desc="An error message when user entered a URL without a top-level domain as a site for a new password. It suggests to use the same value but with common top level domain." meaning="Suggesting user to add the top-level domain in the website text.">
Did you mean <ph name="WEBSITE">$1<ex>twitter.com</ex></ph>?
</message>
</grit-part>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
6f6f7e0b3aee974b7f754d36f66ab1ce4009b3cb
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
13d3bbcbd16feec628a953fbc6ff203e20fab07b
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@
--settings-textarea-footer-display: flex;
}

#websiteInput[invalid],
#usernameInput[invalid] {
--cr-input-error-display: block;
}

#footnote {
margin-inline-start: 2px;
margin-top: 16px;
Expand All @@ -32,9 +37,11 @@
<h1 slot="title" id="title" class="dialog-title">$i18n{addPasswordTitle}</h1>
<div slot="body">
<!-- TODO(crbug.com/1398560): Support choosing store in which to save -->
<!-- TODO(crbug.com/1398560): Show error message when input is invalid -->
<cr-input id="websiteInput" label="$i18n{websiteLabel}" autofocus
placeholder="example.com" required>
<cr-input id="websiteInput" label="$i18n{websiteLabel}" autofocus required
placeholder="example.com" value="{{website_}}"
invalid="[[isWebsiteInputInvalid_(websiteErrorMessage_)]]"
error-message="[[websiteErrorMessage_]]"
on-input="validateWebsite_" on-blur="onWebsiteInputBlur_">
</cr-input>
<!-- TODO(crbug.com/1398560): Show error when username already exists -->
<!-- TODO(crbug.com/1398560): Show a link to existing password -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,24 @@ import 'chrome://resources/cr_elements/cr_shared_style.css.js';
import '../shared_style.css.js';

import {CrDialogElement} from 'chrome://resources/cr_elements/cr_dialog/cr_dialog.js';
import {CrInputElement} from 'chrome://resources/cr_elements/cr_input/cr_input.js';
import {I18nMixin} from 'chrome://resources/cr_elements/i18n_mixin.js';
import {PolymerElement} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';

import {PasswordManagerImpl} from '../password_manager_proxy.js';

import {getTemplate} from './add_password_dialog.html.js';

export interface AddPasswordDialogElement {
$: {
dialog: CrDialogElement,
websiteInput: CrInputElement,
};
}

export class AddPasswordDialogElement extends PolymerElement {
const AddPasswordDialogElementBase = I18nMixin(PolymerElement);

export class AddPasswordDialogElement extends AddPasswordDialogElementBase {
static get is() {
return 'add-password-dialog';
}
Expand All @@ -31,12 +38,48 @@ export class AddPasswordDialogElement extends PolymerElement {
}

static get properties() {
return {};
return {
website_: String,

/**
* Error message if the website input is invalid.
*/
websiteErrorMessage_: {type: String, value: null},
};
}

private website_: string;
private websiteErrorMessage_: string|null;

private onCancel_() {
this.$.dialog.close();
}

/**
* Helper function that checks whether the entered url is valid.
*/
private async validateWebsite_() {
PasswordManagerImpl.getInstance()
.getUrlCollection(this.website_)
.then(urlCollection => {
this.websiteErrorMessage_ = null;
if (!urlCollection) {
this.websiteErrorMessage_ = this.i18n('notValidWebsite');
}
})
.catch(() => this.websiteErrorMessage_ = this.i18n('notValidWebsite'));
}

private onWebsiteInputBlur_() {
if (!this.websiteErrorMessage_ && !this.website_.includes('.')) {
this.websiteErrorMessage_ =
this.i18n('missingTLD', `${this.website_}.com`);
}
}

private isWebsiteInputInvalid_(): boolean {
return !!this.websiteErrorMessage_;
}
}

declare global {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,15 @@ export interface PasswordManagerProxy {
* Shows the file with the exported passwords in the OS shell.
*/
showExportedFileInShell(filePath: string): void;

/**
* Requests whether the given |url| meets the requirements to save a password
* for it (e.g. valid, has proper scheme etc.).
* @return A promise that resolves to the corresponding URLCollection on
* success and to null otherwise.
*/
getUrlCollection(url: string):
Promise<chrome.passwordsPrivate.UrlCollection|null>;
}

/**
Expand Down Expand Up @@ -376,6 +385,10 @@ export class PasswordManagerImpl implements PasswordManagerProxy {
chrome.passwordsPrivate.showExportedFileInShell(filePath);
}

getUrlCollection(url: string) {
return chrome.passwordsPrivate.getUrlCollection(url);
}

static getInstance(): PasswordManagerProxy {
return instance || (instance = new PasswordManagerImpl());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,12 @@ content::WebUIDataSource* CreateAndAddPasswordsUIHTMLSource(
{"localPasswordManager",
IDS_PASSWORD_BUBBLES_PASSWORD_MANAGER_LINK_TEXT_SAVING_ON_DEVICE},
{"menu", IDS_MENU},
{"missingTLD", IDS_PASSWORD_MANAGER_UI_MISSING_TLD},
{"moreActions", IDS_PASSWORD_MANAGER_UI_MORE_ACTIONS},
{"muteCompromisedPassword", IDS_PASSWORD_MANAGER_UI_MUTE_ISSUE},
{"mutedCompromisedCredentials",
IDS_PASSWORD_MANAGER_UI_MUTED_COMPROMISED_PASSWORDS},
{"notValidWebsite", IDS_PASSWORD_MANAGER_UI_NOT_VALID_WEB_ADDRESS},
{"notesLabel", IDS_PASSWORD_MANAGER_UI_NOTES_LABEL},
{"passwordCopiedToClipboard",
IDS_PASSWORD_MANAGER_UI_PASSWORD_COPIED_TO_CLIPBOARD},
Expand Down
1 change: 1 addition & 0 deletions chrome/test/data/webui/password_manager/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ build_webui_tests("build") {
resource_path_prefix = "password_manager"

files = [
"add_password_dialog_test.ts",
"checkup_details_section_test.ts",
"checkup_section_test.ts",
"edit_password_dialog_test.ts",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'chrome://password-manager/password_manager.js';

import {PasswordManagerImpl} from 'chrome://password-manager/password_manager.js';
import {assertEquals, assertFalse, assertTrue} from 'chrome://webui-test/chai_assert.js';
import {flushTasks} from 'chrome://webui-test/polymer_test_util.js';

import {TestPasswordManagerProxy} from './test_password_manager_proxy.js';

suite('AddPasswordDialogTest', function() {
let passwordManager: TestPasswordManagerProxy;

setup(function() {
document.body.innerHTML = window.trustedTypes!.emptyHTML;
passwordManager = new TestPasswordManagerProxy();
PasswordManagerImpl.setInstance(passwordManager);
return flushTasks();
});

test('url validation works', async function() {
const dialog = document.createElement('add-password-dialog');
document.body.appendChild(dialog);
await flushTasks();

assertFalse(dialog.$.websiteInput.invalid);

// Make url invalid
dialog.$.websiteInput.value = 'abc';
dialog.$.websiteInput.dispatchEvent(new CustomEvent('input'));
assertEquals('abc', await passwordManager.whenCalled('getUrlCollection'));
await flushTasks();

assertTrue(dialog.$.websiteInput.invalid);
assertEquals(
dialog.i18n('notValidWebsite'), dialog.$.websiteInput.errorMessage);

// Now make URL valid again
passwordManager.reset();
dialog.$.websiteInput.value = 'www';
dialog.$.websiteInput.dispatchEvent(new CustomEvent('input'));
assertEquals('www', await passwordManager.whenCalled('getUrlCollection'));
await flushTasks();
assertFalse(dialog.$.websiteInput.invalid);

// But after losing focus url is no longer valid due to missing '.'
dialog.$.websiteInput.dispatchEvent(new CustomEvent('blur'));
assertTrue(dialog.$.websiteInput.invalid);
assertEquals(
dialog.i18n('missingTLD', 'www.com'),
dialog.$.websiteInput.errorMessage);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ const PasswordManagerBrowserTest = class extends PolymerTest {
}
};

[['App', 'password_manager_app_test.js'],
[['AddPassword', 'add_password_dialog_test.js'],
['App', 'password_manager_app_test.js'],
['Checkup', 'checkup_section_test.js'],
['CheckupDetails', 'checkup_details_section_test.js'],
['EditPassword', 'edit_password_dialog_test.js'],
['PasswordCard', 'password_details_card_test.js'],
['PasswordDetails', 'password_details_section_test.js'],
['PasswordsExporter', 'passwords_exporter_test.js'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export class TestPasswordManagerProxy extends TestBrowserProxy implements
'getInsecureCredentials',
'getPasswordCheckStatus',
'getSavedPasswordList',
'getUrlCollection',
'muteInsecureCredential',
'recordPasswordCheckInteraction',
'removeBlockedSite',
Expand Down Expand Up @@ -237,4 +238,17 @@ export class TestPasswordManagerProxy extends TestBrowserProxy implements
showExportedFileInShell() {
this.methodCalled('showExportedFileInShell');
}

getUrlCollection(url: string) {
this.methodCalled('getUrlCollection', url);
if (url.includes('www')) {
return Promise.resolve({
signonRealm: `https://${url}/login`,
shown: url,
link: `https://${url}/login`,
});
} else {
return Promise.reject();
}
}
}

0 comments on commit 614d19f

Please sign in to comment.