[UI] Multi user permission separation for artifact api (kubeflow#3522)

* [UI Server] Proxy /namespaces/:namespace/artifacts/get requests to namespace specific artifact services

* [UI] Show artifacts by namespace

* Fix minio artifact link tests

* Fix DetailsTable tests

* Fix OutputArtifactLoader.test

* Change artifact proxy to use query param instead

* Add integration tests for artifact proxy

* Fix unit tests

* Rename service name

* Add comment

* add more comments

* Fix import

* Refactored how to spy on internal methods from tests

frontend/server/.gitignore

@@ -0,0 +1,2 @@
+./BUILD_DATE
+./COMMIT_HASH

frontend/server/app.ts

@@ -19,7 +19,11 @@ import * as proxy from 'http-proxy-middleware';
 import { UIConfigs } from './configs';
 import { getAddress } from './utils';
 import { getBuildMetadata, getHealthzEndpoint, getHealthzHandler } from './handlers/healthz';
-import { getArtifactsHandler } from './handlers/artifacts';
+import {
+  getArtifactsHandler,
+  getArtifactsProxyHandler,
+  getArtifactServiceGetter,
+} from './handlers/artifacts';
 import {
   getCreateTensorboardHandler,
   getTensorboardHandler,
@@ -114,6 +118,14 @@ function createUIServer(options: UIConfigs) {
   );
 
   /** Artifact */
+  registerHandler(
+    app.get,
+    '/artifacts/get',
+    getArtifactsProxyHandler({
+      enabled: options.artifacts.proxy.enabled,
+      namespacedServiceGetter: getArtifactServiceGetter(options.artifacts.proxy),
+    }),
+  );
   registerHandler(app.get, '/artifacts/get', getArtifactsHandler(options.artifacts));
 
   /** Tensorboard viewer */

frontend/server/configs.ts

@@ -13,6 +13,7 @@
 // limitations under the License.
 import * as path from 'path';
 import { loadJSON } from './utils';
+import { loadArtifactsProxyConfig, ArtifactsProxyConfig } from './handlers/artifacts';
 export const BASEPATH = '/pipeline';
 export const apiVersion = 'v1beta1';
 export const apiVersionPrefix = `apis/${apiVersion}`;
@@ -41,10 +42,9 @@ function parseArgs(argv: string[]) {
   return { staticDir, port };
 }
 
-export function loadConfigs(
-  argv: string[],
-  env: NodeJS.ProcessEnv | { [key: string]: string },
-): UIConfigs {
+export type ProcessEnv = NodeJS.ProcessEnv | { [key: string]: string };
+
+export function loadConfigs(argv: string[], env: ProcessEnv): UIConfigs {
   const { staticDir, port } = parseArgs(argv);
   /** All configurable environment variables can be found here. */
   const {
@@ -122,6 +122,7 @@ export function loadConfigs(
         secretKey: MINIO_SECRET_KEY,
         useSSL: asBool(MINIO_SSL),
       },
+      proxy: loadArtifactsProxyConfig(env),
     },
     metadata: {
       envoyService: {
@@ -221,6 +222,7 @@ export interface UIConfigs {
     aws: AWSConfigs;
     minio: MinioConfigs;
     http: HttpConfigs;
+    proxy: ArtifactsProxyConfig;
   };
   argo: ArgoConfigs;
   metadata: MetadataConfigs;

frontend/server/handlers/artifacts.ts

@@ -12,12 +12,13 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 import fetch from 'node-fetch';
-import { AWSConfigs, HttpConfigs, MinioConfigs } from '../configs';
+import { AWSConfigs, HttpConfigs, MinioConfigs, ProcessEnv } from '../configs';
 import { Client as MinioClient } from 'minio';
 import { PreviewStream } from '../utils';
 import { createMinioClient, getObjectStream } from '../minio-helper';
 import { Handler, Request, Response } from 'express';
 import { Storage } from '@google-cloud/storage';
+import * as proxy from 'http-proxy-middleware';
 
 /**
  * ArtifactsQueryStrings describes the expected query strings key value pairs
@@ -237,3 +238,83 @@ function getGCSArtifactHandler(options: { key: string; bucket: string }, peek: n
     }
   };
 }
+
+const AUTH_EMAIL_HEADER = 'x-goog-authenticated-user-email';
+const ARTIFACTS_PROXY_DEFAULTS = {
+  serviceName: 'ml-pipeline-ui-artifact',
+  servicePort: '80',
+};
+export type NamespacedServiceGetter = (namespace: string) => string;
+export interface ArtifactsProxyConfig {
+  serviceName: string;
+  servicePort: number;
+  enabled: boolean;
+}
+export function loadArtifactsProxyConfig(env: ProcessEnv): ArtifactsProxyConfig {
+  const {
+    ARTIFACTS_SERVICE_PROXY_NAME = ARTIFACTS_PROXY_DEFAULTS.serviceName,
+    ARTIFACTS_SERVICE_PROXY_PORT = ARTIFACTS_PROXY_DEFAULTS.servicePort,
+    ARTIFACTS_SERVICE_PROXY_ENABLED = 'false',
+  } = env;
+  return {
+    serviceName: ARTIFACTS_SERVICE_PROXY_NAME,
+    servicePort: parseInt(ARTIFACTS_SERVICE_PROXY_PORT, 10),
+    enabled: ARTIFACTS_SERVICE_PROXY_ENABLED.toLowerCase() === 'true',
+  };
+}
+
+const QUERIES = {
+  NAMESPACE: 'namespace',
+};
+
+export function getArtifactsProxyHandler({
+  enabled,
+  namespacedServiceGetter,
+}: {
+  enabled: boolean;
+  namespacedServiceGetter: NamespacedServiceGetter;
+}): Handler {
+  if (!enabled) {
+    return (req, res, next) => next();
+  }
+  return proxy(
+    (_pathname, req) => {
+      // only proxy requests with namespace query parameter
+      return !!getNamespaceFromUrl(req.url || '');
+    },
+    {
+      changeOrigin: true,
+      onProxyReq: proxyReq => {
+        console.log('Proxied artifact request: ', proxyReq.path);
+      },
+      pathRewrite: (pathStr, req) => {
+        const url = new URL(pathStr || '', DUMMY_BASE_PATH);
+        url.searchParams.delete(QUERIES.NAMESPACE);
+        return url.pathname + url.search;
+      },
+      router: req => {
+        const namespace = getNamespaceFromUrl(req.url || '');
+        if (!namespace) {
+          throw new Error(`namespace query param expected in ${req.url}.`);
+        }
+        return namespacedServiceGetter(namespace);
+      },
+      target: '/artifacts/get',
+    },
+  );
+}
+
+function getNamespaceFromUrl(path: string): string | undefined {
+  // Gets namespace from query parameter "namespace"
+  const params = new URL(path, DUMMY_BASE_PATH).searchParams;
+  return params.get('namespace') || undefined;
+}
+
+// `new URL('/path')` doesn't work, because URL only accepts full URL with scheme and hostname.
+// We use the DUMMY_BASE_PATH like `new URL('/path', DUMMY_BASE_PATH)`, so that URL can parse paths
+// properly.
+const DUMMY_BASE_PATH = 'http://dummy-base-path';
+
+export function getArtifactServiceGetter({ serviceName, servicePort }: ArtifactsProxyConfig) {
+  return (namespace: string) => `http://${serviceName}.${namespace}:${servicePort}`;
+}

frontend/server/integration-tests/artifact-proxy.test.ts

@@ -0,0 +1,147 @@
+import { UIServer } from '../app';
+import { commonSetup, buildQuery } from './test-helper';
+import * as requests from 'supertest';
+import { loadConfigs } from '../configs';
+import * as minioHelper from '../minio-helper';
+import { PassThrough } from 'stream';
+import * as express from 'express';
+import { Server } from 'http';
+import * as artifactsHandler from '../handlers/artifacts';
+
+beforeEach(() => {
+  jest.spyOn(global.console, 'info').mockImplementation();
+  jest.spyOn(global.console, 'log').mockImplementation();
+  jest.spyOn(global.console, 'debug').mockImplementation();
+});
+
+const commonParams = {
+  source: 'minio',
+  bucket: 'ml-pipeline',
+  key: 'hello.txt',
+};
+
+describe('/artifacts/get namespaced proxy', () => {
+  let app: UIServer;
+  const { argv } = commonSetup();
+
+  afterEach(() => {
+    if (app) {
+      app.close();
+    }
+  });
+
+  function setupMinioArtifactDeps({ content }: { content: string }) {
+    const getObjectStreamSpy = jest.spyOn(minioHelper, 'getObjectStream');
+    const objStream = new PassThrough();
+    objStream.end(content);
+    getObjectStreamSpy.mockImplementationOnce(() => Promise.resolve(objStream));
+  }
+
+  let artifactServerInUserNamespace: Server;
+  function setUpNamespacedArtifactService({
+    namespace = 'any-ns',
+    port = 3002,
+  }: {
+    namespace?: string;
+    port?: number;
+  }) {
+    const receivedUrls: string[] = [];
+    const artifactService = express();
+    const response = `artifact service in ${namespace}`;
+    artifactService.all('/*', (req, res) => {
+      receivedUrls.push(req.url);
+      res.status(200).send(response);
+    });
+    artifactServerInUserNamespace = artifactService.listen(port);
+    const getArtifactServiceGetterSpy = jest
+      .spyOn(artifactsHandler, 'getArtifactServiceGetter')
+      .mockImplementation(() => () => `http://localhost:${port}`);
+    return { receivedUrls, getArtifactServiceGetterSpy, response };
+  }
+  afterEach(() => {
+    if (artifactServerInUserNamespace) {
+      artifactServerInUserNamespace.close();
+    }
+  });
+
+  it('is disabled by default', done => {
+    setupMinioArtifactDeps({ content: 'text-data' });
+    const configs = loadConfigs(argv, {});
+    app = new UIServer(configs);
+    requests(app.start())
+      .get(
+        `/artifacts/get${buildQuery({
+          ...commonParams,
+          namespace: 'ns2',
+        })}`,
+      )
+      .expect(200, 'text-data', done);
+  });
+
+  it('proxies a request to namespaced artifact service', done => {
+    const { receivedUrls, getArtifactServiceGetterSpy } = setUpNamespacedArtifactService({
+      namespace: 'ns2',
+    });
+    const configs = loadConfigs(argv, {
+      ARTIFACTS_SERVICE_PROXY_NAME: 'artifact-svc',
+      ARTIFACTS_SERVICE_PROXY_PORT: '80',
+      ARTIFACTS_SERVICE_PROXY_ENABLED: 'true',
+    });
+    app = new UIServer(configs);
+    requests(app.start())
+      .get(
+        `/artifacts/get${buildQuery({
+          ...commonParams,
+          namespace: 'ns2',
+        })}`,
+      )
+      .expect(200, 'artifact service in ns2', err => {
+        expect(getArtifactServiceGetterSpy).toHaveBeenCalledWith({
+          serviceName: 'artifact-svc',
+          servicePort: 80,
+          enabled: true,
+        });
+        expect(receivedUrls).toEqual(
+          // url is the same, except namespace query is omitted
+          ['/artifacts/get?source=minio&bucket=ml-pipeline&key=hello.txt'],
+        );
+        done(err);
+      });
+  });
+
+  it('does not proxy requests without namespace argument', done => {
+    setupMinioArtifactDeps({ content: 'text-data2' });
+    const configs = loadConfigs(argv, { ARTIFACTS_SERVICE_PROXY_ENABLED: 'true' });
+    app = new UIServer(configs);
+    requests(app.start())
+      .get(
+        `/artifacts/get${buildQuery({
+          ...commonParams,
+          namespace: undefined,
+        })}`,
+      )
+      .expect(200, 'text-data2', done);
+  });
+
+  it('proxies a request with basePath too', done => {
+    const { receivedUrls, response } = setUpNamespacedArtifactService({});
+    const configs = loadConfigs(argv, {
+      ARTIFACTS_SERVICE_PROXY_ENABLED: 'true',
+    });
+    app = new UIServer(configs);
+    requests(app.start())
+      .get(
+        `/pipeline/artifacts/get${buildQuery({
+          ...commonParams,
+          namespace: 'ns-any',
+        })}`,
+      )
+      .expect(200, response, err => {
+        expect(receivedUrls).toEqual(
+          // url is the same with base path, except namespace query is omitted
+          ['/pipeline/artifacts/get?source=minio&bucket=ml-pipeline&key=hello.txt'],
+        );
+        done(err);
+      });
+  });
+});

frontend/server/integration-tests/test-helper.ts

@@ -0,0 +1,49 @@
+import * as path from 'path';
+import * as os from 'os';
+import * as fs from 'fs';
+
+export function commonSetup() {
+  const indexHtmlPath = path.resolve(os.tmpdir(), 'index.html');
+  const argv = ['node', 'dist/server.js', os.tmpdir(), '3000'];
+  const buildDate = new Date().toISOString();
+  const commitHash = 'abcdefg';
+  const indexHtmlContent = `
+<html>
+<head>
+  <script>
+  window.KFP_FLAGS.DEPLOYMENT=null
+  </script>
+  <script id="kubeflow-client-placeholder"></script>
+</head>
+</html>`;
+
+  beforeAll(() => {
+    fs.writeFileSync(path.resolve(__dirname, 'BUILD_DATE'), buildDate);
+    fs.writeFileSync(path.resolve(__dirname, 'COMMIT_HASH'), commitHash);
+    fs.writeFileSync(indexHtmlPath, indexHtmlContent);
+  });
+
+  afterAll(() => {
+    fs.unlinkSync(path.resolve(__dirname, 'BUILD_DATE'));
+    fs.unlinkSync(path.resolve(__dirname, 'COMMIT_HASH'));
+    fs.unlinkSync(indexHtmlPath);
+  });
+
+  beforeEach(() => {
+    jest.resetAllMocks();
+    jest.restoreAllMocks();
+  });
+
+  return { argv };
+}
+
+export function buildQuery(queriesMap: { [key: string]: string | undefined }): string {
+  const queryContent = Object.entries(queriesMap)
+    .filter((entry): entry is [string, string] => entry[1] != null)
+    .map(([key, value]) => `${key}=${encodeURIComponent(value)}`)
+    .join('&');
+  if (!queryContent) {
+    return '';
+  }
+  return `?${queryContent}`;
+}

frontend/server/package.json

@@ -36,7 +36,7 @@
     "format": "npx prettier --write './**/*.{ts,tsx}'",
     "format:check": "npx prettier --check './**/*.{ts,tsx}' || node ../scripts/check-format-error-info.js",
     "lint": "npx tslint -c tslint.json -p tsconfig.json",
-    "test": "npx jest",
+    "test": "npx jest --runInBand",
     "test:coverage": "npm test -- --coverage",
     "test:ci": "npm run format:check && npm run lint && npm run test:coverage"
   },