PhET-iO Graphing Quadratics 1.1 maintenance releases should not be password protected

[samreid]

In https://github.com/phetsims/phet-io-website/issues/108 we decided Graphing Quadratics 1.1 PhET-iO should not be password protected, so it can be used for reference by clients without a PhET-iO account.

We can manually modify the existing 1.1.1 publication to remove password protection, but we should set up perennial to take care of this step in the future so we don't have to manually remove password protection for each maintenance release.

@zepumph and I revised taskWorker to try to accomplish this, but thought it would be best to run it past @mattpen before committing. @mattpen does this plan and implementation seem good to you?

Index: js/build-server/taskWorker.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/build-server/taskWorker.js	(revision 8a76c5d85a9d5b776642f98abdd4ff2cbe4c78f9)
+++ js/build-server/taskWorker.js	(date 1566516374102)
@@ -321,19 +321,25 @@
             await notifyServer( simName, email, brand );
 
             // if this build request comes from rosetta it will have a userId field and only one locale
-            const localesArray = typeof( locales ) === 'string' ? locales.split( ',' ) : locales;
+            const localesArray = typeof ( locales ) === 'string' ? locales.split( ',' ) : locales;
             if ( userId && localesArray.length === 1 && localesArray[ 0 ] !== '*' ) {
               await addTranslator( localesArray[ 0 ], simName, userId );
             }
           }
           else if ( brand === constants.PHET_IO_BRAND ) {
             const suffix = originalVersion.split( '-' ).length >= 2 ? originalVersion.split( '-' )[ 1 ] : ( chipperVersion.major < 2 ? 'phetio' : '' );
-            await notifyServer( simName, email, brand, { branch: branch, suffix: suffix, version: SimVersion.parse( version, '' ) } );
+            const parsedVersion = SimVersion.parse( version, '' );
+            await notifyServer( simName, email, brand, { branch: branch, suffix: suffix, version: parsedVersion } );
             winston.debug( 'server notified' );
-            await writePhetioHtaccess(
-              targetVersionDir,
-              { simName: simName, version: originalVersion, directory: constants.PHET_IO_SIMS_DIRECTORY }
-            );
+
+            // Password protect all public releases, except the maintenance releases for Graphing Quadratics 1.1, which
+            // must be public so they can appear with the devguide.
+            if ( simName !== 'graphing-quadratics' && parsedVersion.major === 1 && parsedVersion.minor === 1 ) {
+              await writePhetioHtaccess(
+                targetVersionDir,
+                { simName: simName, version: originalVersion, directory: constants.PHET_IO_SIMS_DIRECTORY }
+              );
+            }
             winston.debug( 'phetio htaccess written' );
           }
         }

[zepumph]

@chrisklus and I found a bug in the boolean logic in the patch above. It should be

!(simName === 'graphing-quadratics' && parsedVersion.major === 1 && parsedVersion.minor === 1).

We reviewed the patch and ended up committing it above. Next steps for this issue:

• Note this work is currently blocking production deploys of
• @mattpen review the commit
• @mattpen or @jonathanolson retart the build server
• Unblock publication of the maintenance release in Maintenance release testing for popup fixes qa#407 (comment)

I'm going to mark this as a high priority because of it blocking a maintenance release. @jonathanolson please reevaluate as necessary.

[jonathanolson]

The patch looks good to me. I'm going to pull and restart the build-server for this.

[zepumph]

bb693fd did not work. writePhetioHtaccess also handles updating the redirect rules. So the logic needs to surround just the wrapper passwork protection. Not rewriting the htaccess that exists at the top level of the sim. I will propose a patch soon.

[zepumph]

In the below patch I added support for the needed logic to be passed into writePhetioHtaccess. This made the most sense to do by adding an options object for the variety of options that it currently supports. @mattpen I'm sorry this issue got a bit out of hand, because of me. Could you please comment on the below patch. The goal is to make sure that GQ 1.1 branch is public for all maintenance releases in the future.

Index: js/build-server/taskWorker.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/build-server/taskWorker.js	(revision bb693fd10d1d438d2bc2fa1f0d0890ffb4322a75)
+++ js/build-server/taskWorker.js	(date 1566594749786)
@@ -332,18 +332,14 @@
             await notifyServer( simName, email, brand, { branch: branch, suffix: suffix, version: parsedVersion } );
             winston.debug( 'server notified' );
 
