Commit f3447f31 authored by Maciek Slusarczyk's avatar Maciek Slusarczyk Committed by Commit Bot

Change in SAML password change flow that addresses b/145206636.

Current implementation of SAML password change flow has 2 issues and does not
work for Okta IdP:

* Empirically chosen timeout used by Authenticator before communicating
injected code on IdP page is too short. As a result injected code sometimes
does not know where to send the message back.
* Password scraping does not work for Okta since it is based on the number of
scraped passwords and Okta has 2 old (the one used to authenticate is re-typed)
and 2 updated (new + verified). As a result code is not able to determine how
to change local credentials and asks user for confirmation every time.

2 changes has been introduced in order to address the issue: timeout was
extended to 2s and new logic based on password field ids replaces existing
logic for Okta.

Bug: 145206636
Change-Id: I0e5083911fd129e4c04e76c07ef5f90cfa5039e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1943140
Commit-Queue: Maciek Slusarczyk <mslus@chromium.org>
Reviewed-by: default avatarRoman Sorokin [CET] <rsorokin@chromium.org>
Reviewed-by: default avatarXiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721929}
parent 4831b60a
...@@ -256,11 +256,33 @@ cr.define('cr.samlPasswordChange', function() { ...@@ -256,11 +256,33 @@ cr.define('cr.samlPasswordChange', function() {
/** /**
* Sends scraped password and resets the state. * Sends scraped password and resets the state.
* @param {bool} isOkta whether the page is Okta page.
* @private * @private
*/ */
onPasswordChangeSuccess_() { onPasswordChangeSuccess_(isOkta) {
const passwordsOnce = this.samlHandler_.getPasswordsScrapedTimes(1); let passwordsOnce;
const passwordsTwice = this.samlHandler_.getPasswordsScrapedTimes(2); let passwordsTwice;
if (isOkta) {
passwordsOnce = this.samlHandler_.getPasswordsWithPropertyScrapedTimes(
1, 'oldPassword');
const newPasswords =
this.samlHandler_.getPasswordsWithPropertyScrapedTimes(
1, 'newPassword');
const verifyPasswords =
this.samlHandler_.getPasswordsWithPropertyScrapedTimes(
1, 'verifyPassword');
if (newPasswords.length == 1 && verifyPasswords.length == 1 &&
newPasswords[0] === verifyPasswords[0]) {
passwordsTwice = Array.from(newPasswords);
} else {
passwordsTwice = [];
}
} else {
passwordsOnce =
this.samlHandler_.getPasswordsWithPropertyScrapedTimes(1);
passwordsTwice =
this.samlHandler_.getPasswordsWithPropertyScrapedTimes(2);
}
this.dispatchEvent(new CustomEvent('authCompleted', { this.dispatchEvent(new CustomEvent('authCompleted', {
detail: { detail: {
...@@ -279,7 +301,6 @@ cr.define('cr.samlPasswordChange', function() { ...@@ -279,7 +301,6 @@ cr.define('cr.samlPasswordChange', function() {
this.webview_.focus(); this.webview_.focus();
} }
/** /**
* Invoked when a new document loading completes. * Invoked when a new document loading completes.
* @param {Object} details The web-request details. * @param {Object} details The web-request details.
...@@ -289,7 +310,7 @@ cr.define('cr.samlPasswordChange', function() { ...@@ -289,7 +310,7 @@ cr.define('cr.samlPasswordChange', function() {
if (details.method == 'POST' && if (details.method == 'POST' &&
detectPasswordChangeSuccess( detectPasswordChangeSuccess(
safeParseUrl_(details.url), safeParseUrl_(details.redirectUrl))) { safeParseUrl_(details.url), safeParseUrl_(details.redirectUrl))) {
this.onPasswordChangeSuccess_(); this.onPasswordChangeSuccess_(false /* isOkta != OKTA */);
} }
} }
...@@ -302,9 +323,11 @@ cr.define('cr.samlPasswordChange', function() { ...@@ -302,9 +323,11 @@ cr.define('cr.samlPasswordChange', function() {
// Okta_detect_success_injected.js needs to be contacted by the parent, // Okta_detect_success_injected.js needs to be contacted by the parent,
// so that it can send messages back to the parent. // so that it can send messages back to the parent.
// Using setTimeout gives the page time to finish initializing. // Using setTimeout gives the page time to finish initializing.
// TODO: timeout value is chosen empirically, we need a better way
// to pass this to the injected code.
setTimeout(() => { setTimeout(() => {
this.webview_.contentWindow.postMessage('connect', details.url); this.webview_.contentWindow.postMessage('connect', details.url);
}, 1000); }, 2000);
} }
/** /**
...@@ -316,7 +339,7 @@ cr.define('cr.samlPasswordChange', function() { ...@@ -316,7 +339,7 @@ cr.define('cr.samlPasswordChange', function() {
if (event.data == 'passwordChangeSuccess') { if (event.data == 'passwordChangeSuccess') {
const pageProvider = detectProvider_(safeParseUrl_(event.origin)); const pageProvider = detectProvider_(safeParseUrl_(event.origin));
if (pageProvider == PasswordChangePageProvider.OKTA) { if (pageProvider == PasswordChangePageProvider.OKTA) {
this.onPasswordChangeSuccess_(); this.onPasswordChangeSuccess_(true /* isOkta == OKTA */);
} }
} }
} }
......
...@@ -307,12 +307,16 @@ cr.define('cr.login', function() { ...@@ -307,12 +307,16 @@ cr.define('cr.login', function() {
} }
/** /**
* Gets the list of passwords which were scpared exactly |times| times. * Gets the list of passwords which have matching passwordProperty and
* are scraped exactly |times| times.
* @return {Array<string>} * @return {Array<string>}
*/ */
getPasswordsScrapedTimes(times) { getPasswordsWithPropertyScrapedTimes(times, passwordProperty) {
const passwords = {}; const passwords = {};
for (const property in this.passwordStore_) { for (const property in this.passwordStore_) {
if (passwordProperty && !property.match(passwordProperty)) {
continue;
}
const key = this.passwordStore_[property]; const key = this.passwordStore_[property];
passwords[key] = (passwords[key] + 1) || 1; passwords[key] = (passwords[key] + 1) || 1;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment