From 55654a1195335b08b92a84e10f65dcc1ec73d5d3 Mon Sep 17 00:00:00 2001 From: Ed Morley <501702+edmorley@users.noreply.github.com> Date: Thu, 10 May 2018 17:58:04 +0100 Subject: [PATCH] Bug 1363722 - Allow login to work in all development workflows (#3534) Previously it was not possible to test features that required an authenticated user when: * using `yarn start` with Vagrant (bug 1363722), which meant slower watch builds * pointing the UI at the prod/stage API (bug 1317752), which was extremely limiting Now login works in all environments, since the frontend no longer uses a URL prefix, but instead webpack-dev-server proxies non-webpack URLs to the chosen `BACKEND_DOMAIN` - avoiding cross-domain issues. Cookies are rewritten to remove any `secure` directive (which is set on production), so that they can still be read from HTTP localhost. The `Referer` has to also be changed to stop Django's CSRF checks from rejecting request. The slower "build into `dist` and watch" mode is therefore no longer necessary, so `yarn start:local` instead invokes webpack-dev-server just like `yarn start` - and the `local-watch.js` workaround has been removed. Support for the "publish to GitHub with hardcoded `SERVICE_DOMAIN`" workflow has been dropped, since it was already rarely used and there is no way to make it support login. The API domain environment variable was renamed to `BACKEND_DOMAIN` to avoid potential confusion given it no longer behaves the same as `SERVICE_DOMAIN` used to. NB: For full stack Vagrant workflows users must now connect to port *5000* on localhost, not 8000. --- docs/common_tasks.rst | 26 ----------------- docs/installation.rst | 4 +-- docs/seta.rst | 8 +++--- docs/ui/installation.rst | 15 ++++------ neutrino-custom/base.js | 10 +++++++ neutrino-custom/development.js | 40 +++++++++++++++----------- neutrino-custom/lint.js | 3 -- neutrino-custom/local-watch.js | 43 ---------------------------- neutrino-custom/production.js | 22 -------------- package.json | 4 +-- tests/ui/unit/init.js | 4 +-- ui/css/treeherder-navbar.css | 6 ---- ui/helpers/urlHelper.js | 7 +++-- ui/js/components/auth.js | 33 ++++++--------------- ui/test-view/redux/configureStore.js | 1 + 15 files changed, 63 insertions(+), 163 deletions(-) delete mode 100755 neutrino-custom/local-watch.js diff --git a/docs/common_tasks.rst b/docs/common_tasks.rst index d22dffd2f..6b30f8cfb 100644 --- a/docs/common_tasks.rst +++ b/docs/common_tasks.rst @@ -106,32 +106,6 @@ Building the docs locally * Source changes will result in automatic rebuilds and browser page reload. -Sharing UI-only changes with others using GitHub Pages ------------------------------------------------------- - -It's possible to share UI-only changes with others (for prototyping/testing) using -GitHub Pages. This is recommended over pushing a custom branch to stage, unless the -feature requires that you be logged into Treeherder (which won't work -cross-domain). - -To do this: - -* Fork the Treeherder repository to your own GitHub account. - -* Create a gh-pages branch locally based on the feature branch you wish to test, that is configured to point at production's API. eg: - - .. code-block:: bash - - git checkout (your feature branch) - git checkout -b gh-pages - SERVICE_DOMAIN=https://treeherder.mozilla.org yarn build - git add -f dist/ - git commit -m "Add dist directory containing built UI" - -* Push the ``gh-pages`` branch to your Treeherder fork. - -* Tell people to visit: ``https://.github.io/treeherder/dist/`` - Updating package.json --------------------- diff --git a/docs/installation.rst b/docs/installation.rst index b205b3e27..7b474c623 100644 --- a/docs/installation.rst +++ b/docs/installation.rst @@ -66,7 +66,7 @@ Starting a local Treeherder instance this is more convenient because it automatically refreshes every time there's a change in the code. -* You must also build the UI. Open a new terminal window and ``vagrant ssh`` to +* You must also start the UI dev server. Open a new terminal window and ``vagrant ssh`` to the VM again, then run the following: .. code-block:: bash @@ -76,7 +76,7 @@ Starting a local Treeherder instance This will build the UI code in the ``dist/`` folder and keep watching for new changes (See the :doc:`UI installation section ` for more ways to work with the UI code). -* Visit http://localhost:8000 in your browser. Note: There will be no data to display until the ingestion tasks are run. +* Visit http://localhost:5000 in your browser (NB: port has changed). Note: There will be no data to display until the ingestion tasks are run. Running the ingestion tasks --------------------------- diff --git a/docs/seta.rst b/docs/seta.rst index e34a2fba6..2f828f15e 100644 --- a/docs/seta.rst +++ b/docs/seta.rst @@ -57,10 +57,10 @@ After you set up Treeherder, ssh (3 different tabs) into the provisioned VM and Try out the various APIs ------------------------ -* http://localhost:8000/api/project/mozilla-inbound/seta/v1/job-priorities/?build_system_type=buildbot -* http://localhost:8000/api/project/mozilla-inbound/seta/v1/job-priorities/?build_system_type=taskcluster -* http://localhost:8000/api/project/mozilla-inbound/seta/v1/job-types/ -* http://localhost:8000/api/seta/v1/failures-fixed-by-commit/ +* http://localhost:5000/api/project/mozilla-inbound/seta/v1/job-priorities/?build_system_type=buildbot +* http://localhost:5000/api/project/mozilla-inbound/seta/v1/job-priorities/?build_system_type=taskcluster +* http://localhost:5000/api/project/mozilla-inbound/seta/v1/job-types/ +* http://localhost:5000/api/seta/v1/failures-fixed-by-commit/ * This one won't work until https://bugzilla.mozilla.org/show_bug.cgi?id=1389123 is fixed diff --git a/docs/ui/installation.rst b/docs/ui/installation.rst index e08c18500..c2f9d7ef3 100644 --- a/docs/ui/installation.rst +++ b/docs/ui/installation.rst @@ -1,10 +1,7 @@ Installation ============ -It's possible to work on the UI without setting up the Vagrant VM. There are a -few limitations, such as login not being available, but it works well enough for -quick testing. For instructions on how to serve the UI with working URL rewriting, -see the Vagrant instructions. +It's possible to work on the UI without setting up the Vagrant VM. To get started: @@ -16,7 +13,7 @@ Running the standalone development server ----------------------------------------- The default development server runs the unminified UI and fetches data from the -production site. You do not need to set up the Vagrant VM, but login will be unavailable. +production site. You do not need to set up the Vagrant VM unless making backend changes. * Start the development server by running: @@ -36,7 +33,7 @@ production site. You do not need to set up the Vagrant VM, but login will be una .. code-block:: bash - $ SERVICE_DOMAIN= yarn start + $ BACKEND_DOMAIN= yarn start This will run the unminified UI using ```` as the service domain. @@ -62,9 +59,7 @@ installation instructions, then follow these steps: vagrant ~/treeherder$ yarn start:local -This will watch UI files for changes and build an unminified version in the ``dist/`` directory. -Note that this process is a little slower than using the regular development server, so you may -wish to use it only for development that requires a frontend login. +* The server will perform an initial build and then watch for new changes. Once the server is running, you can navigate to: ``_ to see the UI. Building the minified UI with Vagrant ------------------------------------- @@ -82,7 +77,7 @@ If you would like to view the minified production version of the UI with Vagrant $ yarn build -Once the build is complete, the minified version of the UI will now be accessible at http://localhost:8000. +Once the build is complete, the minified version of the UI will now be accessible at http://localhost:8000 (NB: port 8000, unlike above). Validating JavaScript ===================== diff --git a/neutrino-custom/base.js b/neutrino-custom/base.js index 49fc0d51b..a20a0d206 100644 --- a/neutrino-custom/base.js +++ b/neutrino-custom/base.js @@ -5,6 +5,7 @@ const lintPreset = require('./lint'); const reactPreset = require('neutrino-preset-react'); const HtmlPlugin = require('html-webpack-plugin'); const htmlTemplate = require('html-webpack-template'); +const CopyPlugin = require('copy-webpack-plugin'); const CWD = process.cwd(); const SRC = path.join(CWD, 'src'); // neutrino's default source directory @@ -279,6 +280,15 @@ module.exports = neutrino => { }); neutrino.config.devtool('source-map'); + + // Overwriting the existing copy rule rather than modifying, since it's only + // enabled in production, whereas it really should be used in development too. + neutrino.config.plugin('copy') + .use(CopyPlugin, [ + { context: UI, from: 'contribute.json' }, + { context: UI, from: 'revision.txt' }, + { context: UI, from: 'robots.txt' }, + ]); }; module.exports.CWD = CWD; diff --git a/neutrino-custom/development.js b/neutrino-custom/development.js index bbee46f46..8d352b285 100644 --- a/neutrino-custom/development.js +++ b/neutrino-custom/development.js @@ -1,30 +1,38 @@ 'use strict'; -const webpack = require('webpack'); const basePreset = require('./base'); const UI = require('./base').UI; -// Set the service domain to production if no environment value was provided, since -// webpack-dev-server doesn't serve data from the vagrant machine. -const SERVICE_DOMAIN = (typeof process.env.SERVICE_DOMAIN !== 'undefined') - ? process.env.SERVICE_DOMAIN : 'https://treeherder.mozilla.org'; +// Default to the production backend (is overridden to localhost when using start:local) +const BACKEND_DOMAIN = process.env.BACKEND_DOMAIN || 'https://treeherder.mozilla.org'; module.exports = neutrino => { basePreset(neutrino); - // Set service domain so that ui/js/config can use it: - neutrino.config - .plugin('define') - .use(webpack.DefinePlugin, { - SERVICE_DOMAIN: JSON.stringify(SERVICE_DOMAIN) - }); - - // Set up the dev server with an api proxy to the service domain: + // Make the dev server proxy any paths not recognised by webpack to the specified backend. neutrino.config.devServer .contentBase(UI) .set('proxy', { - '/api/*': { - target: SERVICE_DOMAIN, - changeOrigin: true + '*': { + target: BACKEND_DOMAIN, + changeOrigin: true, + onProxyReq: (proxyReq) => { + // Adjust the referrer to keep Django's CSRF protection happy, whilst + // still making it clear that the requests were from local development. + proxyReq.setHeader('referer', `${BACKEND_DOMAIN}/webpack-dev-server`); + }, + onProxyRes: (proxyRes) => { + // Strip the cookie `secure` attribute, otherwise prod cookies + // will be rejected by the browser when using non-HTTPS localhost: + // https://github.com/nodejitsu/node-http-proxy/pull/1166 + const removeSecure = str => str.replace(/; secure/i, ''); + const setCookie = proxyRes.headers['set-cookie']; + if (setCookie) { + const result = Array.isArray(setCookie) + ? setCookie.map(removeSecure) + : removeSecure(setCookie); + proxyRes.headers['set-cookie'] = result; + } + } } }); }; diff --git a/neutrino-custom/lint.js b/neutrino-custom/lint.js index 02b921e92..0aca9d658 100644 --- a/neutrino-custom/lint.js +++ b/neutrino-custom/lint.js @@ -59,9 +59,6 @@ module.exports = neutrino => { 'space-infix-ops': 'off', 'spaced-comment': 'off', }, - globals: [ - 'SERVICE_DOMAIN', - ] } })); }; diff --git a/neutrino-custom/local-watch.js b/neutrino-custom/local-watch.js deleted file mode 100755 index 33f0890bc..000000000 --- a/neutrino-custom/local-watch.js +++ /dev/null @@ -1,43 +0,0 @@ -'use strict'; - -const productionPreset = require('./production'); - -// Taskcluster login POSTs in treeherder require that the UI is served from the same -// origin as the backend. To allow for logins in watched, unminified mode, here we -// build a webpack watcher that recompiles sources to dist/ on change, so that the -// Vagrant box may serve them. This approach is slower than using the dev server, but -// it may become unnecessary if Bug 1317752 (Enable logging in with Taskcluster Auth -// cross-domain) is completed. - -module.exports = neutrino => { - // This configuration is based on the production config, but is not minified - productionPreset(neutrino); - - // Removing the dev server configuration puts neutrino into watch mode - neutrino.config.devServer.clear(); - - // Don't minify: - neutrino.config.plugins.delete('minify'); - - // Now we must remove some hot-reload settings triggered by the 'development' - // NODE_ENV variable set by `neutrino start`: - // Remove hot loader plugin: - neutrino.config.plugins.delete('hot'); - - // Remove hot loader patch from index: - const protocol = process.env.HTTPS ? 'https' : 'http'; - const host = process.env.HOST || 'localhost'; - const port = parseInt(process.env.PORT) || 5000; - neutrino.config.entry('index') - .delete(`webpack-dev-server/client?${protocol}://${host}:${port}/`) - .delete('webpack/hot/dev-server') - .delete(require.resolve('react-hot-loader/patch')); - - // Remove hot loader react plugin: - neutrino.config.module - .rule('compile') - .loader('babel', ({ options }) => { - options.env.development.plugins.shift(); - return { options }; - }); -}; diff --git a/neutrino-custom/production.js b/neutrino-custom/production.js index 344cac8c2..e4c26d388 100644 --- a/neutrino-custom/production.js +++ b/neutrino-custom/production.js @@ -1,10 +1,7 @@ 'use strict'; -const webpack = require('webpack'); const basePreset = require('./base'); -const CopyPlugin = require('copy-webpack-plugin'); const CleanPlugin = require('clean-webpack-plugin'); const CWD = require('./base').CWD; -const UI = require('./base').UI; const DIST = require('./base').DIST; module.exports = neutrino => { @@ -23,25 +20,6 @@ module.exports = neutrino => { } )); - // Define the service domain globally: - neutrino.config.plugin('define') - .use(webpack.DefinePlugin, { - SERVICE_DOMAIN: JSON.stringify( - typeof process.env.SERVICE_DOMAIN !== 'undefined' ? process.env.SERVICE_DOMAIN : '') - }); - - - // The copy plugin is overwritten and not injected so that when this preset is - // imported in ./local-watch.js and run via `neutrino start`, it is still included - // in the config (it is only applied in !development by default): - neutrino.config.plugin('copy') - .use(CopyPlugin, [ - {context: UI, from: 'contribute.json'}, - {context: UI, from: 'revision.txt'}, - {context: UI, from: 'robots.txt'}, - ]); - - // Likewise for this clean plugin: neutrino.config.plugin('clean') .use(CleanPlugin, [DIST], { root: CWD } ); diff --git a/package.json b/package.json index 783deaa9b..d47e3cc71 100644 --- a/package.json +++ b/package.json @@ -82,8 +82,8 @@ "build": "node ./node_modules/neutrino/bin/neutrino build --presets ./neutrino-custom/production.js", "lint": "node ./node_modules/eslint/bin/eslint.js --ext .js,.jsx ui/", "start": "node ./node_modules/neutrino/bin/neutrino start --presets ./neutrino-custom/development.js", - "start:local": "node ./node_modules/neutrino/bin/neutrino start --presets ./neutrino-custom/local-watch.js", - "start:stage": "SERVICE_DOMAIN=https://treeherder.allizom.org node ./node_modules/neutrino/bin/neutrino start --presets ./neutrino-custom/development.js", + "start:local": "BACKEND_DOMAIN=http://localhost:8000 node ./node_modules/neutrino/bin/neutrino start --presets ./neutrino-custom/development.js", + "start:stage": "BACKEND_DOMAIN=https://treeherder.allizom.org node ./node_modules/neutrino/bin/neutrino start --presets ./neutrino-custom/development.js", "test": "node ./node_modules/neutrino/bin/neutrino test --presets ./neutrino-custom/test.js", "test:watch": "node ./node_modules/neutrino/bin/neutrino test --watch --presets ./neutrino-custom/test.js" } diff --git a/tests/ui/unit/init.js b/tests/ui/unit/init.js index abfe2d2bd..2f0ee7800 100644 --- a/tests/ui/unit/init.js +++ b/tests/ui/unit/init.js @@ -9,13 +9,13 @@ import { configure } from 'enzyme'; import Adapter from 'enzyme-adapter-react-16'; // Global variables are set here instead of with webpack.ProvidePlugin -// because neutrino removes plugin definitions for karma runs +// because neutrino removes plugin definitions for karma runs: +// https://github.com/mozilla-neutrino/neutrino-dev/issues/617 window.jQuery = jQuery; configure({ adapter: new Adapter() }); const jsContext = require.context('../../../ui/js', true, /^\.\/.*\.jsx?$/); -window.SERVICE_DOMAIN = process.env.SERVICE_DOMAIN || ''; jsContext('./filters.js'); const controllerContext = require.context('../../../ui/js/controllers', true, /^\.\/.*\.jsx?$/); diff --git a/ui/css/treeherder-navbar.css b/ui/css/treeherder-navbar.css index 29aa573f5..db040a980 100644 --- a/ui/css/treeherder-navbar.css +++ b/ui/css/treeherder-navbar.css @@ -122,12 +122,6 @@ label.dropdown-item { padding-right: 14px; } -.nav-login-btn-unavail { - font-size: 12px; - color: grey; - cursor: not-allowed; -} - .th-context-navbar { background-color: #354048; overflow: visible; diff --git a/ui/helpers/urlHelper.js b/ui/helpers/urlHelper.js index 8ad35c838..41b58ca14 100644 --- a/ui/helpers/urlHelper.js +++ b/ui/helpers/urlHelper.js @@ -9,8 +9,10 @@ export const createQueryParams = function createQueryParams(params) { return `?${query.toString()}`; }; +// Leaving this here since even though SERVICE_DOMAIN no longer exists (proxying +// is used instead), it provides a single place to modify if needed in the future. export const getServiceUrl = function getServiceUrl(uri) { - return `${SERVICE_DOMAIN}${uri}`; + return uri; }; export const getApiUrl = function getApiUrl(uri) { @@ -129,9 +131,10 @@ export const parseQueryParams = function parseQueryParams(search) { return obj; }; +// TODO: Combine this with getApiUrl(). export const createApiUrl = function createApiUrl(api, params) { const query = createQueryParams(params); - return `${SERVICE_DOMAIN}/api/${api}${query}`; + return `/api/${api}${query}`; }; //bugs can be one bug or a comma separated (no spaces) string of bugs diff --git a/ui/js/components/auth.js b/ui/js/components/auth.js index fbbedd8e1..07c39d4d3 100644 --- a/ui/js/components/auth.js +++ b/ui/js/components/auth.js @@ -36,11 +36,8 @@ treeherder.component("login", { - `, bindings: { // calls to the HTML which sets the user value in the $rootScope. @@ -55,18 +52,6 @@ treeherder.component("login", { ctrl.user = {}; - // check if the user can login. SERVICE_DOMAIN must match - // host domain. Remove this if we fix - // Bug 1317752 - Enable logging in with Taskcluster Auth cross-domain - if (!SERVICE_DOMAIN) { - // SERVICE_DOMAIN isn't being used, so no mismatch possible. - this.userCanLogin = true; - } else { - const a = document.createElement('a'); - a.href = SERVICE_DOMAIN; - this.userCanLogin = (a.hostname === $location.host() && a.port === $location.port); - } - /** * Using a window listener here because I wasn't getting reliable * results from the events from angular-local-storage. @@ -91,15 +76,13 @@ treeherder.component("login", { }); // Ask the back-end if a user is logged in on page load - if (ctrl.userCanLogin) { - ThUserModel.get().then(async function (currentUser) { - if (currentUser.email && localStorage.getItem('userSession')) { - ctrl.setLoggedIn(currentUser); - } else { - ctrl.setLoggedOut(); - } - }); - } + ThUserModel.get().then(async function (currentUser) { + if (currentUser.email && localStorage.getItem('userSession')) { + ctrl.setLoggedIn(currentUser); + } else { + ctrl.setLoggedOut(); + } + }); /** * Opens a new tab to handle authentication, which will get closed diff --git a/ui/test-view/redux/configureStore.js b/ui/test-view/redux/configureStore.js index ef4267499..0f5c09183 100644 --- a/ui/test-view/redux/configureStore.js +++ b/ui/test-view/redux/configureStore.js @@ -259,6 +259,7 @@ function toggleHideClassified(store, fetchParams) { }); } +// TODO: Remove this now that SERVICE_DOMAIN no longer exists. // Remove the host from the URL. This is because sometimes the URL // will include the SERVICE_DOMAIN and sometimes SERVICE_DOMAIN will // be blank and therefore the browser will fill in the domain. So