-            // Password protect all public releases, except the maintenance releases for Graphing Quadratics 1.1, which
-            // must be public so they can appear with the devguide, see https://github.com/phetsims/perennial/issues/148
-            if ( !( simName === 'graphing-quadratics' && parsedVersion.major === 1 && parsedVersion.minor === 1 ) ) {
-              await writePhetioHtaccess(
-                targetVersionDir,
-                { simName: simName, version: originalVersion, directory: constants.PHET_IO_SIMS_DIRECTORY }
-              );
-              winston.debug( 'phetio htaccess written' );
-            }
-            else {
-              winston.debug( 'phetio htaccess not written for graphing-quadratics' );
-            }
+            await writePhetioHtaccess( targetVersionDir, {
+              latestOption: { simName: simName, version: originalVersion, directory: constants.PHET_IO_SIMS_DIRECTORY },
+
+              // Password protect all public releases, except the maintenance releases for Graphing Quadratics 1.1, which
+              // must be public so they can appear with the devguide, see https://github.com/phetsims/perennial/issues/148
+              passwordProtectWrappers: !( simName === 'graphing-quadratics' && parsedVersion.major === 1 && parsedVersion.minor === 1 )
+            } );
+            winston.debug( 'phetio htaccess written' );
           }
         }
       }
Index: js/common/writePhetioHtaccess.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/writePhetioHtaccess.js	(revision bb693fd10d1d438d2bc2fa1f0d0890ffb4322a75)
+++ js/common/writePhetioHtaccess.js	(date 1566594749777)
@@ -12,32 +12,46 @@
 /**
  * Writes the htaccess file to password protect the exclusive content for phet-io sims
  * @param {string} passwordProtectPath - deployment location
- * @param {object} [latestOption] - if provided, then write the /latest/ redirect .htaccess file.
- *                                - this is only to be used for production deploys by the build-server
- * @param {string} [devVersionPath] - if provided, scp the htaccess files to here, relatively
- * @property {string} latestOption.simName
- * @property {string} latestOption.version
- * @property {string} latestOption.directory
- */
-module.exports = async function writePhetioHtaccess( passwordProtectPath, latestOption, devVersionPath ) {
-  const authFilepath = '/etc/httpd/conf/phet-io_pw';
+ * @param {Object} [options]
+ */
+module.exports = async function writePhetioHtaccess( passwordProtectPath, options ) {
+  const authFilepath = '/etc/httpd/conf/phet-io_pw';
+
+  options = _.extend( {
+
+    /**
+     * if provided, then write the /latest/ redirect .htaccess file.
+     * this is only to be used for production deploys by the build-server
+     * @type {Object}
+     * @property {string} latestOption.simName
+     * @property {string} latestOption.version
+     * @property {string} latestOption.directory
+     */
+    latestOption: null,
+
+    // {string} - if provided, scp the htaccess files to here, relatively
+    devVersionPath: null,
+
+    // {boolean} - set to false to keep the wrappers dir public. NOTE: PhET-iO has IP that PhET wants to protect, do this cautiously.
+    passwordProtectWrappers: false
+  }, options );
 
   // This option is for production deploys by the build-server
   // If we are provided a simName and version then write a .htaccess file to redirect
   // https://phet-io.colorado.edu/sims/{{sim-name}}/{{major}}.{{minor}} to https://phet-io.colorado.edu/sims/{{sim-name}}/{{major}}.{{minor}}.{{latest}}{{[-suffix]}}
-  if ( latestOption ) {
-    if ( latestOption.simName && latestOption.version && latestOption.directory ) {
-      const redirectFilepath = latestOption.directory + latestOption.simName + '/.htaccess';
+  if ( options.latestOption ) {
+    if ( options.latestOption.simName && options.latestOption.version && options.latestOption.directory ) {
+      const redirectFilepath = options.latestOption.directory + options.latestOption.simName + '/.htaccess';
       let latestRedirectContents = 'RewriteEngine on\n' +
-                                   `RewriteBase /sims/${latestOption.simName}/\n`;
-      const versions = JSON.parse( await request( buildLocal.productionServerURL + `/services/metadata/phetio?name=${latestOption.simName}&latest=true` ) );
+                                   `RewriteBase /sims/${options.latestOption.simName}/\n`;
+      const versions = JSON.parse( await request( buildLocal.productionServerURL + `/services/metadata/phetio?name=${options.latestOption.simName}&latest=true` ) );
       for ( const v of versions ) {
         // Add a trailing slash to /sims/sim-name/x.y
         latestRedirectContents += `RewriteRule ^${v.versionMajor}.${v.versionMinor}$ ${v.versionMajor}.${v.versionMinor}/ [R=301,L]\n`;
         // Rewrite /sims/sim-name/x.y/* to /sims/sim-name/x.y.z/*
         latestRedirectContents += `RewriteRule ^${v.versionMajor}.${v.versionMinor}/(.*) ${v.versionMajor}.${v.versionMinor}.${v.versionMaintenance}${v.versionSuffix ? '-' : ''}${v.versionSuffix}/$1\n`;
       }
-      // 'RewriteRule latest(.*) ' + latestOption.version + '$1\n';
+      // 'RewriteRule latest(.*) ' + options.latestOption.version + '$1\n';
       latestRedirectContents += 'RewriteCond %{QUERY_STRING} =download\n' +
                                 'RewriteRule ([^/]*)$ - [L,E=download:$1]\n' +
                                 'Header onsuccess set Content-disposition "attachment; filename=%{download}e" env=download\n';
@@ -49,38 +63,39 @@
       }
     }
     else {
-      winston.error( `simName: ${latestOption.simName}` );
-      winston.error( `version: ${latestOption.version}` );
-      winston.error( `directory: ${latestOption.directory}` );
-      return Promise.reject( 'latestOption is missing one of the required parameters (simName, version, or directory)' );
+      winston.error( `simName: ${options.latestOption.simName}` );
+      winston.error( `version: ${options.latestOption.version}` );
+      winston.error( `directory: ${options.latestOption.directory}` );
+      return Promise.reject( 'options.latestOption is missing one of the required parameters (simName, version, or directory)' );
     }
   }
 
-  // Always write a file to add authentication to the ./wrappers directory
-  try {
-    const passwordProtectWrapperContents = 'AuthType Basic\n' +
-                                           'AuthName "PhET-iO Password Protected Area"\n' +
-                                           'AuthUserFile ' + authFilepath + '\n' +
-                                           'Require valid-user\n';
-    let filePath = 'wrappers/.htaccess';
-    await writeFile( `${passwordProtectPath}/${filePath}`, passwordProtectWrapperContents );
-    if ( devVersionPath ) {
-      await devScp( `${passwordProtectPath}/${filePath}`, `${devVersionPath}/phet-io/${filePath}` );
-    }
+  if ( options.passwordProtectWrappers ) {
+    try {
+      const passwordProtectWrapperContents = 'AuthType Basic\n' +
+                                             'AuthName "PhET-iO Password Protected Area"\n' +
+                                             'AuthUserFile ' + authFilepath + '\n' +
+                                             'Require valid-user\n';
+      let filePath = 'wrappers/.htaccess';
+      await writeFile( `${passwordProtectPath}/${filePath}`, passwordProtectWrapperContents );
+      if ( options.devVersionPath ) {
+        await devScp( `${passwordProtectPath}/${filePath}`, `${options.devVersionPath}/phet-io/${filePath}` );
+      }
 
-    const phetioPackage = JSON.parse( fs.readFileSync( '../phet-io/package.json' ) );
-    if ( phetioPackage.phet && phetioPackage.phet.addRootHTAccessFile ) {
-      const passwordProtectIndexContents = '<FilesMatch "index.*">\n'
-                                           + passwordProtectWrapperContents
-                                           + '</FilesMatch>\n';
-      filePath = '.htaccess';
-      await writeFile( `${passwordProtectPath}/${filePath}`, passwordProtectIndexContents );
-      if ( devVersionPath ) {
-        await devScp( `${passwordProtectPath}/${filePath}`, `${devVersionPath}/phet-io/${filePath}` );
-      }
-    }
-  }
-  catch( err ) {
-    return Promise.reject( err );
+      const phetioPackage = JSON.parse( fs.readFileSync( '../phet-io/package.json' ) );
+      if ( phetioPackage.phet && phetioPackage.phet.addRootHTAccessFile ) {
+        const passwordProtectIndexContents = '<FilesMatch "index.*">\n'
+                                             + passwordProtectWrapperContents
+                                             + '</FilesMatch>\n';
+        filePath = '.htaccess';
+        await writeFile( `${passwordProtectPath}/${filePath}`, passwordProtectIndexContents );
+        if ( options.devVersionPath ) {
+          await devScp( `${passwordProtectPath}/${filePath}`, `${options.devVersionPath}/phet-io/${filePath}` );
+        }
+      }
+    }
+    catch( err ) {
+      return Promise.reject( err );
+    }
   }
 };
Index: js/grunt/dev.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/grunt/dev.js	(revision bb693fd10d1d438d2bc2fa1f0d0890ffb4322a75)
+++ js/grunt/dev.js	(date 1566594749781)
@@ -142,7 +142,9 @@
   // https://github.com/phetsims/phet-io/issues/641
   if ( brands.includes( 'phet-io' ) && buildLocal.devDeployServer === 'bayes.colorado.edu' ) {
     const htaccessLocation = `../${repo}/build/phet-io/`;
-    await writePhetioHtaccess( htaccessLocation, null, versionPath );
+    await writePhetioHtaccess( htaccessLocation, {
+      devVersionPath: versionPath
+    } );
   }
 
   // Move over dependencies.json and commit/push

[mattpen]

Will this be the only time we want to deploy a sim with public wrappers? Would it be better to make this an option sent to the build-server rather than hard coding in GQ v1.1 into the build server?

Hard-coding it in doesn't seem very robust to me, but perhaps I'm missing the context.

---

@zepumph - The diff in #148 (comment) looks ok to me. There is a significant argument/options refactor in addition to the logic change for GQ so I'm less than 100% confident. Do we want to test it before deploying to production? I think ox-dev is setup to test the build-server relatively easily, I could help with this if needed.

[samreid]

I agree this would be better not to hard-code into perennial. If perennial can read package.json, perhaps we could add a new metadata entry there (in the branch)?

[mattpen]

@samreid - Yes. This could be handled like we are handling the optional root htaccess file as described in https://github.com/phetsims/phet-io-wrappers/issues/135.

We could put it in package.json either for the 1.1 branch of GQ or the graphing-quadratics-1.1 branch of phet-io.

[samreid]

We could put it in package.json either for the 1.1 branch of GQ or the graphing-quadratics-1.1 branch of phet-io.

The 1.1 branch of GQ sounds best. Who should take the lead on the implementation for this issue?

[zepumph]

Looking through https://github.com/phetsims/phet-io-wrappers/issues/135, I think it is best to add something to package that specifies if we password protect the wrappers.

• What is the name of the key? For parallelism I think a good name is addWrapperHTAccessFile.
• Would this be nested under the phet.phet-io key, or just under phet?
• @mattpen do you mind taking this on? I could work on some pieces of this, but I would largely be unable to test the build-server stuff. I'd also be happy to pair if that would be easier.

Tagging @samreid to sign off before implementation.

[samreid]

What is the name of the key? For parallelism I think a good name is addWrapperHTAccessFile
Would this be nested under the phet.phet-io key, or just under phet?

Being password-protected should be the default, so perhaps the key should be named around being opt-in for being public, and this seems it should be in phet-io only.

  "phet": {
    "phet-io": {
      "isProductionVersionPublic": true
    }
  }

[mattpen]

@mattpen do you mind taking this on?

@zepumph @samreid - When is this needed? I'm happy to work on it, but unless it is top/very high priority I can't address it immediately.

[samreid]

Until this is fixed, every time we run a maintenance release, graphing-quadratics PhET-iO becomes password protected again. Hence if @kathy-phet shares the PhET-iO documentation with a new client right after a maintenance release, the client may run into a password dialog, before they have a password. We do not have a process in place for maintenance releasers to do this step manually or to communicate with @zepumph and me to make the manual step.

Also tagging @jonathanolson since me mentioned a maintenance release process on slack earlier today. I did check https://phet-io.colorado.edu/sims/graphing-quadratics/1.1/ just now and found it is correctly not password protected, perhaps the maintenance release process hasn't finished yet.

[jonathanolson]

So we'll probably have one or two maintenance releases finish this week. What's the desired short-term plan?

[zepumph]

@mattpen and I worked on this today. We added support for a package.json flag called allowPublicAccess in phet[ 'phet-io' ]. This felt like a good place because it is very much phet-io brand specific. When this is present only the production deploy will be public, not rc/dev versions (this includes the rc that is published on production deploy).

This ended up being a bit more complicated than expected because we had to add logic that would remove the local htaccess files from the rc deploy that occurs on production. It wasn't enough to just "not create" htaccess files, but instead we have to make sure they don't exist locally at all.

We also tacked on work for #149 in this issue.
https://github.com/phetsims/website/commit/53a0022acc5bbea42807cba0b73caadb1c0b0ad4
f69bd49
8fb47dd
are all commits that belong in that issue.

We ended up getting the public piece working on phet-io-test-sim 1.3.2, so that was published without password protection, but once I confirmed that it was working, I went in and manually copied .htaccess files into that version to make is private again. I also removed allowPublicAccess from phet-io-test-sim, so the only spot this is located is in GQ's 1.1 branch.

To test and make sure that all works, we tested the following combinations, and all worked as expected

phet-io-test-sim	yes allowPublicAccess	no allowPublicAccess
dev
rc
production

and everything worked as expected. The only public one is allowPublicAccess with production.

I redeployed GQ 1.1 once these changes were all set. And the most recent MR is public, https://phet-io.colorado.edu/sims/graphing-quadratics/1.1.3/

It also successfully rewrote the sim level htaccess so the latest rule is fixed also (https://phet-io.colorado.edu/sims/graphing-quadratics/1.1/).

I don't think there is anything else here, but since @mattpen had to leave before the end of this work, I wonder if you wouldn't mind reviewing the tail end of this work. Is there any more cleanup to be done here?

[mattpen]

I wonder if you wouldn't mind reviewing the tail end of this work. Is there any more cleanup to be done here?

I reviewed the next few commits and everything looks great. Thanks for finishing this up.

we tested the following combinations, and all worked as expected

This sounds ok to close.