Merge pull request #1151 from GnsP/scm-do-not-expose-pat-in-ui

[CDAP-20904] Update the UI to not expose the Github PAT in the Namespace Admin page
cdapio · Nov 22, 2023 · 4b80093 · 4b80093
2 parents 218c4cf + 6929e0f
commit 4b80093
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 40 deletions.
diff --git a/app/cdap/components/NamespaceAdmin/SourceControlManagement/SourceControlManagementForm.tsx b/app/cdap/components/NamespaceAdmin/SourceControlManagement/SourceControlManagementForm.tsx
@@ -78,11 +78,13 @@ const StyledHr = styled.hr`
 interface ISourceControlManagementFormProps {
   initialSourceControlManagementConfig?: ISourceControlManagementConfig | null;
   onToggle: () => void;
+  isEdit: boolean;
 }
 
 const SourceControlManagementForm = ({
   initialSourceControlManagementConfig,
   onToggle,
+  isEdit,
 }: ISourceControlManagementFormProps) => {
   const [formState, formStateDispatch] = useReducer(
     sourceControlManagementFormReducer,
@@ -94,7 +96,7 @@ const SourceControlManagementForm = ({
     if (!formState.config?.link) {
       count++;
     }
-    if (!formState.config?.auth?.token) {
+    if (!isEdit && !formState.config?.auth?.token) {
       count++;
     }
     if (!formState.config?.auth?.patConfig?.passwordName) {
@@ -326,7 +328,7 @@ const SourceControlManagementForm = ({
                   name: 'token',
                   description: T.translate(`${PREFIX}.auth.pat.tokenHelperText`).toString(),
                   label: T.translate(`${PREFIX}.auth.pat.token`).toString(),
-                  required: true,
+                  required: !isEdit,
                   'widget-type': 'password',
                 }}
                 onChange={(val) => {

diff --git a/app/cdap/components/NamespaceAdmin/SourceControlManagement/index.tsx b/app/cdap/components/NamespaceAdmin/SourceControlManagement/index.tsx
@@ -48,6 +48,7 @@ const StyledInfo = styled.div`
 
 export const SourceControlManagement = () => {
   const [isFormOpen, setIsFormOpen] = useState(false);
+  const [isEditingConfig, setIsEditingConfig] = useState(false);
   const [isUnlinkModalOpen, setIsUnlinkModalOpen] = useState(false);
   const [errorMessage, setErrorMessage] = useState(null);
   const [loading, setLoading] = useState(false);
@@ -57,14 +58,25 @@ export const SourceControlManagement = () => {
   const toggleForm = () => {
     setIsFormOpen(!isFormOpen);
   };
+
+  const openCreateForm = () => {
+    setIsEditingConfig(false);
+    setIsFormOpen(true);
+  };
+
+  const openEditForm = () => {
+    setIsEditingConfig(true);
+    setIsFormOpen(true);
+  };
+
   const toggleUnlinkModal = () => {
     setIsUnlinkModalOpen(!isUnlinkModalOpen);
   };
 
   const actions = [
     {
       label: T.translate('commons.edit'),
-      actionFn: () => toggleForm(),
+      actionFn: () => openEditForm(),
     },
     {
       label: T.translate('commons.delete'),
@@ -100,21 +112,20 @@ export const SourceControlManagement = () => {
       <StyledInfo>{T.translate(`${PREFIX}.info`)}</StyledInfo>
       {!sourceControlManagementConfig && (
         <PrimaryContainedButton
-          onClick={toggleForm}
+          onClick={openCreateForm}
           style={{ marginBottom: '15px' }}
           data-testid="link-repository-button"
         >
           {T.translate(`${PREFIX}.linkButton`)}
         </PrimaryContainedButton>
       )}
       {sourceControlManagementConfig && (
-        <Table columnTemplate="100px 2fr 1fr 2fr 1fr 2fr 120px 100px">
+        <Table columnTemplate="100px 2fr 1fr 1fr 2fr 120px 100px">
           <TableHeader>
             <TableRow>
               <TableCell>{T.translate(`${PREFIX}.configModal.provider.label`)}</TableCell>
               <TableCell>{T.translate(`${PREFIX}.configModal.repoUrl.label`)}</TableCell>
               <TableCell>{T.translate(`${PREFIX}.configModal.auth.label`)}</TableCell>
-              <TableCell>{T.translate(`${PREFIX}.configModal.auth.pat.token`)}</TableCell>
               <TableCell>{T.translate(`${PREFIX}.configModal.branch.label`)}</TableCell>
               <TableCell>{T.translate(`${PREFIX}.configModal.pathPrefix.label`)}</TableCell>
               <TableCell></TableCell>
@@ -137,9 +148,6 @@ export const SourceControlManagement = () => {
               <TableCell data-testid="repository-auth-type">
                 {sourceControlManagementConfig.auth.type}
               </TableCell>
-              <TableCell data-testid="repository-auth-token">
-                <StyledPasswordWrapper value={sourceControlManagementConfig.auth.token} />
-              </TableCell>
               <TableCell>
                 {sourceControlManagementConfig.defaultBranch
                   ? sourceControlManagementConfig.defaultBranch
@@ -166,6 +174,7 @@ export const SourceControlManagement = () => {
         <SourceControlManagementForm
           onToggle={toggleForm}
           initialSourceControlManagementConfig={sourceControlManagementConfig}
+          isEdit={isEditingConfig}
         />
       )}
       <UnlinkSourceControlModal

diff --git a/app/cdap/components/NamespaceAdmin/store/ActionCreator.ts b/app/cdap/components/NamespaceAdmin/store/ActionCreator.ts
@@ -201,18 +201,6 @@ export function deleteConnection(conn: IConnection) {
 export const getSourceControlManagement = (namespace) => {
   const params = { namespace };
   return MyNamespaceApi.getSourceControlManagement(params).pipe(
-    switchMap((res: any) => {
-      const config = res.config;
-      return Observable.forkJoin(
-        of(config),
-        MySecureKeyApi.getSecureData({ ...params, key: config.auth.patConfig.passwordName }).pipe(
-          // return a null token if token is not found
-          catchError(() => {
-            return of(...[config, null]);
-          })
-        )
-      );
-    }),
     // config does not exist
     catchError((err) => {
       Store.dispatch({
@@ -227,10 +215,7 @@ export const getSourceControlManagement = (namespace) => {
 };
 
 export const getAndSetSourceControlManagement = (namespace) => {
-  getSourceControlManagement(namespace).subscribe((res) => {
-    // after getting the saved config, we need to fetch the token using the passwordName
-    const [config, token] = res;
-    config.auth.token = token;
+  getSourceControlManagement(namespace).subscribe(({ config }) => {
     Store.dispatch({
       type: NamespaceAdminActions.setSourceControlManagementConfig,
       payload: {
@@ -252,12 +237,16 @@ export const addOrValidateSourceControlManagementForm = (
     // validate the connection
     // TODO: currently it requires to save the token to secure store first.
     // In the future we might want to test connection on the fly
-    return MySecureKeyApi.put(
-      { ...params, key: formState.auth.patConfig.passwordName },
-      {
-        data: formState.auth.token,
-      }
-    ).pipe(
+    const secureKeyObservable = formState.auth.token
+      ? MySecureKeyApi.put(
+          { ...params, key: formState.auth.patConfig.passwordName },
+          {
+            data: formState.auth.token,
+          }
+        )
+      : of(null);
+
+    return secureKeyObservable.pipe(
       switchMap(() => {
         return MyNamespaceApi.setSourceControlManagement(
           params,
@@ -271,12 +260,14 @@ export const addOrValidateSourceControlManagementForm = (
   }
   return Observable.forkJoin(
     MyNamespaceApi.setSourceControlManagement(params, getBodyForSubmit(formState)),
-    MySecureKeyApi.put(
-      { ...params, key: formState.auth.patConfig.passwordName },
-      {
-        data: formState.auth.token,
-      }
-    )
+    formState.auth.token
+      ? MySecureKeyApi.put(
+          { ...params, key: formState.auth.patConfig.passwordName },
+          {
+            data: formState.auth.token,
+          }
+        )
+      : Observable.of(null)
   ).map(() => getAndSetSourceControlManagement(namespace));
 };
 

diff --git a/src/e2e-test/java/io/cdap/cdap/ui/stepsdesign/SourceControlManagement.java b/src/e2e-test/java/io/cdap/cdap/ui/stepsdesign/SourceControlManagement.java
@@ -93,9 +93,6 @@ public void verifySavedRepoConfig() {
     Assert.assertTrue(Helper.locateElementByTestId("repository-provider").getText().contains("GITHUB"));
     Assert.assertTrue(Helper.locateElementByTestId("repository-link").getText().contains(Constants.FAKE_REPO_LINK));
     Assert.assertTrue(Helper.locateElementByTestId("repository-auth-type").getText().contains("PAT"));
-    Assert.assertTrue(Helper.locateElementByTestId("repository-auth-token")
-        .findElement(By.cssSelector("input")).getAttribute("value")
-        .contains(Constants.FAKE_TOKEN));
   }
 
   @Then("Delete the repo config")