Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Commit

Permalink
Fixes #4959 - Login flow should work from a non-Firefox browser (with…
Browse files Browse the repository at this point in the history
… no extension)

Needs better abstractions and a close look at all the references to deviceId.
Needs to be tested across all the use cases we care about.
Should fix the confirm-login cookie issue, so both cases (browser and extension) log in similarly
  • Loading branch information
ianb authored and punamdahiya committed Oct 25, 2018
1 parent 06af967 commit 791fe5d
Show file tree
Hide file tree
Showing 27 changed files with 428 additions and 179 deletions.
2 changes: 1 addition & 1 deletion locales/en-US/server.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ homePageCookiesLink = Cookies

leavePageRemoveAllData = Remove All Data
# Note: do not translate 'Firefox Screenshots' when translating this string
leavePageErrorAddonRequired = You must have Firefox Screenshots installed to delete your account
leavePageErrorAuthRequired = You must have Firefox Screenshots installed or signed in to Firefox Account to delete your account
leavePageErrorGeneric = An error occurred
# Note: do not translate 'Firefox Screenshots' when translating this string
leavePageWarning = This will permanently erase all of your Firefox Screenshots data.
Expand Down
1 change: 1 addition & 0 deletions server/db-patches/patch-27-28.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE states DROP CONSTRAINT states_deviceid_fkey;
4 changes: 4 additions & 0 deletions server/db-patches/patch-28-27.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
DELETE FROM STATES WHERE DEVICEID NOT IN
(SELECT DEVICEID FROM STATES, DEVICES WHERE STATES.DEVICEID = DEVICES.ID);
ALTER TABLE ONLY states
ADD CONSTRAINT states_deviceid_fkey FOREIGN KEY (deviceid) REFERENCES devices(id) ON DELETE CASCADE;
4 changes: 2 additions & 2 deletions server/src/ad-banner.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ exports.AdBanner = class AdBanner extends React.Component {
render() {
let bannerContent = null;

if (this.props.shouldGetFirefox) {
if (this.props.shouldGetFirefox && !this.props.isOwner) {
const upsellLink = <a className="upsellLink"
href="https://www.mozilla.org/firefox/new/?utm_source=screenshots.firefox.com&utm_medium=referral&utm_campaign=screenshots-acquisition&utm_content=from-shot"
onClick={ this.clickedInstallFirefox.bind(this) }>Get Firefox now</a>;
Expand All @@ -24,7 +24,7 @@ exports.AdBanner = class AdBanner extends React.Component {
Screenshots made simple. Take, save and share screenshots without leaving Firefox. {upsellLink}
</p>
</Localized>;
} else if (this.props.isOwner && !this.props.hasFxa) {
} else if (!this.props.hasFxa) {
bannerContent = <Localized id="bannerMessage">
<p>
Sign in or sign up to sync shots across devices, save your favorite shots forever.
Expand Down
4 changes: 2 additions & 2 deletions server/src/dbschema.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ const mozlog = require("./logging").mozlog("dbschema");

// When updating the database, please also run ./bin/dumpschema --record
// This updates schema.sql with the latest full database schema
const MAX_DB_LEVEL = exports.MAX_DB_LEVEL = 27;
const MAX_DB_LEVEL = exports.MAX_DB_LEVEL = 28;
// This is a list of all the Keygrip scopes we allow (and we make sure these exist):
const KEYGRIP_SCOPES = ["auth", "proxy-url", "download-url", "ga-user-nonce"];
const KEYGRIP_SCOPES = ["auth", "proxy-url", "download-url", "ga-user-nonce", "fxa-oauth"];

exports.forceDbVersion = function(version) {
mozlog.info("forcing-db-version", {db: db.constr, version});
Expand Down
6 changes: 2 additions & 4 deletions server/src/pages/homepage/homepage-header.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@ exports.HomePageHeader = class HomePageHeader extends React.Component {

renderFxASignIn() {
return (
this.props.isFirefox && this.props.isOwner ?
<SignInButton isAuthenticated={this.props.hasFxa} initialPage=""
staticLink={this.props.staticLink} /> : null
<SignInButton isFxaAuthenticated={this.props.hasFxa} initialPage=""
staticLink={this.props.staticLink} />
);
}

Expand Down Expand Up @@ -48,6 +47,5 @@ exports.HomePageHeader = class HomePageHeader extends React.Component {
exports.HomePageHeader.propTypes = {
hasFxa: PropTypes.bool,
isOwner: PropTypes.bool,
isFirefox: PropTypes.bool,
staticLink: PropTypes.func,
};
1 change: 0 additions & 1 deletion server/src/pages/homepage/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ exports.createModel = function(req) {
const firefoxVersion = exports.getFirefoxVersion(req.headers["user-agent"]);
const model = {
title: "Firefox Screenshots",
showMyShots: !!req.deviceId,
isFirefox: !!firefoxVersion && !isMobile,
firefoxVersion,
};
Expand Down
5 changes: 2 additions & 3 deletions server/src/pages/homepage/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,7 @@ class Body extends React.Component {
return (
<reactruntime.BodyTemplate {...this.props}>
<HomePageHeader
isOwner={this.props.showMyShots}
isFirefox={this.props.isFirefox}
isOwner={this.props.authenticated}
hasFxa={this.props.hasFxa}
staticLink={this.props.staticLink}
/>
Expand Down Expand Up @@ -269,7 +268,7 @@ Body.propTypes = {
hasFxa: PropTypes.bool,
firefoxVersion: PropTypes.string,
isFirefox: PropTypes.bool,
showMyShots: PropTypes.bool,
authenticated: PropTypes.bool,
staticLink: PropTypes.func,
};

Expand Down
8 changes: 4 additions & 4 deletions server/src/pages/leave-screenshots/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,17 @@ app.get("/", function(req, res) {
if (req.query && req.query.complete !== undefined) {
res.clearCookie("_csrf");
}
if (!req.deviceId) {
res.status(403).send(req.getText("leavePageErrorAddonRequired"));
if (!(req.deviceId || req.accountId)) {
res.status(403).send(req.getText("leavePageErrorAuthRequired"));
return;
}
const page = require("./page").page;
reactrender.render(req, res, page);
});

app.post("/leave", function(req, res) {
if (!req.deviceId) {
res.status(403).send(req.getText("leavePageErrorAddonRequired"));
if (!(req.deviceId || req.accountId)) {
res.status(403).send(req.getText("leavePageErrorAuthRequired"));
}
Shot.deleteEverythingForDevice(req.backend, req.deviceId, req.accountId).then(() => {
res.redirect("/leave-screenshots/?complete");
Expand Down
6 changes: 3 additions & 3 deletions server/src/pages/settings/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@ exports.createModel = function(req) {
title,
docTitle: title,
};

return db.select(`
SELECT accounts.avatarurl, accounts.nickname, accounts.email FROM accounts, devices
WHERE accounts.id = devices.accountid
AND devices.id = $1
WHERE accounts.id = $1
`,
[req.deviceId]
[req.accountId]
).then((rows) => {
if (rows.length) {
model.accountInfo = {
Expand Down
2 changes: 1 addition & 1 deletion server/src/pages/settings/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const app = express();
exports.app = app;

app.get("/", function(req, res) {
if (!req.deviceId) {
if (!(req.deviceId || req.accountId)) {
res.status(403).send("You must have Screenshots installed");
return;
}
Expand Down
6 changes: 2 additions & 4 deletions server/src/pages/shot/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,10 @@ exports.createModel = function(req) {
id: req.shot.id,
productName: req.config.productName,
isExtInstalled: !!req.deviceId,
isOwner: req.deviceId === req.shot.ownerId || isFxaAuthenticated,
isOwner: req.shot.isOwner,
isFxaAuthenticated,
gaId: req.config.gaId,
deviceId: req.deviceId,
authenticated: !!req.deviceId,
buildTime,
simple: false,
shotDomain: req.url, // FIXME: should be a property of the shot
Expand All @@ -57,12 +56,11 @@ exports.createModel = function(req) {
id: req.shot.id,
productName: req.config.productName,
isExtInstalled: !!req.deviceId,
isOwner: req.deviceId === req.shot.ownerId || isFxaAuthenticated,
isOwner: req.shot.isOwner,
isFxaAuthenticated,
gaId: req.config.gaId,
deviceId: req.deviceId,
shotAccountId: req.shot.accountId,
authenticated: !!req.deviceId,
shotDomain: req.url,
urlIfDeleted: req.shot.urlIfDeleted,
expireTime: req.shot.expireTime === null ? null : req.shot.expireTime.getTime(),
Expand Down
4 changes: 2 additions & 2 deletions server/src/pages/shot/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ exports.app = app;

app.get("/:id/:domain", function(req, res) {
const shotId = `${req.params.id}/${req.params.domain}`;
Shot.get(req.backend, shotId).then((shot) => {
Shot.get(req.backend, shotId, req.deviceId, req.accountId).then((shot) => {
const noSuchShot = !shot;
const nonOwnerAndBlocked = shot && shot.blockType !== "none" && req.deviceId !== shot.ownerId;
const nonOwnerAndBlocked = shot && shot.blockType !== "none" && !shot.isOwner;
if (noSuchShot || nonOwnerAndBlocked || shot.deleted) {
mozlog.info("shot-404", {shotId, ip: req.ip});
notFound(req, res);
Expand Down
12 changes: 7 additions & 5 deletions server/src/pages/shot/shotpage-header.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,15 @@ exports.ShotPageHeader = class ShotPageHeader extends React.Component {
}

renderFxASignIn() {
return (
this.props.isOwner ?
if (this.props.isOwner) {
return (
<div className="shot-fxa-signin">
<SignInButton isAuthenticated={this.props.isFxaAuthenticated} initialPage={this.props.shot.id}
<SignInButton isFxaAuthenticated={this.props.isFxaAuthenticated} initialPage={this.props.shot.id}
staticLink={this.props.staticLink} />
</div> : null
);
</div>
);
}
return null;
}

render() {
Expand Down
4 changes: 2 additions & 2 deletions server/src/pages/shotindex/controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const queryParamModelPropertyMap = {
};

exports.launch = function(m) {
if (m.hasDeviceId) {
if (m.authenticated) {
if (m.shots) {
m.shots = m.shots.map((shot) => {
const s = new AbstractShot(m.backend, shot.id, shot.json);
Expand All @@ -31,7 +31,7 @@ exports.launch = function(m) {

if (window.wantsauth) {
// Handle non-owner my shots page redirection
if (!m.hasDeviceId) {
if (!m.authenticated) {
if (window.wantsauth.getAuthData() && !location.search.includes("reloaded")) {
location.search = "reloaded";
return;
Expand Down
1 change: 0 additions & 1 deletion server/src/pages/shotindex/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ exports.createModel = function(req) {
}
const serverModel = {
title,
hasDeviceId: req.deviceId !== undefined,
defaultSearch: query || null,
};
serverModel.shotsPerPage = req.shotsPerPage;
Expand Down
8 changes: 4 additions & 4 deletions server/src/pages/shotindex/myshots-header.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ const { SignInButton } = require("../../signin-button.js");
const { Header } = require("../../header.js");

exports.MyShotsHeader = function MyShotsHeader(props) {
const signin = props.enableUserSettings && props.hasDeviceId ?
const signin = props.enableUserSettings && props.authenticated ?
<SignInButton
isAuthenticated={props.hasFxa} initialPage="shots"
isFxaAuthenticated={props.hasFxa} initialPage="shots"
staticLink={props.staticLink} /> : null;

return (
<Header hasLogo={true} isOwner={props.hasDeviceId} hasFxa={props.hasFxa}>
<Header hasLogo={true} isOwner={props.authenticated} hasFxa={props.hasFxa}>
<div className="alt-actions">
{ signin }
</div>
Expand All @@ -20,7 +20,7 @@ exports.MyShotsHeader = function MyShotsHeader(props) {

exports.MyShotsHeader.propTypes = {
hasFxa: PropTypes.bool,
hasDeviceId: PropTypes.bool,
authenticated: PropTypes.bool,
enableUserSettings: PropTypes.bool,
staticLink: PropTypes.func,
};
4 changes: 2 additions & 2 deletions server/src/pages/shotindex/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ const app = express();
exports.app = app;

app.get("/", function(req, res) {
if (!req.deviceId) {
if (!(req.deviceId || req.accountId)) {
_render();
return;
}
const pageNumber = req.query.p || 1;
const query = req.query.q || null;
let getShotsPage = Promise.resolve(Shot.emptyShotsPage);
if (req.deviceId && req.query.withdata) {
if ((req.deviceId || req.accountId) && req.query.withdata) {
getShotsPage = Shot.getShotsForDevice(req.backend, req.deviceId, req.accountId, query, pageNumber);
}
getShotsPage.then(_render)
Expand Down
13 changes: 8 additions & 5 deletions server/src/pages/shotindex/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class Body extends React.Component {
<reactruntime.BodyTemplate {...this.props}>
<div className="column-space full-height" id="shot-index-page">
<MyShotsHeader
hasDeviceId={this.props.hasDeviceId} hasFxa={this.props.hasFxa}
authenticated={this.props.authenticated} hasFxa={this.props.hasFxa}
enableUserSettings={this.props.enableUserSettings} staticLink={this.props.staticLink} />
{ this.props.disableSearch ? null : this.renderSearchForm() }
<div id="shot-index" className="flex-1">
Expand All @@ -62,12 +62,16 @@ class Body extends React.Component {
const children = [];
if (this.props.shots && this.props.shots.length) {
for (const shot of this.props.shots) {
children.push(<Card shot={shot} downloadUrl={this.props.downloadUrls[shot.id]} abTests={this.props.abTests} clipUrl={shot.urlDisplay} isOwner={this.props.isOwner} staticLink={this.props.staticLink} isExtInstalled={this.props.isExtInstalled} hasFxa={this.props.hasFxa} key={shot.id} />);
children.push(<Card shot={shot} downloadUrl={this.props.downloadUrls[shot.id]}
abTests={this.props.abTests} clipUrl={shot.urlDisplay}
isOwner={this.props.authenticated} staticLink={this.props.staticLink}
isExtInstalled={this.props.isExtInstalled}
hasFxa={this.props.hasFxa} key={shot.id} />);
}
}

if (children.length === 0) {
if (!this.props.hasDeviceId) {
if (!this.props.authenticated) {
children.push(this.renderNoDeviceId());
} else if (this.props.defaultSearch) {
children.push(this.renderNoSearchResults());
Expand Down Expand Up @@ -276,10 +280,9 @@ Body.propTypes = {
disableSearch: PropTypes.bool,
downloadUrls: PropTypes.object,
enableUserSettings: PropTypes.bool,
hasDeviceId: PropTypes.bool,
authenticated: PropTypes.bool,
hasFxa: PropTypes.bool,
isExtInstalled: PropTypes.bool,
isOwner: PropTypes.bool,
pageNumber: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
shots: PropTypes.array,
shotsPerPage: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
Expand Down
4 changes: 2 additions & 2 deletions server/src/reactrender.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ exports.render = function(req, res, page) {
let serverModel = model.serverModel || model;
const csrfToken = req.csrfToken && req.csrfToken();
jsonModel = Object.assign({
authenticated: !!req.deviceId,
authenticated: !!(req.deviceId || req.accountId),
hasFxa: !!req.accountId,
sentryPublicDSN: req.config.sentryPublicDSN,
backend: req.backend,
Expand All @@ -36,7 +36,7 @@ exports.render = function(req, res, page) {
parseMarkup,
}, jsonModel);
serverModel = Object.assign({
authenticated: !!req.deviceId,
authenticated: !!(req.deviceId || req.accountId),
hasFxa: !!req.accountId,
sentryPublicDSN: req.config.sentryPublicDSN,
staticLink: req.staticLink,
Expand Down
Loading

0 comments on commit 791fe5d

Please sign in to comment.