App: revert addition of extra flag --internal-login-pages

See discussion at https://github.com/nativefier/nativefier/pull/1124#issuecomment-794751514 :

> @TheCleric I was about to merge this, then reconsidered one little thing (yes I wrote "little", I'm not reconsidering this whole thing 😅).
>
> I'm re-considering having the extra flag. I'm not so sure this will harm a lot of use cases. I'd like to 1. merge this PR, 2. immediately follow up with a small commit removing the flag & adjusting api.md, 3. release with the change well-documented / asking for feedback if this is problematic to anyone. (I'm not asking you any extra work, and like leaving an in-tree commit trace of considering the flag). If people complain with a valid reason, we'll restore the flag with a quick revert, else we're happy with one less flag and a reasonably-handled breaking change.
>
> Thoughts / objections?

Answered by:

> That seems reasonable to me.
>
> [discussion on an extra structured way to pass flags]

app/src/components/mainWindow.ts

@@ -225,14 +225,7 @@ export function createMainWindow(
   };
 
   const onWillNavigate = (event: Event, urlToGo: string): void => {
-    if (
-      !linkIsInternal(
-        options.targetUrl,
-        urlToGo,
-        options.internalLoginPages,
-        options.internalUrls,
-      )
-    ) {
+    if (!linkIsInternal(options.targetUrl, urlToGo, options.internalUrls)) {
       event.preventDefault();
       if (options.blockExternalUrls) {
         onBlockedExternalUrl(urlToGo);
@@ -301,7 +294,6 @@ export function createMainWindow(
       urlToGo,
       disposition,
       options.targetUrl,
-      options.internalLoginPages,
       options.internalUrls,
       preventDefault,
       shell.openExternal.bind(this),

app/src/components/mainWindowHelpers.test.ts

@@ -21,7 +21,6 @@ test('internal urls should not be handled', () => {
     internalUrl,
     undefined,
     originalUrl,
-    true,
     undefined,
     preventDefault,
     openExternal,
@@ -50,7 +49,6 @@ test('external urls should be opened externally', () => {
     externalUrl,
     undefined,
     originalUrl,
-    true,
     undefined,
     preventDefault,
     openExternal,
@@ -80,7 +78,6 @@ test('external urls should be ignored if blockExternal is true', () => {
     externalUrl,
     undefined,
     originalUrl,
-    true,
     undefined,
     preventDefault,
     openExternal,
@@ -109,7 +106,6 @@ test('tab disposition should be ignored if tabs are not enabled', () => {
     internalUrl,
     foregroundDisposition,
     originalUrl,
-    true,
     undefined,
     preventDefault,
     openExternal,
@@ -138,7 +134,6 @@ test('tab disposition should be ignored if url is external', () => {
     externalUrl,
     foregroundDisposition,
     originalUrl,
-    true,
     undefined,
     preventDefault,
     openExternal,
@@ -167,7 +162,6 @@ test('foreground tabs with internal urls should be opened in the foreground', ()
     internalUrl,
     foregroundDisposition,
     originalUrl,
-    true,
     undefined,
     preventDefault,
     openExternal,
@@ -197,7 +191,6 @@ test('background tabs with internal urls should be opened in background tabs', (
     internalUrl,
     backgroundDisposition,
     originalUrl,
-    true,
     undefined,
     preventDefault,
     openExternal,
@@ -227,7 +220,6 @@ test('about:blank urls should be handled', () => {
     'about:blank',
     undefined,
     originalUrl,
-    true,
     undefined,
     preventDefault,
     openExternal,

app/src/components/mainWindowHelpers.ts

@@ -4,7 +4,6 @@ export function onNewWindowHelper(
   urlToGo: string,
   disposition: string,
   targetUrl: string,
-  internalLoginPages: boolean,
   internalUrls: string | RegExp,
   preventDefault,
   openExternal,
@@ -14,7 +13,7 @@ export function onNewWindowHelper(
   blockExternal: boolean,
   onBlockedExternalUrl: (url: string) => void,
 ): void {
-  if (!linkIsInternal(targetUrl, urlToGo, internalLoginPages, internalUrls)) {
+  if (!linkIsInternal(targetUrl, urlToGo, internalUrls)) {
     preventDefault();
     if (blockExternal) {
       onBlockedExternalUrl(urlToGo);

app/src/helpers/helpers.test.ts

@@ -10,61 +10,46 @@ const externalUrl = 'https://www.wikipedia.org/wiki/Electron';
 const wildcardRegex = /.*/;
 
 test('the original url should be internal', () => {
-  expect(linkIsInternal(internalUrl, internalUrl, false, undefined)).toEqual(
-    true,
-  );
+  expect(linkIsInternal(internalUrl, internalUrl, undefined)).toEqual(true);
 });
 
 test('sub-paths of the original url should be internal', () => {
   expect(
-    linkIsInternal(
-      internalUrl,
-      internalUrl + internalUrlSubPath,
-      false,
-      undefined,
-    ),
+    linkIsInternal(internalUrl, internalUrl + internalUrlSubPath, undefined),
   ).toEqual(true);
 });
 
 test("'about:blank' should be internal", () => {
-  expect(linkIsInternal(internalUrl, 'about:blank', false, undefined)).toEqual(
-    true,
-  );
+  expect(linkIsInternal(internalUrl, 'about:blank', undefined)).toEqual(true);
 });
 
 test('urls from different sites should not be internal', () => {
-  expect(linkIsInternal(internalUrl, externalUrl, false, undefined)).toEqual(
-    false,
-  );
+  expect(linkIsInternal(internalUrl, externalUrl, undefined)).toEqual(false);
 });
 
 test('all urls should be internal with wildcard regex', () => {
-  expect(
-    linkIsInternal(internalUrl, externalUrl, false, wildcardRegex),
-  ).toEqual(true);
+  expect(linkIsInternal(internalUrl, externalUrl, wildcardRegex)).toEqual(true);
 });
 
 test('a "www." of a domain should be considered internal', () => {
-  expect(linkIsInternal(internalUrl, internalUrlWww, false, undefined)).toEqual(
+  expect(linkIsInternal(internalUrl, internalUrlWww, undefined)).toEqual(true);
+});
+
+test('urls on the same "base domain" should be considered internal', () => {
+  expect(linkIsInternal(internalUrl, sameBaseDomainUrl, undefined)).toEqual(
     true,
   );
 });
 
-test('urls on the same "base domain" should be considered internal', () => {
-  expect(
-    linkIsInternal(internalUrl, sameBaseDomainUrl, false, undefined),
-  ).toEqual(true);
-});
-
 test('urls on the same "base domain" should be considered internal, even with a www', () => {
-  expect(
-    linkIsInternal(internalUrlWww, sameBaseDomainUrl, false, undefined),
-  ).toEqual(true);
+  expect(linkIsInternal(internalUrlWww, sameBaseDomainUrl, undefined)).toEqual(
+    true,
+  );
 });
 
 test('urls on the same "base domain" should be considered internal, long SLD', () => {
   expect(
-    linkIsInternal(internalUrlCoUk, sameBaseDomainUrlCoUk, false, undefined),
+    linkIsInternal(internalUrlCoUk, sameBaseDomainUrlCoUk, undefined),
   ).toEqual(true);
 });
 
@@ -98,20 +83,9 @@ const testLoginPages = [
 ];
 
 test.each(testLoginPages)(
-  '%s login page should be internal if internalLoginPages is enabled',
+  '%s login page should be internal',
   (loginUrl: string) => {
-    expect(linkIsInternal(internalUrl, loginUrl, true, undefined)).toEqual(
-      true,
-    );
-  },
-);
-
-test.each(testLoginPages)(
-  '%s login page should not be internal if internalLoginPages is disabled',
-  (loginUrl: string) => {
-    expect(linkIsInternal(internalUrl, loginUrl, false, undefined)).toEqual(
-      false,
-    );
+    expect(linkIsInternal(internalUrl, loginUrl, undefined)).toEqual(true);
   },
 );
 

app/src/helpers/helpers.ts

@@ -36,14 +36,13 @@ function isInternalLoginPage(url: string): boolean {
 export function linkIsInternal(
   currentUrl: string,
   newUrl: string,
-  internalLoginPages: boolean,
   internalUrlRegex: string | RegExp,
 ): boolean {
   if (newUrl === 'about:blank') {
     return true;
   }
 
-  if (internalLoginPages && isInternalLoginPage(newUrl)) {
+  if (isInternalLoginPage(newUrl)) {
     return true;
   }
 

docs/api.md

@@ -43,7 +43,6 @@
   - [[ignore-gpu-blacklist]](#ignore-gpu-blacklist)
   - [[enable-es3-apis]](#enable-es3-apis)
   - [[insecure]](#insecure)
-  - [[internal-login-pages]](#internal-login-pages)
   - [[internal-urls]](#internal-urls)
   - [[block-external-urls]](#block-external-urls)
   - [[proxy-rules]](#proxy-rules)
@@ -395,30 +394,6 @@ Passes the enable-es3-apis flag to the Chrome engine, to force the activation of
 
 Forces the packaged app to ignore web security errors, such as [Mixed Content](https://developer.mozilla.org/en-US/docs/Security/Mixed_content) errors when receiving HTTP content on a HTTPS site.
 
-#### [internal-login-pages]
-
-```
---internal-login-pages <boolean>
-```
-
-Whether to consider known login providers to be internal, so as to avoid login urls being opened in a browser without being able to login to the application. Default: true
-
-Known login providers:
-- Amazon
-- Facebook
-- GitHub
-- Google
-- LinkedIn
-- Microsoft
-- Okta
-- Twitter
-
-Example:
-
-```bash
-nativefier https://google.com --internal-login-pages false
-```
-
 #### [internal-urls]
 
 ```

src/build/prepareElectronApp.ts

@@ -44,7 +44,6 @@ function pickElectronAppArgs(options: AppOptions): any {
     ignoreCertificate: options.nativefier.ignoreCertificate,
     ignoreGpuBlacklist: options.nativefier.ignoreGpuBlacklist,
     insecure: options.nativefier.insecure,
-    internalLoginPages: options.nativefier.internalLoginPages,
     internalUrls: options.nativefier.internalUrls,
     blockExternalUrls: options.nativefier.blockExternalUrls,
     maxHeight: options.nativefier.maxHeight,

src/cli.ts

@@ -9,11 +9,7 @@ import * as log from 'loglevel';
 import { isArgFormatInvalid } from './helpers/helpers';
 import { supportedArchs, supportedPlatforms } from './infer/inferOs';
 import { buildNativefierApp } from './main';
-import {
-  parseBoolean,
-  parseBooleanOrString,
-  parseJson,
-} from './utils/parseUtils';
+import { parseBooleanOrString, parseJson } from './utils/parseUtils';
 
 // package.json is `require`d to let tsc strip the `src` folder by determining
 // baseUrl=src. A static import would prevent that and cause an ugly extra "src" folder
@@ -212,11 +208,6 @@ if (require.main === module) {
       'default zoom factor to use when the app is opened; defaults to 1.0',
       parseFloat,
     )
-    .option(
-      '--internal-login-pages',
-      "Force known login pages (Amazon, Facebook, GitHub, Google, LinkedIn, Microsoft, Okta, Twitter) to be internal urls so they don't have to be individually specified. Default: true",
-      (value) => parseBoolean(value, true),
-    )
     .option(
       '--internal-urls <value>',
       'regex of URLs to consider "internal"; all other URLs will be opened in an external browser. Default: URLs on same second-level domain as app',

src/integration-test.ts

@@ -36,13 +36,6 @@ function checkApp(appRoot: string, inputOptions: any): void {
   const iconPath = path.join(appPath, iconFile);
   expect(fs.existsSync(iconPath)).toBe(true);
   expect(fs.statSync(iconPath).size).toBeGreaterThan(1000);
-
-  // Internal Login Pages enabled by default
-  expect(nativefierConfig.internalLoginPages).toBe(
-    inputOptions.internalLoginPages === undefined
-      ? true
-      : inputOptions.internalLoginPages,
-  );
 }
 
 describe('Nativefier', () => {
@@ -62,17 +55,4 @@ describe('Nativefier', () => {
       checkApp(appPath, options);
     },
   );
-
-  test('can disable internalLoginPages', async () => {
-    const tempDirectory = getTempDir('integtest');
-    const options = {
-      targetUrl: 'https://google.com/',
-      out: tempDirectory,
-      overwrite: true,
-      platform: 'linux',
-      internalLoginPages: false,
-    };
-    const appPath = await buildNativefierApp(options);
-    checkApp(appPath, options);
-  });
 });

src/options/model.ts

@@ -34,7 +34,6 @@ export interface AppOptions {
     ignoreGpuBlacklist: boolean;
     inject: string[];
     insecure: boolean;
-    internalLoginPages: boolean;
     internalUrls: string;
     blockExternalUrls: boolean;
     maximize: boolean;

src/options/optionsMain.ts

@@ -74,11 +74,6 @@ export async function getOptions(rawOptions: any): Promise<AppOptions> {
       ignoreGpuBlacklist: rawOptions.ignoreGpuBlacklist || false,
       inject: rawOptions.inject || [],
       insecure: rawOptions.insecure || false,
-      internalLoginPages:
-        rawOptions.internalLoginPages === undefined ||
-        typeof rawOptions.internalLoginPages !== 'boolean'
-          ? true
-          : rawOptions.internalLoginPages,
       internalUrls: rawOptions.internalUrls || null,
       blockExternalUrls: rawOptions.blockExternalUrls || false,
       maximize: rawOptions.maximize || false,