Apply review comments of #1507

chart/kubeapps/templates/kubeops-rbac.yaml

@@ -45,7 +45,7 @@ subjects:
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:
-  name: {{ template "kubeapps.kubeops.fullname" . }}-ns-discovery
+  name: "kubeapps:controller:kubeops-ns-discovery-{{ .Release.Namespace }}"
   labels:
     app: {{ template "kubeapps.kubeops.fullname" . }}
     chart: {{ template "kubeapps.chart" . }}
@@ -62,7 +62,7 @@ rules:
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding
 metadata:
-  name: {{ template "kubeapps.kubeops.fullname" . }}-ns-discovery
+  name: "kubeapps:controller:kubeops-ns-discovery-{{ .Release.Namespace }}"
   labels:
     app: {{ template "kubeapps.kubeops.fullname" . }}
     chart: {{ template "kubeapps.chart" . }}
@@ -71,7 +71,7 @@ metadata:
 roleRef:
   apiGroup: rbac.authorization.k8s.io
   kind: ClusterRole
-  name: {{ template "kubeapps.kubeops.fullname" . }}-ns-discovery
+  name: "kubeapps:controller:kubeops-ns-discovery-{{ .Release.Namespace }}"
 subjects:
   - kind: ServiceAccount
     name: {{ template "kubeapps.kubeops.fullname" . }}

chart/kubeapps/templates/tiller-proxy-rbac.yaml

@@ -45,7 +45,7 @@ subjects:
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:
-  name: {{ template "kubeapps.tiller-proxy.fullname" . }}-ns-discovery
+  name: "kubeapps:controller:tiller-proxy-ns-discovery-{{ .Release.Namespace }}"
   labels:
     app: {{ template "kubeapps.tiller-proxy.fullname" . }}
     chart: {{ template "kubeapps.chart" . }}
@@ -62,7 +62,7 @@ rules:
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding
 metadata:
-  name: {{ template "kubeapps.tiller-proxy.fullname" . }}-ns-discovery
+  name: "kubeapps:controller:tiller-proxy-ns-discovery-{{ .Release.Namespace }}"
   labels:
     app: {{ template "kubeapps.tiller-proxy.fullname" . }}
     chart: {{ template "kubeapps.chart" . }}
@@ -71,7 +71,7 @@ metadata:
 roleRef:
   apiGroup: rbac.authorization.k8s.io
   kind: ClusterRole
-  name: {{ template "kubeapps.tiller-proxy.fullname" . }}-ns-discovery
+  name: "kubeapps:controller:tiller-proxy-ns-discovery-{{ .Release.Namespace }}"
 subjects:
   - kind: ServiceAccount
     name: {{ template "kubeapps.tiller-proxy.fullname" . }}

chart/kubeapps/values.yaml

@@ -13,7 +13,7 @@
 useHelm3: false
 
 ## Enable this feature flag to allow users to discover available namespaces (only the ones they have access).
-## If you set it to true, Kubeapps create a ClusterRole to be able to list namespaces.
+## If you set it to true, Kubeapps creates a ClusterRole to be able to list namespaces.
 allowNamespaceDiscovery: false
 
 ## The frontend service is the main reverse proxy used to access the Kubeapps UI

dashboard/src/actions/apps.test.tsx

