Fixes #396: add default values to environment variables #550

Silvyre · 2020-01-19T19:07:02Z

Issue This PR Addresses

Fixes #396

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

Ensures that every process.env call within the code base is provided with a default value (as specified in #396) to ensure that they have some value on CI, and thus do not cause unwanted behaviour during CI testing.

The default value provided to each process.env is either one specified within env.example (if one exists), or null.

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

src/backend/index.js

src/backend/lib/redis.js

tools/add-feed.js

cindyorangis

Looks good

humphd

These nulls are probably not a good idea.

Also, can you add some code to src/backend/lib/config.js where we load the env using dotenv: we can check to see if it worked or not (i.e., if there is an .env file): https://www.npmjs.com/package/dotenv#config

If we get back result.error, we should probably either warn (like you did with redis) or perhaps throw and let the app crash now vs. fail later with random missing data. I think we should warn, so we don't have to mess with CI too much at this point.

src/backend/lib/redis.js

Silvyre · 2020-01-22T02:44:10Z

Also, can you add some code to src/backend/lib/config.js where we load the env using dotenv: we can check to see if it worked or not (i.e., if there is an .env file): https://www.npmjs.com/package/dotenv#config

If we get back result.error, we should probably either warn (like you did with redis) or perhaps throw and let the app crash now vs. fail later with random missing data. I think we should warn, so we don't have to mess with CI too much at this point.

@humphd How's this?

telescope/src/backend/lib/config.js

Lines 1 to 12 in 1074030

    
           const dotenv = require('dotenv'); 
        
           const { logger } = require('../utils/logger'); 
        
           const result = dotenv.config(); 
        
           if (result.error && logger) { 
        
             logger.error( 
        
               '\n\n\t💡  It appears that you have not yet configured a .env file.', 
        
               '\n\t   Please refer to our documentation regarding environment configuration:', 
        
               '\n\t   https://github.com/Seneca-CDOT/telescope/blob/master/docs/CONTRIBUTING.md\n' 
        
             ); 
        
           }

Because src/backend/utils/logger.js and src/backend/lib/config.js currently require each other, I added the && logger condition to ensure that logger.error() doesn't get called before logger is defined.

Also, it is worth mentioning that src/backend/lib/config.js is imported by a dozen other modules, which will result in a dozen warning messages being logged if .env is missing. I am not sure how we might ensure that only one warning message is logged in this case... Any suggestions?

manekenpix

👍

src/backend/lib/config.js

humphd · 2020-01-22T14:59:41Z

src/backend/login/usingPassport.js

@@ -12,7 +12,8 @@ SAML2_REDIRECT_URI=http://localhost:3000/oauth/callback
 */
 let cert = null;

-const { SAML2_BASE_URI, SAML2_REDIRECT_URI } = process.env;
+const SAML2_BASE_URI = process.env.SAML2_BASE_URI || '';


I don't think this is a good idea. Let's revert the changes in this file. We don't want to hide a missing value by having a proper but empty string get passed in.

Sure, done: e4c5a12

Currently, env.example assigns an empty string to be the value of SAML2_BASE_URI:

telescope/env.example

Lines 28 to 29 in 08c55d4

# SAML2 = BASE URL e.g. http://localhost:8080/simplesaml/saml2/idp/SSOService.php - This is pointing to the authentication service. e.g. What happens when you try to login to Seneca's site.

SAML2_BASE_URI=""

Let me know if you'd like me to remove this value, as well.

humphd · 2020-01-22T15:01:00Z

src/backend/utils/logger.js

@@ -9,7 +9,7 @@ require('../lib/config');
 let logLevel = (process.env.LOG_LEVEL || 'info').toLowerCase();
 if (!pino.levels.values[logLevel]) {
  // Use `debug` by default in development mode, `info` otherwise.
-  logLevel = process.env.NODE_ENV === 'development' ? 'debug' : 'info';
+  logLevel = !process.env.NODE_ENV || process.env.NODE_ENV === 'development' ? 'debug' : 'info';


I don't understand this change. We need to set one of debug or info. What is the first part of this doing? If process.env.NODE_ENV is undefined, the original code will return 'debug' already. I think you can revert this unless I'm missing something.

If process.env.NODE_ENV is undefined, the original code will return 'debug' already.

Sorry, Dave, but my understanding is that, since undefined === 'development' evaluates to false, the enclosing ternary operation would return 'info'. However, I did also assume that CI would want to utilize a more verbose log level, and may be wrong about this. Let me know.

humphd · 2020-01-22T15:04:11Z

src/backend/utils/wiki-feed-parser.js

@@ -11,7 +11,9 @@ const { JSDOM } = jsdom;
 * That data is then returned as a Promise
 */
 module.exports.getData = function() {
-  return fetch(process.env.FEED_URL)
+  return fetch(
+    process.env.FEED_URL || 'https://wiki.cdot.senecacollege.ca/wiki/Planet_CDOT_Feed_List'


This is probably wise, since this URL doesn't change very often (if ever). But could you add a log that says we're doing this? Something like:

let ur = process.env.FEED_URL; if(!url) { url = 'https://wiki.cdot.senecacollege.ca/wiki/Planet_CDOT_Feed_List' logger.debug('No value found for FEED_URL in env, using default ${url} instead'); }

This will help us debug things later, if we can't figure out why it's always loading a URL we don't intend.

Great idea! Done: e4c5a12

humphd · 2020-01-22T15:04:48Z

src/backend/web/routes/login.js

@@ -13,7 +13,7 @@ router.use(cookieParser());
 router.use(bodyParser.urlencoded({ extended: false }));
 router.use(bodyParser.json());

-const { SAML2_CLIENT_SECRET } = process.env;
+const SAML2_CLIENT_SECRET = process.env.SAML2_CLIENT_SECRET || 'secret';


This seems like a security issue. Let's not provide any default fallback if we're missing a secret. We should fail loudly if we are.

For sure... done: e4c5a12

Currently, env.example mentions secret to be the default value of SAML2_CLIENT_SECRET:

telescope/env.example

Line 34 in 08c55d4

# SAML2_CLIENT_SECRET = CLIENT SECRET obtained from SAML Strategy, default : secret;

Let me know if you'd like me to remove this piece of information, as well.

I'll also ask if you'd like me to do the same for SAML2_CLIENT_ID—its default value is likewise mentioned:

telescope/env.example

Line 31 in 08c55d4

# SAML2_CLIENT_ID = CLIENT ID obtained from SAML Strategy default: saml-poc

humphd

This needs a rebase, but looks good otherwise.

Silvyre · 2020-01-24T00:14:11Z

Rebase completed and ready for final review; thank you everyone for your help!

humphd

Tested locally, works great. Well done.

manekenpix

👍

Silvyre added the area: CI/CD Continuous integration / Continuous delivery label Jan 19, 2020

Silvyre self-assigned this Jan 19, 2020

humphd added the developer experience Helping the Developer Experience label Jan 19, 2020

manekenpix requested changes Jan 19, 2020

View reviewed changes

src/backend/index.js Outdated Show resolved Hide resolved

src/backend/lib/redis.js Outdated Show resolved Hide resolved

tools/add-feed.js Outdated Show resolved Hide resolved

cindyorangis previously approved these changes Jan 20, 2020

View reviewed changes

humphd requested changes Jan 21, 2020

View reviewed changes

src/backend/lib/redis.js Outdated Show resolved Hide resolved

humphd mentioned this pull request Jan 21, 2020

Friendly error message if wiki feed URL is missing from .env #562

Closed

Silvyre dismissed cindyorangis’s stale review via 273499f January 22, 2020 01:23

Silvyre requested review from humphd, manekenpix and cindyorangis January 22, 2020 02:45

manekenpix previously approved these changes Jan 22, 2020

View reviewed changes

src/backend/lib/config.js Show resolved Hide resolved

humphd requested changes Jan 22, 2020

View reviewed changes

manekenpix self-requested a review January 22, 2020 20:52

Silvyre dismissed manekenpix’s stale review via e4c5a12 January 23, 2020 04:24

humphd requested changes Jan 23, 2020

View reviewed changes

Silvyre force-pushed the issue-396 branch 2 times, most recently from c8624da to 2094902 Compare January 23, 2020 23:49

Fixes Seneca-CDOT#396: add default values to environment variables

c600b5f

Silvyre force-pushed the issue-396 branch from 2094902 to c600b5f Compare January 24, 2020 00:08

Silvyre requested a review from humphd January 24, 2020 00:14

humphd approved these changes Jan 24, 2020

View reviewed changes

manekenpix approved these changes Jan 24, 2020

View reviewed changes

manekenpix merged commit ebf788b into Seneca-CDOT:master Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #396: add default values to environment variables #550

Fixes #396: add default values to environment variables #550

Silvyre commented Jan 19, 2020 •

edited

Loading

cindyorangis left a comment

humphd left a comment

Silvyre commented Jan 22, 2020 •

edited

Loading

manekenpix left a comment

humphd Jan 22, 2020

Silvyre Jan 23, 2020

humphd Jan 22, 2020

Silvyre Jan 23, 2020

humphd Jan 22, 2020

Silvyre Jan 23, 2020

humphd Jan 22, 2020

Silvyre Jan 23, 2020

humphd left a comment

Silvyre commented Jan 24, 2020 •

edited

Loading

humphd left a comment

manekenpix left a comment

	# SAML2 = BASE URL e.g. http://localhost:8080/simplesaml/saml2/idp/SSOService.php - This is pointing to the authentication service. e.g. What happens when you try to login to Seneca's site.
	SAML2_BASE_URI=""

Fixes #396: add default values to environment variables #550

Fixes #396: add default values to environment variables #550

Conversation

Silvyre commented Jan 19, 2020 • edited Loading

Issue This PR Addresses

Type of Change

Description

Checklist

cindyorangis left a comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Silvyre commented Jan 22, 2020 • edited Loading

manekenpix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Silvyre commented Jan 24, 2020 • edited Loading

humphd left a comment

Choose a reason for hiding this comment

manekenpix left a comment

Choose a reason for hiding this comment

Silvyre commented Jan 19, 2020 •

edited

Loading

Silvyre commented Jan 22, 2020 •

edited

Loading

Silvyre commented Jan 24, 2020 •

edited

Loading