-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(password): Add ability to set password after third party login to Sync #15602
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,54 @@ | ||||||
<div class="card"> | ||||||
<header class="mb-2"> | ||||||
<h1 id="fxa-signup-password-header" class="card-header"> | ||||||
{{#t}}Set your password{{/t}} | ||||||
</h1> | ||||||
</header> | ||||||
|
||||||
<section> | ||||||
<div class="error"></div> | ||||||
<div class="success"></div> | ||||||
|
||||||
<form novalidate> | ||||||
<p id="prefillEmail" class="text-base break-all mb-5">{{ email }}</p> | ||||||
|
||||||
<p class="mb-5"> | ||||||
{{#t}}Please create a password to continue to Firefox Sync. Your data is encrypted with your password to protect your privacy.{{/t}} | ||||||
</p> | ||||||
|
||||||
<div class="tooltip-container mb-5"> | ||||||
<input name="password" id="password" type="password" class="input-text tooltip-below" placeholder="{{#t}}Password{{/t}}" value="{{ password }}" pattern=".{8,}" required autofocus data-form-prefill="password" data-synchronize-show="true" /> | ||||||
<div id="password-strength-balloon-container" tabindex="-1"></div> | ||||||
</div> | ||||||
|
||||||
<div class="tooltip-container mb-5"> | ||||||
<input name="vpassword" id="vpassword" type="password" class="input-text tooltip-below" placeholder="{{#t}}Repeat password{{/t}}" pattern=".{8,}" required data-synchronize-show="true" /> | ||||||
|
||||||
<div class="input-balloon hidden" id="vpassword-balloon"> | ||||||
<div class="before:content-key flex before:w-8"> | ||||||
<p class="ltr:pl-2 rtl:pr-2"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @LZoog / @vpomerleau can we use
Suggested change
|
||||||
{{#unsafeTranslate}}You need this password to access any encrypted data you store with us.<br class="mb-3">A reset means potentially losing data like passwords and bookmarks.{{/unsafeTranslate}} | ||||||
</p> | ||||||
</div> | ||||||
</div> | ||||||
</div> | ||||||
|
||||||
<header> | ||||||
<h2 id="fxa-choose-what-to-sync-header" class="font-normal mb-5 text-base"> | ||||||
{{#t}}Choose what to sync{{/t}} | ||||||
</h2> | ||||||
</header> | ||||||
<div class="flex flex-wrap text-start ltr:mobileLandscape:ml-6 rtl:mobileLandscape:mr-6 mb-1"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same Q here re:
Suggested change
|
||||||
{{#engines}} | ||||||
<div class="mb-4 relative flex-50% rtl:mobileLandscape:pr-6 ltr:mobileLandscape:pl-6 rtl:pr-3 ltr:pl-3 flex items-center"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. … and here:
Suggested change
Disclaimer: I'm very possibly not correct, so I'll defer to Tailwind aficionados @vpomerleau and @LZoog. |
||||||
<input name="sync-content" class="input-checkbox" id="sync-engine-{{id}}" type="checkbox" {{#checked}}checked="checked"{{/checked}} value="{{id}}" tabindex="{{tabindex}}" /><label class="input-checkbox-label" for="sync-engine-{{id}}">{{text}}</label> | ||||||
</div> | ||||||
{{/engines}} | ||||||
</div> | ||||||
|
||||||
<div class="flex"> | ||||||
<button id="submit-btn" class="cta-primary cta-xl" type="submit">{{#t}}Create Password{{/t}}</button> | ||||||
</div> | ||||||
</form> | ||||||
</section> | ||||||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -206,6 +206,13 @@ export default { | |
this.logFlowEvent(`${provider}.signin-complete`); | ||
|
||
this.metrics.flush(); | ||
|
||
// Sync service requires a password to be set before it can be used. | ||
// Note that once a password is set, the user will not have an option to use | ||
// third party login for Sync since it always requires a password. | ||
if (this.relier.isSync()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also check if the account is missing a password? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yea we should, updating There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested this a few more times, a user won't be able to do social login for Sync. They are almost prompted to enter password if they have it. |
||
return this.navigate('/post_verify/third_party_auth/set_password', { provider }); | ||
} | ||
|
||
return this.signIn(updatedAccount); | ||
}) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,87 @@ | ||||||
/* This Source Code Form is subject to the terms of the Mozilla Public | ||||||
* License, v. 2.0. If a copy of the MPL was not distributed with this | ||||||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||||||
|
||||||
import AuthErrors from '../../../lib/auth-errors'; | ||||||
import Cocktail from '../../../lib/cocktail'; | ||||||
import Template from '../../../templates/post_verify/third_party_auth/set_password.mustache'; | ||||||
import FormView from '../../form'; | ||||||
import FlowEventsMixin from '../../mixins/flow-events-mixin'; | ||||||
import PasswordMixin from '../../mixins/password-mixin'; | ||||||
import PasswordStrengthMixin from '../../mixins/password-strength-mixin'; | ||||||
import CWTSOnSignupPasswordMixin from '../../mixins/cwts-on-signup-password'; | ||||||
import ServiceMixin from '../../mixins/service-mixin'; | ||||||
import AccountSuggestionMixin from '../../mixins/account-suggestion-mixin'; | ||||||
import SigninMixin from '../../mixins/signin-mixin'; | ||||||
|
||||||
const PASSWORD_INPUT_SELECTOR = '#password'; | ||||||
const VPASSWORD_INPUT_SELECTOR = '#vpassword'; | ||||||
Comment on lines
+17
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: should these be fxa/packages/fxa-content-server/app/tests/spec/views/post_verify/third_party_auth/set_password.js Lines 19 to 20 in 3d66e9e
|
||||||
|
||||||
class SetPassword extends FormView { | ||||||
template = Template; | ||||||
|
||||||
_getPassword() { | ||||||
return this.$(PASSWORD_INPUT_SELECTOR).val(); | ||||||
} | ||||||
|
||||||
_getVPassword() { | ||||||
return this.$(VPASSWORD_INPUT_SELECTOR).val(); | ||||||
} | ||||||
|
||||||
isValidEnd() { | ||||||
return this._getPassword() === this._getVPassword(); | ||||||
} | ||||||
|
||||||
showValidationErrorsEnd() { | ||||||
if (this._getPassword() !== this._getVPassword()) { | ||||||
const err = AuthErrors.toError('PASSWORDS_DO_NOT_MATCH'); | ||||||
this.showValidationError(this.$(PASSWORD_INPUT_SELECTOR), err, true); | ||||||
} | ||||||
} | ||||||
|
||||||
getAccount() { | ||||||
return this.getSignedInAccount(); | ||||||
} | ||||||
|
||||||
beforeRender() { | ||||||
const account = this.getSignedInAccount(); | ||||||
if (account.isDefault()) { | ||||||
return this.replaceCurrentPage('/'); | ||||||
} | ||||||
} | ||||||
|
||||||
setInitialContext(context) { | ||||||
const email = this.getAccount().get('email'); | ||||||
context.set({ | ||||||
email, | ||||||
}); | ||||||
} | ||||||
|
||||||
submit() { | ||||||
const account = this.getAccount(); | ||||||
const password = this._getPassword(); | ||||||
|
||||||
return account.createPassword(account.get('email'), password) | ||||||
.then(() => { | ||||||
// After the user has set a password, initiated the standard Sync | ||||||
// login flow with the password they set. | ||||||
return this.signIn(account, password); | ||||||
}); | ||||||
} | ||||||
} | ||||||
|
||||||
Cocktail.mixin( | ||||||
SetPassword, | ||||||
PasswordMixin, | ||||||
CWTSOnSignupPasswordMixin, | ||||||
PasswordStrengthMixin({ | ||||||
balloonEl: '#password-strength-balloon-container', | ||||||
passwordEl: PASSWORD_INPUT_SELECTOR, | ||||||
}), | ||||||
ServiceMixin, | ||||||
AccountSuggestionMixin, | ||||||
FlowEventsMixin, | ||||||
SigninMixin | ||||||
); | ||||||
|
||||||
export default SetPassword; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,147 @@ | ||||||
/* This Source Code Form is subject to the terms of the Mozilla Public | ||||||
* License, v. 2.0. If a copy of the MPL was not distributed with this | ||||||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||||||
|
||||||
import _ from 'underscore'; | ||||||
import { assert } from 'chai'; | ||||||
import Account from 'models/account'; | ||||||
import Backbone from 'backbone'; | ||||||
import BaseBroker from 'models/auth_brokers/base'; | ||||||
import Metrics from 'lib/metrics'; | ||||||
import Relier from 'models/reliers/relier'; | ||||||
import SentryMetrics from 'lib/sentry'; | ||||||
import sinon from 'sinon'; | ||||||
import User from 'models/user'; | ||||||
import View from 'views/post_verify/third_party_auth/set_password'; | ||||||
import WindowMock from '../../../../mocks/window'; | ||||||
import $ from 'jquery'; | ||||||
|
||||||
const PASSWORD_INPUT_SELECTOR = '#password'; | ||||||
const VPASSWORD_INPUT_SELECTOR = '#vpassword'; | ||||||
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left a note above about import/exporting these from fxa/packages/fxa-content-server/app/scripts/views/post_verify/third_party_auth/set_password.js Lines 17 to 18 in 3d66e9e
But maybe they're far enough apart (maybe |
||||||
const PASSWORD = 'passwordzxcv'; | ||||||
|
||||||
describe('views/post_verify/third_party_auth/set_password', () => { | ||||||
let account; | ||||||
let broker; | ||||||
let metrics; | ||||||
let model; | ||||||
let notifier; | ||||||
let relier; | ||||||
let sentryMetrics; | ||||||
let user; | ||||||
let view; | ||||||
let windowMock; | ||||||
|
||||||
beforeEach(() => { | ||||||
windowMock = new WindowMock(); | ||||||
relier = new Relier({ | ||||||
window: windowMock, | ||||||
}); | ||||||
broker = new BaseBroker({ | ||||||
relier, | ||||||
window: windowMock, | ||||||
}); | ||||||
account = new Account({ | ||||||
email: '[email protected]', | ||||||
uid: 'uid', | ||||||
}); | ||||||
model = new Backbone.Model({ | ||||||
account, | ||||||
}); | ||||||
notifier = _.extend({}, Backbone.Events); | ||||||
sentryMetrics = new SentryMetrics(); | ||||||
metrics = new Metrics({ notifier, sentryMetrics }); | ||||||
user = new User(); | ||||||
view = new View({ | ||||||
broker, | ||||||
metrics, | ||||||
model, | ||||||
notifier, | ||||||
relier, | ||||||
user, | ||||||
}); | ||||||
|
||||||
sinon.stub(view, 'getSignedInAccount').callsFake(() => account); | ||||||
sinon | ||||||
.stub(account, 'createPassword') | ||||||
.callsFake(() => Promise.resolve()); | ||||||
sinon | ||||||
.stub(view, 'signIn') | ||||||
.callsFake(() => Promise.resolve()); | ||||||
|
||||||
return view.render().then(() => $('#container').html(view.$el)); | ||||||
}); | ||||||
|
||||||
afterEach(function () { | ||||||
view.remove(); | ||||||
view.destroy(); | ||||||
}); | ||||||
|
||||||
describe('render', () => { | ||||||
it('renders the view', () => { | ||||||
assert.lengthOf(view.$('#fxa-signup-password-header'), 1); | ||||||
assert.include(view.$('#prefillEmail').text(), '[email protected]'); | ||||||
assert.lengthOf(view.$('#fxa-choose-what-to-sync-header'), 1); | ||||||
assert.lengthOf(view.$('#submit-btn'), 1); | ||||||
}); | ||||||
|
||||||
describe('without an account', () => { | ||||||
beforeEach(() => { | ||||||
account = new Account({}); | ||||||
sinon.spy(view, 'navigate'); | ||||||
return view.render(); | ||||||
}); | ||||||
|
||||||
it('redirects to the email first page', () => { | ||||||
assert.isTrue(view.navigate.calledWith('/')); | ||||||
}); | ||||||
}); | ||||||
}); | ||||||
|
||||||
describe('validateAndSubmit', () => { | ||||||
beforeEach(() => { | ||||||
sinon.stub(view, 'submit').callsFake(() => Promise.resolve()); | ||||||
sinon.spy(view, 'showValidationError'); | ||||||
}); | ||||||
|
||||||
describe('with invalid password', () => { | ||||||
beforeEach(() => { | ||||||
view.$(PASSWORD_INPUT_SELECTOR).val(''); | ||||||
return view.validateAndSubmit().then(assert.fail, () => {}); | ||||||
}); | ||||||
|
||||||
it('displays a tooltip, does not call submit', () => { | ||||||
assert.isTrue(view.showValidationError.called); | ||||||
assert.isFalse(view.submit.called); | ||||||
}); | ||||||
}); | ||||||
|
||||||
describe('with valid password', () => { | ||||||
beforeEach(() => { | ||||||
view.$(PASSWORD_INPUT_SELECTOR).val(PASSWORD); | ||||||
view.$(VPASSWORD_INPUT_SELECTOR).val(PASSWORD); | ||||||
return view.validateAndSubmit(); | ||||||
}); | ||||||
|
||||||
it('calls submit', () => { | ||||||
assert.equal(view.submit.callCount, 1); | ||||||
}); | ||||||
}); | ||||||
}); | ||||||
|
||||||
describe('submit', () => { | ||||||
describe('success', () => { | ||||||
beforeEach(() => { | ||||||
sinon.spy(view, 'navigate'); | ||||||
view.$(PASSWORD_INPUT_SELECTOR).val(PASSWORD); | ||||||
view.$(VPASSWORD_INPUT_SELECTOR).val(PASSWORD); | ||||||
return view.submit(); | ||||||
}); | ||||||
|
||||||
it('calls correct methods', () => { | ||||||
assert.isTrue(account.createPassword.calledWith('[email protected]', PASSWORD),); | ||||||
assert.isTrue(view.signIn.calledWith(account, PASSWORD,)); | ||||||
}); | ||||||
}); | ||||||
}); | ||||||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HHmm, I should probably know this, but do these
<input>
fields need a<label>
for a11y reasons? Or maybe a screen-reader-only label field isn't great for a11y-ing./summon my a11y gurus @sardesam and @xlisachan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdehaan I'm gonna defer some of these changes for now, they were taken from other pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There are some exceptions (like a submit button for example), but in general they all should. Placeholders do not replace labels and while seen by sighted customers, they are not heard by screenreaders or other ATs.
As suggested, they can be hidden visually, but they should be included for a11y.