From a547ce775341a1056e6867bb94ac22d78c98ff54 Mon Sep 17 00:00:00 2001 From: Stuart Colville Date: Thu, 14 Jul 2016 23:09:34 +0100 Subject: [PATCH 1/3] Add accept-language support --- config/default-search.js | 2 + config/default.js | 2 + package.json | 20 ++- src/amo/routes.js | 3 +- src/core/client/base.js | 4 +- src/core/i18n/utils.js | 97 ++++++++++++-- src/core/server/base.js | 17 ++- src/disco/routes.js | 4 +- tests/client/core/i18n/test_utils.js | 183 +++++++++++++++++++++++---- tests/server/disco/TestCSPConfig.js | 14 +- tests/server/disco/TestViews.js | 38 +++--- tests/server/helpers.js | 10 +- tests/server/search/TestViews.js | 2 +- 13 files changed, 313 insertions(+), 83 deletions(-) diff --git a/config/default-search.js b/config/default-search.js index 4a6155bac5d..e72f8b6ff06 100644 --- a/config/default-search.js +++ b/config/default-search.js @@ -18,4 +18,6 @@ module.exports = { ], }, }, + + redirectLangPrefix: false, }; diff --git a/config/default.js b/config/default.js index 5f60d57c8ac..e2848f779fb 100644 --- a/config/default.js +++ b/config/default.js @@ -137,6 +137,8 @@ module.exports = { }, rtlLangs: ['ar', 'dbr', 'fa', 'he'], defaultLang: 'en-US', + redirectLangPrefix: true, + localeDir: path.resolve(path.join(__dirname, '../locale')), // This is off by default diff --git a/package.json b/package.json index 027876433fc..0f5fa50a33c 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "eslint": "eslint .", "stylelint": "stylelint --syntax scss **/*.scss", "lint": "npm run eslint && npm run stylelint", - "servertest": "ADDONS_FRONTEND_BUILD_ALL=1 npm run build && better-npm-run servertest", + "servertest": "ADDONS_FRONTEND_BUILD_ALL=1 npm run build && better-npm-run servertest && better-npm-run servertest:disco && better-npm-run servertest:search", "start": "npm run version-check && NODE_PATH='./:./src' node bin/server.js", "test": "better-npm-run test", "unittest": "better-npm-run unittest", @@ -63,12 +63,28 @@ } }, "servertest": { - "command": "mocha --compilers js:babel-register --timeout 10000 --recursive tests/server/", + "command": "mocha --compilers js:babel-register --timeout 10000 tests/server/", "env": { "NODE_PATH": "./:./src", "NODE_ENV": "production" } }, + "servertest:disco": { + "command": "mocha --compilers js:babel-register --timeout 10000 tests/server/disco", + "env": { + "NODE_PATH": "./:./src", + "NODE_ENV": "production", + "NODE_APP_INSTANCE": "disco" + } + }, + "servertest:search": { + "command": "mocha --compilers js:babel-register --timeout 10000 tests/server/search", + "env": { + "NODE_PATH": "./:./src", + "NODE_ENV": "production", + "NODE_APP_INSTANCE": "search" + } + }, "test": { "command": "npm run version-check && npm run unittest && npm run servertest && npm run eslint && npm run stylelint", "env": { diff --git a/src/amo/routes.js b/src/amo/routes.js index b35772da9af..7ae242e1318 100644 --- a/src/amo/routes.js +++ b/src/amo/routes.js @@ -5,8 +5,7 @@ import App from './containers/App'; import Home from './containers/Home'; export default ( - + - ); diff --git a/src/core/client/base.js b/src/core/client/base.js index 347b35e6fb3..4107f719809 100644 --- a/src/core/client/base.js +++ b/src/core/client/base.js @@ -6,7 +6,7 @@ import { render } from 'react-dom'; import { Provider } from 'react-redux'; import { Router, browserHistory } from 'react-router'; import { ReduxAsyncConnect } from 'redux-async-connect'; -import { langToLocale, getLanguage } from 'core/i18n/utils'; +import { langToLocale, sanitizeLanguage } from 'core/i18n/utils'; import I18nProvider from 'core/i18n/Provider'; import Jed from 'jed'; @@ -18,7 +18,7 @@ export default function makeClient(routes, createStore) { let initialState; const html = document.querySelector('html'); - const lang = getLanguage(html.getAttribute('lang')); + const lang = sanitizeLanguage(html.getAttribute('lang')); const locale = langToLocale(lang); const appName = config.get('appName'); diff --git a/src/core/i18n/utils.js b/src/core/i18n/utils.js index a4723294782..faee30e6f78 100644 --- a/src/core/i18n/utils.js +++ b/src/core/i18n/utils.js @@ -59,11 +59,11 @@ export function normalizeLocale(locale) { return langToLocale(localeToLang(locale)); } -export function isValidLang(lang) { - return validLangs.includes(lang); +export function isValidLang(lang, { _validLangs = validLangs } = {}) { + return _validLangs.includes(normalizeLang(lang)); } -export function getLanguage(langOrLocale) { +export function sanitizeLanguage(langOrLocale) { let language = normalizeLang(langOrLocale); // Only look in the un-mapped lang list. if (!langs.includes(language)) { @@ -73,7 +73,7 @@ export function getLanguage(langOrLocale) { } export function isRtlLang(lang) { - const language = getLanguage(lang); + const language = sanitizeLanguage(lang); return rtlLangs.includes(language); } @@ -81,16 +81,85 @@ export function getDirection(lang) { return isRtlLang(lang) ? 'rtl' : 'ltr'; } -export function getLangFromRouter(renderProps) { - if (renderProps) { - // Get the lang from the url param by default - // if it exists. - if (renderProps.params && renderProps.params.lang) { - return getLanguage(renderProps.params.lang); - } else if (renderProps.location && renderProps.location.query && - renderProps.location.query.lang) { - return getLanguage(renderProps.location.query.lang); + +function qualityCmp(a, b) { + if (a.quality === b.quality) { + return 0; + } else if (a.quality < b.quality) { + return 1; + } + return -1; +} + +/* + * Parses the HTTP accept-language header and returns a + * sorted array of objects. Example object: + * { + * lang: 'pl', quality: 0.7 + * } + */ +export function parseAcceptLanguage(header) { + // pl,fr-FR;q=0.3,en-US;q=0.1 + if (!header || !header.split) { + return []; + } + const rawLangs = header.split(','); + const langList = rawLangs.map((rawLang) => { + const parts = rawLang.split(';'); + let q = 1; + if (parts.length > 1 && parts[1].indexOf('q=') === 0) { + const qVal = parseFloat(parts[1].split('=')[1]); + if (isNaN(qVal) === false) { + q = qVal; + } } + return { + lang: parts[0].trim(), + quality: q, + }; + }); + langList.sort(qualityCmp); + return langList; +} + + +/* + * Given an accept-language header and a list of currently + * supported languages, returns the best match with no normalization. + * + */ +export function getLangFromHeader(acceptLanguage, { _validLangs } = {}) { + let userLang; + if (acceptLanguage) { + const langList = parseAcceptLanguage(acceptLanguage); + for (const langPref of langList) { + if (isValidLang(langPref.lang, { _validLangs })) { + userLang = langPref.lang; + break; + // Match locale, even if region isn't supported + } else if (isValidLang(langPref.lang.split('-')[0], { _validLangs })) { + userLang = langPref.lang.split('-')[0]; + break; + } + } + } + return userLang; +} + +/* + * Looks up the language from the router renderProps. + * When that fails fall-back to accept-language. + * + */ +export function getFilteredUserLanguage({ renderProps, acceptLanguage } = {}) { + let userLang; + // Get the lang from the url param by default if it exists. + if (renderProps && renderProps.params && renderProps.params.lang) { + userLang = renderProps.params.lang; + } + // If we don't have a valid userLang set yet try accept-language. + if (!isValidLang(userLang) && acceptLanguage) { + userLang = getLangFromHeader(acceptLanguage); } - return defaultLang; + return sanitizeLanguage(userLang); } diff --git a/src/core/server/base.js b/src/core/server/base.js index 7078ac316cc..07fc4deaa59 100644 --- a/src/core/server/base.js +++ b/src/core/server/base.js @@ -19,7 +19,7 @@ import ServerHtml from 'core/containers/ServerHtml'; import config from 'config'; import { setLang, setJWT } from 'core/actions'; import log from 'core/logger'; -import { getDirection, getLangFromRouter, langToLocale } from 'core/i18n/utils'; +import { getDirection, getFilteredUserLanguage, langToLocale } from 'core/i18n/utils'; import I18nProvider from 'core/i18n/Provider'; import Jed from 'jed'; @@ -113,19 +113,24 @@ function baseServer(routes, createStore, { appInstanceName = appName } = {}) { fs.readFileSync(path.join(config.get('basePath'), 'dist/sri.json')) ) : {}; - const lang = getLangFromRouter(renderProps); + const acceptLanguage = req.headers['accept-language']; + // Get language from URL or fall-back to accept-language. + const lang = getFilteredUserLanguage({ renderProps, acceptLanguage }); - if (renderProps.params && renderProps.params.lang) { + if (renderProps.params) { const origLang = renderProps.params.lang; if (lang !== origLang) { - // If the original lang param (found via the router) matches - // the first part of the original url path, redirect to the - // normalized lang (which will be the default if invalid). + // If a lang was provided but the lang looked up is different + // redirect to the looked-up lang (or default). // eslint-disable-next-line no-unused-vars const [_, firstPart, ...rest] = req.originalUrl.split('/'); if (origLang === decodeURIComponent(firstPart)) { // The '' provides a leading / return res.redirect(301, ['', lang, ...rest].join('/')); + } else if (!origLang && config.get('redirectLangPrefix')) { + // If there was no lang param. Redirect to the same URL with + // a lang prepended. + return res.redirect(301, `/${lang}${req.originalUrl}`); } } } diff --git a/src/disco/routes.js b/src/disco/routes.js index 603ac6e1856..7146f83c62b 100644 --- a/src/disco/routes.js +++ b/src/disco/routes.js @@ -5,10 +5,10 @@ import App from './containers/App'; import DiscoPane from './containers/DiscoPane'; export default ( - + diff --git a/tests/client/core/i18n/test_utils.js b/tests/client/core/i18n/test_utils.js index 77909297f28..158e534ca0d 100644 --- a/tests/client/core/i18n/test_utils.js +++ b/tests/client/core/i18n/test_utils.js @@ -82,25 +82,25 @@ describe('i18n utils', () => { }); }); - describe('getLanguage()', () => { + describe('sanitizeLanguage()', () => { it('should get a standard language ', () => { - assert.equal(utils.getLanguage('ar'), 'ar'); + assert.equal(utils.sanitizeLanguage('ar'), 'ar'); }); it('should convert short form lang to longer', () => { - assert.equal(utils.getLanguage('en'), 'en-US'); + assert.equal(utils.sanitizeLanguage('en'), 'en-US'); }); it('should return the default if lookup not present', () => { - assert.equal(utils.getLanguage('awooga'), 'en-US'); + assert.equal(utils.sanitizeLanguage('awooga'), 'en-US'); }); it('should return the default if bad type', () => { - assert.equal(utils.getLanguage(1), 'en-US'); + assert.equal(utils.sanitizeLanguage(1), 'en-US'); }); it('should return a lang if handed a locale', () => { - assert.equal(utils.getLanguage('en_US'), 'en-US'); + assert.equal(utils.sanitizeLanguage('en_US'), 'en-US'); }); }); @@ -144,10 +144,35 @@ describe('i18n utils', () => { }); }); - describe('getLangFromRouter()', () => { + describe('getFilteredUserLanguage()', () => { + it('should return default lang if called without args', () => { + assert.equal(utils.getFilteredUserLanguage(), config.get('defaultLang')); + }); + it('should return default lang if no lang is provided', () => { const fakeRenderProps = {}; - assert.equal(utils.getLangFromRouter(fakeRenderProps), config.get('defaultLang')); + const result = utils.getFilteredUserLanguage({ renderProps: fakeRenderProps }); + assert.equal(result, config.get('defaultLang')); + }); + + it('should return default lang if bad lang is provided', () => { + const fakeRenderProps = { + params: { + lang: 'bogus', + }, + }; + const result = utils.getFilteredUserLanguage({ renderProps: fakeRenderProps }); + assert.equal(result, config.get('defaultLang')); + }); + + it('should return default lang if bad lang type provided', () => { + const fakeRenderProps = { + params: { + lang: 1, + }, + }; + const result = utils.getFilteredUserLanguage({ renderProps: fakeRenderProps }); + assert.equal(result, config.get('defaultLang')); }); it('should return lang if provided via the URL', () => { @@ -156,32 +181,146 @@ describe('i18n utils', () => { lang: 'fr', }, }; - assert.equal(utils.getLangFromRouter(fakeRenderProps), 'fr'); + assert.equal(utils.getFilteredUserLanguage({ renderProps: fakeRenderProps }), 'fr'); }); - it('should return lang if provided via a query param', () => { + it('should fall-back to accept-language', () => { const fakeRenderProps = { - location: { - query: { - lang: 'pt-PT', - }, + params: { + lang: 'bogus', }, }; - assert.equal(utils.getLangFromRouter(fakeRenderProps), 'pt-PT'); + const acceptLanguage = 'pt-br;q=0.5,en-us;q=0.3,en;q=0.2'; + const result = utils.getFilteredUserLanguage( + { renderProps: fakeRenderProps, acceptLanguage }); + assert.equal(result, 'pt-BR'); }); - it('should use url param if both that and query string are present', () => { + it('should map lang from accept-language too', () => { const fakeRenderProps = { params: { - lang: 'fr', + lang: 'wat', }, - location: { - query: { - lang: 'pt-PT', - }, + }; + const acceptLanguage = 'pt;q=0.5,en-us;q=0.3,en;q=0.2'; + const result = utils.getFilteredUserLanguage( + { renderProps: fakeRenderProps, acceptLanguage }); + assert.equal(result, 'pt-PT'); + }); + + it('should fallback when nothing matches', () => { + const fakeRenderProps = { + params: { + lang: 'wat', }, }; - assert.equal(utils.getLangFromRouter(fakeRenderProps), 'fr'); + const acceptLanguage = 'awooga;q=0.5'; + const result = utils.getFilteredUserLanguage( + { renderProps: fakeRenderProps, acceptLanguage }); + assert.equal(result, 'en-US'); + }); + }); + + describe('utils.parseAcceptLanguage()', () => { + it('returns an empty list if no arg is passed', () => { + assert.deepEqual(utils.parseAcceptLanguage(), []); + }); + + it('orders an accept-language header', () => { + const input = 'fil;q=0.5,en;q=0.7'; + assert.deepEqual(utils.parseAcceptLanguage(input), [ + { lang: 'en', quality: 0.7 }, + { lang: 'fil', quality: 0.5 }, + ], JSON.stringify(utils.parseAcceptLanguage(input))); + }); + + it('orders non-quality items higher', () => { + const input = 'fil,en;q=0.7'; + assert.deepEqual(utils.parseAcceptLanguage(input), [ + { lang: 'fil', quality: 1 }, + { lang: 'en', quality: 0.7 }, + ], JSON.stringify(utils.parseAcceptLanguage(input))); + }); + }); + + describe('utils.getLangFromHeader()', () => { + it('should find an exact language match for Punjabi', () => { + const acceptLanguage = 'pa,sv;q=0.8,fi;q=0.7,it-ch;q=0.5,en-us;q=0.3,en;q=0.2'; + const supportedLangs = ['af', 'en-US', 'pa']; + const result = utils.getLangFromHeader(acceptLanguage, { _validLangs: supportedLangs }); + assert.equal(result, 'pa'); + }); + + it('should find an exact language match for Punjabi India', () => { + const acceptLanguage = 'pa-in,sv;q=0.8,fi;q=0.7,it-ch;q=0.5,en-us;q=0.3,en;q=0.2'; + const supportedLangs = ['af', 'en-US', 'pa']; + const result = utils.getLangFromHeader(acceptLanguage, { _validLangs: supportedLangs }); + assert.equal(result, 'pa'); + }); + + it('should not extend into region unless exact match is found', () => { + const acceptLanguage = 'pa,sv;q=0.8,fi;q=0.7,it-ch;q=0.5,en-us;q=0.3,en;q=0.2'; + const supportedLangs = ['af', 'en-US', 'pa-IN']; + const result = utils.getLangFromHeader(acceptLanguage, { _validLangs: supportedLangs }); + assert.equal(result, 'en-us'); + }); + + it('should not match Finnish to Filipino (Philiippines)', () => { + const acceptLanguage = dedent`fil-PH,fil;q=0.97,en-US;q=0.94,en;q=0.91,en-ph; + q=0.89,en-gb;q=0.86,hu-HU;q=0.83,hu;q=0.8,en-AU;q=0.77,en-nl; + q=0.74,nl-en;q=0.71,nl;q=0.69,en-HK;q=0.66,en-sg;q=0.63,en-th; + q=0.6,pl-PL;q=0.57,pl;q=0.54,fr-FR;q=0.51,fr;q=0.49,en-AE; + q=0.46,zh-CN;q=0.43,zh;q=0.4,ja-JP;q=0.37,ja;q=0.34,id-ID; + q=0.31,id;q=0.29,ru-RU;q=0.26,ru;q=0.23,de-DE;q=0.2,de; + q=0.17,ko-KR;q=0.14,ko;q=0.11,es-ES;q=0.09,es;q=0.06,en-AP;q=0.0`; + const supportedLangs = ['en-US', 'fi']; + const result = utils.getLangFromHeader(acceptLanguage, { _validLangs: supportedLangs }); + assert.equal(result, 'en-US'); + }); + + it('should support Filipino (Philippines)', () => { + const acceptLanguage = dedent`fil-PH,fil;q=0.97,en-US;q=0.94,en;q=0.91,en-ph; + q=0.89,en-gb;q=0.86,hu-HU;q=0.83,hu;q=0.8,en-AU;q=0.77,en-nl; + q=0.74,nl-en;q=0.71,nl;q=0.69,en-HK;q=0.66,en-sg;q=0.63,en-th; + q=0.6,pl-PL;q=0.57,pl;q=0.54,fr-FR;q=0.51,fr;q=0.49,en-AE; + q=0.46,zh-CN;q=0.43,zh;q=0.4,ja-JP;q=0.37,ja;q=0.34,id-ID; + q=0.31,id;q=0.29,ru-RU;q=0.26,ru;q=0.23,de-DE;q=0.2,de; + q=0.17,ko-KR;q=0.14,ko;q=0.11,es-ES;q=0.09,es;q=0.06,en-AP;q=0.0`; + const supportedLangs = ['en-US', 'fi', 'fil-PH']; + const result = utils.getLangFromHeader(acceptLanguage, { _validLangs: supportedLangs }); + assert.equal(result, 'fil-PH'); + }); + + it('should support Filipino without region', () => { + const acceptLanguage = dedent`fil-PH,fil;q=0.97,en-US;q=0.94,en;q=0.91,en-ph; + q=0.89,en-gb;q=0.86,hu-HU;q=0.83,hu;q=0.8,en-AU;q=0.77,en-nl; + q=0.74,nl-en;q=0.71,nl;q=0.69,en-HK;q=0.66,en-sg;q=0.63,en-th; + q=0.6,pl-PL;q=0.57,pl;q=0.54,fr-FR;q=0.51,fr;q=0.49,en-AE; + q=0.46,zh-CN;q=0.43,zh;q=0.4,ja-JP;q=0.37,ja;q=0.34,id-ID; + q=0.31,id;q=0.29,ru-RU;q=0.26,ru;q=0.23,de-DE;q=0.2,de; + q=0.17,ko-KR;q=0.14,ko;q=0.11,es-ES;q=0.09,es;q=0.06,en-AP;q=0.0`; + const supportedLangs = ['en-US', 'fi', 'fil']; + const result = utils.getLangFromHeader(acceptLanguage, { _validLangs: supportedLangs }); + assert.equal(result, 'fil'); + }); + + it('should return undefined language for no match', () => { + const acceptLanguage = 'whatever'; + const supportedLangs = ['af', 'en-US', 'pa']; + const result = utils.getLangFromHeader(acceptLanguage, { _validLangs: supportedLangs }); + assert.equal(result, undefined); + }); + + it('should return undefined for empty string', () => { + const acceptLanguage = ''; + const result = utils.getLangFromHeader(acceptLanguage); + assert.equal(result, undefined); + }); + + it('should return undefined for bad type', () => { + const acceptLanguage = null; + const result = utils.getLangFromHeader(acceptLanguage); + assert.equal(result, undefined); }); }); }); diff --git a/tests/server/disco/TestCSPConfig.js b/tests/server/disco/TestCSPConfig.js index d8e7b7101cb..2c1966deca9 100644 --- a/tests/server/disco/TestCSPConfig.js +++ b/tests/server/disco/TestCSPConfig.js @@ -1,19 +1,7 @@ import { assert } from 'chai'; -import requireUncached from 'require-uncached'; +import config from 'config'; describe('Disco App Specific CSP Config', () => { - let config; - - afterEach(() => { - process.env.NODE_ENV = 'production'; - delete process.env.NODE_APP_INSTANCE; - }); - - beforeEach(() => { - process.env.NODE_APP_INSTANCE = 'disco'; - config = requireUncached('config'); - }); - it('should set style-src for disco', () => { const cspConfig = config.get('CSP').directives; assert.deepEqual(cspConfig.styleSrc, ['https://addons-discovery.cdn.mozilla.net']); diff --git a/tests/server/disco/TestViews.js b/tests/server/disco/TestViews.js index ee1e9ad1d82..4e2e6fdd224 100644 --- a/tests/server/disco/TestViews.js +++ b/tests/server/disco/TestViews.js @@ -22,67 +22,75 @@ describe('Discovery Pane GET requests', () => { }); it('should have a CSP policy for / on the disco app', () => request(app) - .get('/') + .get('/en-US/') .expect(200) .then((res) => { const policy = new Policy(res.header['content-security-policy']); assert.notInclude(policy.get('script-src'), "'self'"); - assert.include(policy.get('script-src'), 'https://addons.cdn.mozilla.net'); + assert.include(policy.get('script-src'), 'https://addons-discovery.cdn.mozilla.net'); assert.notInclude(policy.get('connect-src'), "'self'"); assert.include(policy.get('connect-src'), 'https://addons.mozilla.org'); })); it('should be using SRI for script and style in /', () => request(app) - .get('/') + .get('/en-US/') .expect(200) .then((res) => checkSRI(res))); it('should redirect an invalid locale', () => request(app) - .get('/whatevs/firefox/discovery/pane/48.0/Darwin/normal?lang=dbl') + .get('/whatevs/firefox/discovery/pane/48.0/Darwin/normal') .expect(301) .then((res) => { assert.equal(res.header.location, - '/en-US/firefox/discovery/pane/48.0/Darwin/normal?lang=dbl'); + '/en-US/firefox/discovery/pane/48.0/Darwin/normal'); })); it('should redirect an invalid locale which will be encoded', () => request(app) - .get('/