@@ -256,13 +256,9 @@ describe("deploy chart", () => {
   });
   it("returns false and dispatches UnprocessableEntity if the given values don't satisfy the schema ", async () => {
     const res = await store.dispatch(
-      actions.apps.deployChart(
-        "my-version" as any,
-        "my-release",
-        definedNamespaces.default,
-        "foo: 1",
-        { properties: { foo: { type: "string" } } },
-      ),
+      actions.apps.deployChart("my-version" as any, "my-release", "default", "foo: 1", {
+        properties: { foo: { type: "string" } },
+      }),
     );
     expect(res).toBe(false);
     const expectedActions = [
@@ -323,13 +319,9 @@ describe("upgradeApp", () => {
 
   it("returns false and dispatches UnprocessableEntity if the given values don't satisfy the schema ", async () => {
     const res = await store.dispatch(
-      actions.apps.upgradeApp(
-        "my-version" as any,
-        "my-release",
-        definedNamespaces.default,
-        "foo: 1",
-        { properties: { foo: { type: "string" } } },
-      ),
+      actions.apps.upgradeApp("my-version" as any, "my-release", "default", "foo: 1", {
+        properties: { foo: { type: "string" } },
+      }),
     );
 
     expect(res).toBe(false);
@@ -347,7 +339,7 @@ describe("upgradeApp", () => {
 });
 
 describe("rollbackApp", () => {
-  const provisionCMD = actions.apps.rollbackApp("my-release", definedNamespaces.default, 1);
+  const provisionCMD = actions.apps.rollbackApp("my-release", "default", 1);
 
   it("success and re-request apps info", async () => {
     App.rollback = jest.fn().mockImplementationOnce(() => true);
@@ -360,7 +352,7 @@ describe("rollbackApp", () => {
       { type: getType(actions.apps.requestApps) },
     ];
     expect(store.getActions()).toEqual(expectedActions);
-    expect(App.rollback).toHaveBeenCalledWith("my-release", definedNamespaces.default, 1);
+    expect(App.rollback).toHaveBeenCalledWith("my-release", "default", 1);
   });
 
   it("dispatches an error", async () => {

dashboard/src/actions/auth.test.tsx

@@ -17,7 +17,7 @@ beforeEach(() => {
     authenticated: false,
     authenticating: false,
     oidcAuthenticated: false,
-    defaultNamespace: "default",
+    defaultNamespace: "_all",
   };
 
   Auth.validateToken = jest.fn();
@@ -66,7 +66,7 @@ describe("authenticate", () => {
         type: getType(actions.auth.authenticating),
       },
       {
-        payload: { authenticated: true, oidc: false, defaultNamespace: "default" },
+        payload: { authenticated: true, oidc: false, defaultNamespace: "_all" },
         type: getType(actions.auth.setAuthenticated),
       },
     ];
@@ -84,7 +84,7 @@ describe("authenticate", () => {
         type: getType(actions.auth.authenticating),
       },
       {
-        payload: { authenticated: true, oidc: true, defaultNamespace: "default" },
+        payload: { authenticated: true, oidc: true, defaultNamespace: "_all" },
         type: getType(actions.auth.setAuthenticated),
       },
       {
@@ -111,7 +111,7 @@ describe("OIDC authentication", () => {
         type: getType(actions.auth.authenticating),
       },
       {
-        payload: { authenticated: true, oidc: true, defaultNamespace: "default" },
+        payload: { authenticated: true, oidc: true, defaultNamespace: "_all" },
         type: getType(actions.auth.setAuthenticated),
       },
       {

dashboard/src/actions/namespace.test.tsx

@@ -56,7 +56,9 @@ actionTestCases.forEach(tc => {
 describe("fetchNamespaces", () => {
   it("dispatches the list of namespace names if no error", async () => {
     Namespace.list = jest.fn().mockImplementationOnce(() => {
-      return [{ metadata: { name: "overlook-hotel" } }, { metadata: { name: "room-217" } }];
+      return {
+        namespaces: [{ metadata: { name: "overlook-hotel" } }, { metadata: { name: "room-217" } }],
+      };
     });
     const expectedActions = [
       {
@@ -89,7 +91,9 @@ describe("createNamespace", () => {
   it("dispatches the new namespace and re-fetch namespaces", async () => {
     Namespace.create = jest.fn();
     Namespace.list = jest.fn().mockImplementationOnce(() => {
-      return [{ metadata: { name: "overlook-hotel" } }, { metadata: { name: "room-217" } }];
+      return {
+        namespaces: [{ metadata: { name: "overlook-hotel" } }, { metadata: { name: "room-217" } }],
+      };
     });
     const expectedActions = [
       {

dashboard/src/actions/namespace.ts

@@ -44,8 +44,8 @@ export type NamespaceAction = ActionType<typeof allActions[number]>;
 export function fetchNamespaces(): ThunkAction<Promise<void>, IStoreState, null, NamespaceAction> {
   return async dispatch => {
     try {
-      const namespaces = await Namespace.list();
-      const namespaceStrings = namespaces.map((n: IResource) => n.metadata.name);
+      const namespaceList = await Namespace.list();
+      const namespaceStrings = namespaceList.namespaces.map((n: IResource) => n.metadata.name);
       dispatch(receiveNamespaces(namespaceStrings));
     } catch (e) {
       dispatch(errorNamespaces(e, "list"));

dashboard/src/components/Header/NamespaceSelector.test.tsx

@@ -56,26 +56,6 @@ it("render with the default namespace selected if no current selection", () => {
   expect(select.props().value).toEqual(expectedValue);
 });
 
-it("renders the default namespace option if no namespaces provided", () => {
-  const props = {
-    ...defaultProps,
-    namespace: {
-      current: "",
-      namespaces: [],
-    },
-  };
-  const wrapper = shallow(<NamespaceSelector {...props} />);
-  const select = wrapper.find(".NamespaceSelector__select").first();
-
-  expect(select.props()).toMatchObject({
-    options: [
-      { label: "All Namespaces", value: "_all" },
-      { label: defaultProps.defaultNamespace, value: defaultProps.defaultNamespace },
-      { label: "Create New", value: "_new" },
-    ],
-  });
-});
-
 it("opens the modal to add a new namespace and creates it", async () => {
   const createNamespace = jest.fn(() => true);
   const wrapper = mount(<NamespaceSelector {...defaultProps} createNamespace={createNamespace} />);

dashboard/src/components/Header/NamespaceSelector.tsx

@@ -41,12 +41,8 @@ class NamespaceSelector extends React.Component<INamespaceSelectorProps, INamesp
   public render() {
     const {
       namespace: { namespaces, error },
-      defaultNamespace,
     } = this.props;
-    const options =
-      namespaces.length > 0
-        ? namespaces.map(n => ({ value: n, label: n }))
-        : [{ value: defaultNamespace, label: defaultNamespace }];
+    const options = namespaces.length > 0 ? namespaces.map(n => ({ value: n, label: n })) : [];
     const allOption = { value: definedNamespaces.all, label: "All Namespaces" };
     options.unshift(allOption);
     const newOption = { value: "_new", label: "Create New" };

dashboard/src/reducers/auth.test.ts

@@ -19,7 +19,7 @@ describe("authReducer", () => {
       authenticated: false,
       authenticating: false,
       oidcAuthenticated: false,
-      defaultNamespace: "default",
+      defaultNamespace: "_all",
     };
   });
 

dashboard/src/reducers/auth.ts

@@ -2,7 +2,8 @@ import { getType } from "typesafe-actions";
 
 import actions from "../actions";
 import { AuthAction } from "../actions/auth";
-import { Auth, DEFAULT_NAMESPACE } from "../shared/Auth";
+import { Auth } from "../shared/Auth";
+import { definedNamespaces } from "../shared/Namespace";
 
 export interface IAuthState {
   sessionExpired: boolean;
@@ -33,7 +34,7 @@ const authReducer = (state: IAuthState = initialState, action: AuthAction): IAut
         authenticated: action.payload.authenticated,
         oidcAuthenticated: action.payload.oidc,
         authenticating: false,
-        defaultNamespace: action.payload.defaultNamespace || DEFAULT_NAMESPACE,
+        defaultNamespace: action.payload.defaultNamespace || definedNamespaces.all,
       };
     case getType(actions.auth.authenticating):
       return { ...state, authenticated: false, authenticating: true };

dashboard/src/reducers/namespace.test.ts

@@ -72,7 +72,7 @@ describe("namespaceReducer", () => {
         namespaceReducer(dirtyState, {
           type: getType(actions.namespace.clearNamespaces),
         }),
-      ).toEqual({ current: "default", namespaces: [] });
+      ).toEqual({ current: "_all", namespaces: [] });
     });
   });
 

dashboard/src/shared/Auth.test.ts

@@ -1,4 +1,4 @@
-import Axios from "axios";
+import Axios, { AxiosResponse } from "axios";
 import * as jwt from "jsonwebtoken";
 import { Auth } from "./Auth";
 import { APIBase } from "./Kube";
@@ -130,7 +130,7 @@ describe("Auth", () => {
       expect(defaultNamespace).toEqual(customNamespace);
     });
 
-    it("should return default if the namespace is not present", () => {
+    it("should return _all if the namespace is not present", () => {
       const token = jwt.sign(
         {
           iss: "kubernetes/serviceaccount",
@@ -140,15 +140,15 @@ describe("Auth", () => {
 
       const defaultNamespace = Auth.defaultNamespaceFromToken(token);
 
-      expect(defaultNamespace).toEqual("default");
+      expect(defaultNamespace).toEqual("_all");
     });
 
     it("should return default if the token cannot be decoded", () => {
       const token = "not a jwt token";
 
       const defaultNamespace = Auth.defaultNamespaceFromToken(token);
 
-      expect(defaultNamespace).toEqual("default");
+      expect(defaultNamespace).toEqual("_all");
     });
   });
 
@@ -192,3 +192,15 @@ describe("Auth", () => {
     });
   });
 });
+
+describe("is403FromAuthProxy", () => {
+  it("does not assume to be authenticated from the auth proxy if the message contains a service account", () => {
+    expect(
+      Auth.is403FromAuthProxy({
+        status: 403,
+        data:
+          'namespaces is forbidden: User "system:serviceaccount:kubeapps:kubeapps-internal-kubeops" cannot list resource "namespaces" in API group "" at the cluster scope',
+      } as AxiosResponse<any>),
+    ).toBe(false);
+  });
+});

dashboard/src/shared/Auth.ts

@@ -4,8 +4,7 @@ const AuthTokenKey = "kubeapps_auth_token";
 const AuthTokenOIDCKey = "kubeapps_auth_token_oidc";
 import { IConfig } from "./Config";
 import { APIBase } from "./Kube";
-
-export const DEFAULT_NAMESPACE = "default";
+import { definedNamespaces } from "./Namespace";
 
 export class Auth {
   public static getAuthToken() {
@@ -86,6 +85,10 @@ export class Auth {
   // it in the one spot. We may need to query `/oauth2/info` to avoid potential
   // false positives.
   public static is403FromAuthProxy(r: AxiosResponse): boolean {
+    if (r.data && typeof r.data === "string" && r.data.match("system:serviceaccount")) {
+      // If the error message is related to a service account is not from the auth proxy
+      return false;
+    }
     return r.status === 403 && (!r.data || !r.data.message);
   }
 
@@ -133,7 +136,7 @@ export class Auth {
     const payload = jwt.decode(token);
     const namespaceKey = "kubernetes.io/serviceaccount/namespace";
     if (!payload || !payload[namespaceKey]) {
-      return DEFAULT_NAMESPACE;
+      return definedNamespaces.all;
     }
     return payload[namespaceKey];
   }

dashboard/src/shared/AxiosInstance.ts

@@ -58,7 +58,7 @@ export function addErrorHandling(axiosInstance: AxiosInstance, store: Store<ISto
           return Promise.reject(new UnauthorizedError(message));
         case 403:
           // A 403 directly from the auth proxy requires reauthentication.
-          if (Auth.is403FromAuthProxy(response)) {
+          if (Auth.usingOIDCToken() && Auth.is403FromAuthProxy(response)) {
             dispatchErrorAndLogout(message);
           }
           return Promise.reject(new ForbiddenError(message));

dashboard/src/shared/Namespace.ts

@@ -6,12 +6,14 @@ import { ForbiddenError, IResource, NotFoundError } from "./types";
 
 export default class Namespace {
   public static async list() {
-    const { data } = await axiosWithAuth.get<IResource[]>(url.backend.namespaces.list());
+    const { data } = await axiosWithAuth.get<{ namespaces: IResource[] }>(
+      url.backend.namespaces.list(),
+    );
     return data;
   }
 
   public static async create(name: string) {
-    const { data } = await axiosWithAuth.post<IResource>(Namespace.APIEndpoint, {
+    const { data } = await axiosWithAuth.post<IResource>(`${APIBase}/api/v1/namespaces/`, {
       apiVersion: "v1",
       kind: "Namespace",
       metadata: {
@@ -23,7 +25,7 @@ export default class Namespace {
 
   public static async get(name: string) {
     try {
-      const { data } = await axiosWithAuth.get<IResource>(`${Namespace.APIEndpoint}/${name}`);
+      const { data } = await axiosWithAuth.get<IResource>(`${APIBase}/api/v1/namespaces/${name}`);
       return data;
     } catch (err) {
       switch (err.constructor) {
@@ -38,13 +40,9 @@ export default class Namespace {
       }
     }
   }
-
-  private static APIBase: string = APIBase;
-  private static APIEndpoint: string = `${Namespace.APIBase}/api/v1/namespaces/`;
 }
 
 // Set of namespaces used accross the applications as default and "all ns" placeholders
 export const definedNamespaces = {
-  default: "default",
   all: "_all",
 };