diff --git a/resources/static/dialog/controllers/authenticate.js b/resources/static/dialog/controllers/authenticate.js index a805af2408c42dae48f38909827316a6cae3bc5f..13a1abb08d291e0580f34e3157519ec3119d9250 100644 --- a/resources/static/dialog/controllers/authenticate.js +++ b/resources/static/dialog/controllers/authenticate.js @@ -169,7 +169,9 @@ BrowserID.Modules.Authenticate = (function() { // account automatically have an account created with no further // interaction. To make sure they see the TOS/PP agreement, show it // here. - dialogHelpers.showRPTosPP.call(self); + if (options.siteTOSPP) { + dialogHelpers.showRPTosPP.call(self); + } self.bind("#email", "keyup", emailKeyUp); self.click("#forgotPassword", forgotPassword); diff --git a/resources/static/dialog/controllers/pick_email.js b/resources/static/dialog/controllers/pick_email.js index f0c90f9d79b480cdd61121d090441aed463f7b08..80e0332834eef65ba742787c16b1f24aff08e320 100644 --- a/resources/static/dialog/controllers/pick_email.js +++ b/resources/static/dialog/controllers/pick_email.js @@ -83,7 +83,6 @@ BrowserID.Modules.PickEmail = (function() { var Module = bid.Modules.PageModule.extend({ start: function(options) { var origin = user.getOrigin(), - originEmail = user.getOriginEmail(), self=this; options = options || {}; @@ -96,12 +95,10 @@ BrowserID.Modules.PickEmail = (function() { self.renderDialog("pick_email", { identities: identities, - siteemail: originEmail + siteemail: user.getOriginEmail() }); - // If the user has been to this site before, they have already implicitly - // agreed to TOS/PP agreement. Don't bug them a second time. - if (!originEmail && options.siteTOSPP) { + if (options.siteTOSPP) { dialogHelpers.showRPTosPP.call(self); } diff --git a/resources/static/dialog/controllers/required_email.js b/resources/static/dialog/controllers/required_email.js index 2314717b335c4674741637666b3cb6611db081ff..037621924d000b0b2c5925f1ad34b22efd58f550 100644 --- a/resources/static/dialog/controllers/required_email.js +++ b/resources/static/dialog/controllers/required_email.js @@ -103,7 +103,6 @@ BrowserID.Modules.RequiredEmail = (function() { email = options.email || ""; secondaryAuth = options.secondary_auth; primaryInfo = null; - var siteTOSPP = options.siteTOSPP; function ready() { options.ready && options.ready(); @@ -148,7 +147,13 @@ BrowserID.Modules.RequiredEmail = (function() { // We know the user has control of this address, give them // a chance to hit "sign in" before we kick them off to the // primary flow account. - showTemplate({ signin: true, primary: true }); + + // Show the Persona TOS/PP to any primary user who is authed with + // their IdP but not with Persona. Unfortunately, addressInfo + // does not tell us whether a primary address already has an + // account, so we have to show the personaTOSPP to any user who + // is not authenticated. + showTemplate({ signin: true, primary: true, personaTOSPP: !auth_level }); } else if(info.type === "primary" && !info.authed) { // User who does not control a primary address. @@ -213,25 +218,13 @@ BrowserID.Modules.RequiredEmail = (function() { signin: false, password: false, secondary_auth: false, - primary: false + primary: false, + personaTOSPP: false }, templateData); self.renderDialog("required_email", templateData); - // For the first revision, if the required_email template is shown, the - // Persona TOS/PP agreement will always be shown. The RP TOS/PP will - // be shown if there is no email set for the origin (user has not - // visited site before). - // - // This leaves two important holes in coverage for both Persona and the - // RP. #1) New secondary user. The new secondary user flow is - // redirected to the "new_user" flow - eventually landing the user on - // the "set_password" screen. #2) New primary user who is not - // authenticated with their IdP. These users are passed on to the - // "primary_user" flow. In neither case is the Persona TOS/PP - // agreement shown. - var originEmail = user.getOriginEmail(); - if (!originEmail && siteTOSPP) { + if (options.siteTOSPP) { dialogHelpers.showRPTosPP.call(self); } diff --git a/resources/static/dialog/controllers/set_password.js b/resources/static/dialog/controllers/set_password.js index 7bc6848c59663f9f30ea5f8023feb0e97c9cb984..02c6de1038dd03df26c63c2dc377a9708e8af66f 100644 --- a/resources/static/dialog/controllers/set_password.js +++ b/resources/static/dialog/controllers/set_password.js @@ -36,9 +36,14 @@ BrowserID.Modules.SetPassword = (function() { self.renderDialog("set_password", { password_reset: !!options.password_reset, - cancelable: options.cancelable !== false + cancelable: options.cancelable !== false, + personaTOSPP: options.personaTOSPP }); + if (options.siteTOSPP) { + dialogHelpers.showRPTosPP.call(self); + } + self.click("#cancel", cancel); sc.start.call(self, options); diff --git a/resources/static/dialog/controllers/verify_primary_user.js b/resources/static/dialog/controllers/verify_primary_user.js index 97a6317e2e7c06c2901756b83190bd4c31763763..c92fdb25219fcb4f666c1ff5e3493e41b9f2c7c5 100644 --- a/resources/static/dialog/controllers/verify_primary_user.js +++ b/resources/static/dialog/controllers/verify_primary_user.js @@ -49,10 +49,16 @@ BrowserID.Modules.VerifyPrimaryUser = (function() { email = data.email; auth_url = data.auth_url; - var templateData = helpers.extend({}, data, { - requiredEmail: data.requiredEmail || false + self.renderDialog("verify_primary_user", { + email: data.email, + auth_url: data.auth_url, + requiredEmail: data.requiredEmail || false, + personaTOSPP: data.personaTOSPP }); - self.renderDialog("verify_primary_user", templateData); + + if (data.siteTOSPP) { + dialogHelpers.showRPTosPP.call(self); + } self.click("#cancel", cancel); diff --git a/resources/static/dialog/css/popup.css b/resources/static/dialog/css/popup.css index d157ccf00caf91116ef1fa9e0ac15b30b08cb748..ead1f555b32e3df95c3b37548a342c81d322bf8b 100644 --- a/resources/static/dialog/css/popup.css +++ b/resources/static/dialog/css/popup.css @@ -389,10 +389,6 @@ div#required_email { margin-top: 10px; } -.submit > p + p:last-child { - margin-top: 15px; -} - .tospp { font-size: 11px; line-height: 1.2; diff --git a/resources/static/dialog/resources/state.js b/resources/static/dialog/resources/state.js index 13195aaeaad5525c8940b0243e71dc29e27027ad..27d818159d06513a4df6e1bf980fb00ddc4edba4 100644 --- a/resources/static/dialog/resources/state.js +++ b/resources/static/dialog/resources/state.js @@ -1,9 +1,11 @@ -/*jshint browser:true, jQuery: true, forin: true, laxbreak:true */ +/*jshint browser:true, jquery: true, forin: true, laxbreak:true */ /*global BrowserID: true */ /* 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/. */ BrowserID.State = (function() { + "use strict"; + var bid = BrowserID, storage = bid.Storage, network = bid.Network, @@ -76,7 +78,10 @@ BrowserID.State = (function() { self.email = requiredEmail; startAction("doAuthenticateWithRequiredEmail", { email: requiredEmail, - siteTOSPP: self.siteTOSPP + // New users are handled by either the "new_user" flow or the + // "primary_user" flow. The Persona TOS/PP will be shown to users in + // these stages. + siteTOSPP: self.siteTOSPP && !user.getOriginEmail() }); } else if (authenticated) { @@ -87,7 +92,6 @@ BrowserID.State = (function() { }); handleState("authenticate", function(msg, info) { - info = info || {}; info.siteTOSPP = self.siteTOSPP; startAction("doAuthenticate", info); }); @@ -95,9 +99,23 @@ BrowserID.State = (function() { handleState("new_user", function(msg, info) { self.newUserEmail = info.email; - // cancel is disabled if the user is doing the initial password set - // for a requiredEmail. - info.cancelable = !requiredEmail; + _.extend(info, { + // cancel is disabled if the user is doing the initial password set + // for a requiredEmail. + cancelable: !requiredEmail, + + // New users in the requiredEmail flow are sent directly to the + // set_password screen. If this happens, they have not yet seen the + // TOS/PP agreement. + + // Always show the Persona TOS/PP to new requiredEmail users. + personaTOSPP: !!requiredEmail, + + // The site TOS/PP must be shown to new requiredEmail users if there is + // a site TOS/PP + siteTOSPP: !!requiredEmail && self.siteTOSPP + }); + startAction(false, "doSetPassword", info); }); @@ -161,11 +179,24 @@ BrowserID.State = (function() { }); handleState("primary_user_unauthenticated", function(msg, info) { - info = helpers.extend(info, { + // a user who lands here is not authenticated with their identity + // provider. + _.extend(info, { add: !!addPrimaryUser, email: email, requiredEmail: !!requiredEmail, - siteTOSPP: self.siteTOSPP + + // In the requiredEmail flow, a user who is not authenticated with + // their primary will be sent directly to the "you must verify + // with your IdP" screen. + // + // Show the siteTOSPP to all requiredEmail users who have never visited + // the site before. + siteTOSPP: requiredEmail && self.siteTOSPP && !user.getOriginEmail(), + + // Show the persona TOS/PP only to requiredEmail users who are creating + // a new account. + personaTOSPP: requiredEmail && !addPrimaryUser }); if (primaryVerificationInfo) { @@ -202,7 +233,7 @@ BrowserID.State = (function() { handleState("pick_email", function() { startAction("doPickEmail", { origin: self.hostname, - siteTOSPP: self.siteTOSPP + siteTOSPP: self.siteTOSPP && !user.getOriginEmail() }); }); @@ -241,7 +272,12 @@ BrowserID.State = (function() { startAction("doAuthenticateWithRequiredEmail", { email: email, secondary_auth: true, - siteTOSPP: self.siteTOSPP + + // This is a user is already authenticated to the assertion + // level who has chosen a secondary email address from the + // pick_email screen. They would have been shown the + // siteTOSPP there. + siteTOSPP: false }); } else { @@ -366,9 +402,26 @@ BrowserID.State = (function() { user.passwordNeededToAddSecondaryEmail(function(passwordNeeded) { if(passwordNeeded) { self.addEmailEmail = info.email; - // cancel is disabled if the user is doing the initial password set - // for a requiredEmail. - info.cancelable = !requiredEmail; + + _.extend(info, { + // cancel is disabled if the user is doing the initial password set + // for a requiredEmail. + cancelable: !requiredEmail, + + // stage_email is called to add an email to an already existing + // account. Since it is an already existing account, the + // personaTOSPP does not need to be shown. + personaTOSPP: false, + + // requiredEmail users who are adding an email but must set their + // password will be redirected here without seeing any other + // screens. non-requiredEmail users will have already seen the site + // TOS/PP in the pick-email screen if it was necessary. Since + // requiredEmail users may not have seen the screen before, show it + // here if there is no originEmail. + siteTOSPP: self.siteTOSPP && requiredEmail && !user.getOriginEmail() + }); + startAction(false, "doSetPassword", info); } else { diff --git a/resources/static/dialog/views/required_email.ejs b/resources/static/dialog/views/required_email.ejs index 831b4d3d5586d298e54367523f34de6a5a0854cf..e2b362b5d6022c8c510524a14e7249228041feaa 100644 --- a/resources/static/dialog/views/required_email.ejs +++ b/resources/static/dialog/views/required_email.ejs @@ -50,16 +50,7 @@ </ul> <div class="submit cf"> - <p class="tospp"> - <%= format( - gettext('By proceeding, you agree to %s\'s <a %s>Terms</a> and <a %s>Privacy Policy</a>.'), - [ "Persona", - ' href="https://login.persona.org/tos" target="_new"', - ' href="https://login.persona.org/privacy" target="_new"', - ]) %> - </p> - - <p> + <p class="cf"> <% if (signin) { %> <button id="sign_in" tabindex="3"><%= gettext("sign in") %></button> <% } else if (verify) { %> @@ -70,5 +61,15 @@ <a href="#" id="cancel" class="action" tabindex="4"><%= gettext("cancel") %></a> <% } %> </p> + <% if (personaTOSPP) { %> + <p class="tospp"> + <%= format( + gettext('By proceeding, you agree to %s\'s <a %s>Terms</a> and <a %s>Privacy Policy</a>.'), + [ "Persona", + ' href="https://login.persona.org/tos" target="_new"', + ' href="https://login.persona.org/privacy" target="_new"', + ]) %> + </p> + <% } %> </div> </div> diff --git a/resources/static/dialog/views/set_password.ejs b/resources/static/dialog/views/set_password.ejs index 3cc5e014cee1ea61fed2bad1b7490025d959294c..2d5a3cd48c5cbaae5ca344b1d2b0cc16ae291867 100644 --- a/resources/static/dialog/views/set_password.ejs +++ b/resources/static/dialog/views/set_password.ejs @@ -48,11 +48,23 @@ </ul> <div class="submit cf"> + <p class="cf"> <button tabindex="1" id="<%= password_reset ? "password_reset" : "verify_user" %>"> <%= password_reset ? gettext('reset password') : gettext('verify email') %> </button> <% if(cancelable) { %> <a id="cancel" class="action" href="#"><%= gettext('cancel') %></a> <% } %> + </p> + <% if (personaTOSPP) { %> + <p id="persona_tospp" class="tospp"> + <%= format( + gettext('By proceeding, you agree to %s\'s <a %s>Terms</a> and <a %s>Privacy Policy</a>.'), + [ "Persona", + ' href="https://login.persona.org/tos" target="_new"', + ' href="https://login.persona.org/privacy" target="_new"', + ]) %> + </p> + <% } %> </div> </div> diff --git a/resources/static/dialog/views/verify_primary_user.ejs b/resources/static/dialog/views/verify_primary_user.ejs index a0cdffb03374e422dc3c0e38f74e8bedfb2d7591..cad3c7e4ea5652b143a36f96051d8bdb41164fd8 100644 --- a/resources/static/dialog/views/verify_primary_user.ejs +++ b/resources/static/dialog/views/verify_primary_user.ejs @@ -4,7 +4,7 @@ <% if (requiredEmail) { %> <strong><%= gettext('The site requested you sign in using') %></strong> - <div class="form_section"> + <div id="verify_primary_user" class="cf form_section"> <ul class="inputs"> <li> @@ -15,7 +15,7 @@ </div> </li> - <li> + <li class="small"> <%= gettext("You must sign in with your email provider to verify ownership of this address. This window will be redirected to") %> <p> <strong><%= auth_url %></strong>. @@ -24,26 +24,49 @@ </ul> <div class="submit cf"> - <p> + <p class="cf"> <button id="verifyWithPrimary"><%= gettext("verify") %></button> </p> + + <% if (personaTOSPP) { %> + <p id="persona_tospp" class="tospp"> + <%= format( + gettext('By proceeding, you agree to %s\'s <a %s>Terms</a> and <a %s>Privacy Policy</a>.'), + [ "Persona", + ' href="https://login.persona.org/tos" target="_new"', + ' href="https://login.persona.org/privacy" target="_new"', + ]) %> + </p> + <% } %> </div> </div> <% } else { %> <strong><%= gettext("Verify With Email Provider") %></strong> - <div class="cf form_section"> - <%= gettext("You must sign in with your email provider to verify ownership of this address. This window will be redirected to") %> - <p> - <strong><%= auth_url %></strong>. - </p> + <div id="verify_primary_user" class="cf form_section"> + <div class="small"> + <%= gettext("You must sign in with your email provider to verify ownership of this address. This window will be redirected to") %> + <p> + <strong><%= auth_url %></strong>. + </p> + </div> <div class="submit cf"> - <p> + <p class="cf"> <button id="verifyWithPrimary"><%= gettext("verify") %></button> <a href="#" id="cancel" class="action"><%= gettext("cancel") %></a> </p> + <% if (personaTOSPP) { %> + <p id="persona_tospp" class="tospp"> + <%= format( + gettext('By proceeding, you agree to %s\'s <a %s>Terms</a> and <a %s>Privacy Policy</a>.'), + [ "Persona", + ' href="https://login.persona.org/tos" target="_new"', + ' href="https://login.persona.org/privacy" target="_new"', + ]) %> + </p> + <% } %> </div> </div> diff --git a/resources/static/test/cases/controllers/required_email.js b/resources/static/test/cases/controllers/required_email.js index 5f5683748108127bd2e17dc6d3565d91c3e35c3e..97800512f5c0df71e88c4529c20d7fc3e71b3658 100644 --- a/resources/static/test/cases/controllers/required_email.js +++ b/resources/static/test/cases/controllers/required_email.js @@ -95,7 +95,7 @@ } - asyncTest("siteTOSPP specified, no origin email - show TOS/PP", function() { + asyncTest("siteTOSPP specified - show TOS/PP", function() { var email = "registered@testuser.com"; xhr.useResult("known_secondary"); xhr.setContextInfo("auth_level", "password"); @@ -110,40 +110,6 @@ }); }); - asyncTest("siteTOSPP specified, origin has email - do not show TOS/PP", function() { - var email = "registered@testuser.com"; - xhr.useResult("known_secondary"); - xhr.setContextInfo("auth_level", "password"); - - storage.addSecondaryEmail(email); - user.setOriginEmail(email); - - createController({ - email: email, - siteTOSPP: true, - ready: function() { - testHelpers.testRPTosPPNotShown(); - start(); - } - }); - }); - - asyncTest("siteTOSPP not specified, no origin email - do not show TOS/PP", function() { - var email = "registered@testuser.com"; - xhr.useResult("known_secondary"); - xhr.setContextInfo("auth_level", "password"); - - createController({ - email: email, - siteTOSPP: false, - ready: function() { - testHelpers.testRPTosPPNotShown(); - start(); - } - }); - }); - - asyncTest("known_secondary: user who is not authenticated - show password form", function() { var email = "registered@testuser.com"; xhr.useResult("known_secondary"); diff --git a/resources/static/test/cases/controllers/set_password.js b/resources/static/test/cases/controllers/set_password.js index a1ae3b3c683434ebf000678e3b77943fbaf15d82..0d64302f07c16ac0042558e60dd26840fdc97db3 100644 --- a/resources/static/test/cases/controllers/set_password.js +++ b/resources/static/test/cases/controllers/set_password.js @@ -10,6 +10,8 @@ el = $("body"), bid = BrowserID, testHelpers = bid.TestHelpers, + testElementExists = testHelpers.testElementExists, + testElementNotExists = testHelpers.testElementDoesNotExist, register = testHelpers.register, controller; @@ -33,22 +35,29 @@ test("create with no options - show template, user must verify email, can cancel", function() { ok($("#set_password").length, "set_password template added"); - equal($("#verify_user").length, 1, "correct button shown"); - equal($("#cancel").length, 1, "cancel button shown"); + testElementExists("#verify_user"); + testElementExists("#cancel"); + testElementNotExists("#persona_tospp"); }); test("create with password_reset option - show template, show reset password button", function() { controller.destroy(); createController({ password_reset: true }); - ok($("#set_password").length, "set_password template added"); - equal($("#password_reset").length, 1, "correct button shown"); - equal($("#cancel").length, 1, "cancel button shown"); + testElementExists("#set_password"); + testElementExists("#password_reset"); + testElementExists("#cancel"); + }); + + test("create with personaTOSPP option - show Persona TOS/PP", function() { + controller.destroy(); + createController({ personaTOSPP: true }); + testElementExists("#persona_tospp"); }); test("create with cancelable=false option - cancel button not shown", function() { controller.destroy(); createController({ cancelable: false }); - equal($("#cancel").length, 0, "cancel button not shown"); + testElementNotExists("#cancel"); }); asyncTest("submit with good password/vpassword - password_set message raised", function() { diff --git a/resources/static/test/cases/controllers/verify_primary_user.js b/resources/static/test/cases/controllers/verify_primary_user.js index de8da5ad94b8b5f25064f5499011a2ecad185421..bde1c0d47b4396c0433077e6796ed4d4b6ad568b 100644 --- a/resources/static/test/cases/controllers/verify_primary_user.js +++ b/resources/static/test/cases/controllers/verify_primary_user.js @@ -10,6 +10,8 @@ controller, el, testHelpers = bid.TestHelpers, + testElementExists = testHelpers.testElementExists, + testElementNotExists = testHelpers.testElementDoesNotExist, WindowMock = bid.Mocks.WindowMock, win, mediator = bid.Mediator; @@ -33,14 +35,45 @@ } }); + test("personaTOSPP true, requiredEmail: true - show TOS/PP", function() { + createController({ + window: win, + add: false, + email: "unregistered@testuser.com", + auth_url: "http://testuser.com/sign_in", + requiredEmail: true, + personaTOSPP: false + }); + + testElementNotExists("#persona_tospp"); + }); + + test("personaTOSPP true, requiredEmail: false - show TOS/PP", function() { + createController({ + window: win, + add: false, + email: "unregistered@testuser.com", + auth_url: "http://testuser.com/sign_in", + requiredEmail: false, + personaTOSPP: false + }); + + testElementNotExists("#persona_tospp"); + }); + + asyncTest("submit with `add: false` option opens a new tab with CREATE_EMAIL URL", function() { var messageTriggered = false; createController({ window: win, add: false, email: "unregistered@testuser.com", - auth_url: "http://testuser.com/sign_in" + auth_url: "http://testuser.com/sign_in", + personaTOSPP: true }); + + testElementExists("#persona_tospp"); + mediator.subscribe("primary_user_authenticating", function() { messageTriggered = true; }); @@ -61,9 +94,12 @@ window: win, add: true, email: "unregistered@testuser.com", - auth_url: "http://testuser.com/sign_in" + auth_url: "http://testuser.com/sign_in", + personaTOSPP: true }); + testElementExists("#persona_tospp"); + // Also checking to make sure the NATIVE is stripped out. win.document.location.href = "sign_in"; win.document.location.hash = "#NATIVE"; diff --git a/resources/static/test/cases/resources/state.js b/resources/static/test/cases/resources/state.js index b5a4ebca8f08eb3149bcd92a2bacde4da10f40f5..82b5f4c79d92b82a07db304af8d2132f4b02ae19 100644 --- a/resources/static/test/cases/resources/state.js +++ b/resources/static/test/cases/resources/state.js @@ -439,7 +439,7 @@ mediator.publish("email_chosen", { email: email, complete: function() { - testActionStarted("doAuthenticateWithRequiredEmail", { siteTOSPP: true }); + testActionStarted("doAuthenticateWithRequiredEmail", { siteTOSPP: false }); start(); } }); diff --git a/resources/static/test/testHelpers/helpers.js b/resources/static/test/testHelpers/helpers.js index 882d0049e0470879e1f031c1add47717f60d80cf..1fced179ca9d9c550d7ef0156502872b2e052eb2 100644 --- a/resources/static/test/testHelpers/helpers.js +++ b/resources/static/test/testHelpers/helpers.js @@ -247,7 +247,7 @@ BrowserID.TestHelpers = (function() { }, testElementDoesNotExist: function(selector, msg) { - equal($(selector).length, 0, msg || ("element '" + selector + "' does not exist")); + ok(!$(selector).length, msg || ("element '" + selector + "' does not exist")); }, testRPTosPPShown: function(msg) {