From 8dfe4061f29ec3c5f12c0b582330a3dc547d692b Mon Sep 17 00:00:00 2001
From: Brian Saville <bksaville@gmail.com>
Date: Tue, 14 Nov 2023 15:36:57 -0700
Subject: [PATCH] Raise exception when access is denied instead of silently
 failing (#257)

Co-authored-by: saville <saville@adobe.com>
---
 .../datapipe/jenkins/vault/VaultAccessor.java |  4 +-
 .../jenkins/vault/VaultBuildWrapperTest.java  | 56 ++++++++++++++-----
 2 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/src/main/java/com/datapipe/jenkins/vault/VaultAccessor.java b/src/main/java/com/datapipe/jenkins/vault/VaultAccessor.java
index ada4c5bc..3fa216e3 100644
--- a/src/main/java/com/datapipe/jenkins/vault/VaultAccessor.java
+++ b/src/main/java/com/datapipe/jenkins/vault/VaultAccessor.java
@@ -227,8 +227,8 @@ public static boolean responseHasErrors(VaultConfiguration configuration, PrintS
         }
         int status = restResponse.getStatus();
         if (status == 403) {
-            logger.printf("Access denied to Vault Secrets at '%s'%n", path);
-            return true;
+            throw new VaultPluginException(
+                String.format("Access denied to Vault path '%s'", path));
         } else if (status == 404) {
             if (configuration.getFailIfNotFound()) {
                 throw new VaultPluginException(
diff --git a/src/test/java/com/datapipe/jenkins/vault/VaultBuildWrapperTest.java b/src/test/java/com/datapipe/jenkins/vault/VaultBuildWrapperTest.java
index b8b714ac..f0c5af18 100644
--- a/src/test/java/com/datapipe/jenkins/vault/VaultBuildWrapperTest.java
+++ b/src/test/java/com/datapipe/jenkins/vault/VaultBuildWrapperTest.java
@@ -38,7 +38,11 @@ public class VaultBuildWrapperTest {
     @Test
     public void testWithNonExistingPath() throws IOException, InterruptedException {
         String path = "not/existing";
-        TestWrapper wrapper = new TestWrapper(standardSecrets(path));
+        VaultAccessor mockAccessor = mock(VaultAccessor.class);
+        doReturn(mockAccessor).when(mockAccessor).init();
+        LogicalResponse response = getNotFoundResponse();
+        when(mockAccessor.read(path, 2)).thenReturn(response);
+        TestWrapper wrapper = new TestWrapper(standardSecrets(path), mockAccessor);
         final ByteArrayOutputStream baos = new ByteArrayOutputStream();
         PrintStream logger = new PrintStream(baos);
         SimpleBuildWrapper.Context context = null;
@@ -56,11 +60,38 @@ public void testWithNonExistingPath() throws IOException, InterruptedException {
             assertThat(e.getMessage(), is("Vault credentials not found for 'not/existing'"));
         }
 
-        wrapper.verifyCalls();
+        verify(mockAccessor, times(2)).init();
+        verify(mockAccessor, times(2)).read(path, 2);
         assertThat(new String(baos.toByteArray(), StandardCharsets.UTF_8),
             containsString("Vault credentials not found for 'not/existing'"));
     }
 
+    @Test
+    public void testWithAccessDeniedPath() throws IOException, InterruptedException {
+        String path = "not/allowed";
+        VaultAccessor mockAccessor = mock(VaultAccessor.class);
+        doReturn(mockAccessor).when(mockAccessor).init();
+        LogicalResponse response = getAccessDeniedResponse();
+        when(mockAccessor.read(path, 2)).thenReturn(response);
+        TestWrapper wrapper = new TestWrapper(standardSecrets(path), mockAccessor);
+        final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        PrintStream logger = new PrintStream(baos);
+        SimpleBuildWrapper.Context context = null;
+        Run<?, ?> build = mock(Build.class);
+        when(build.getParent()).thenReturn(null);
+        EnvVars envVars = mock(EnvVars.class);
+        when(envVars.expand(path)).thenReturn(path);
+
+        try {
+            wrapper.run(context, build, envVars, logger);
+        } catch (VaultPluginException e) {
+            assertThat(e.getMessage(), is("Access denied to Vault path 'not/allowed'"));
+        }
+
+        verify(mockAccessor).init();
+        verify(mockAccessor).read(path, 2);
+    }
+
     private List<VaultSecret> standardSecrets(String path) {
         List<VaultSecret> secrets = new ArrayList<>();
         VaultSecretValue secretValue = new VaultSecretValue("envVar1", "key1");
@@ -81,21 +112,25 @@ private LogicalResponse getNotFoundResponse() {
         return resp;
     }
 
+    private LogicalResponse getAccessDeniedResponse() {
+        LogicalResponse resp = mock(LogicalResponse.class);
+        RestResponse rest = mock(RestResponse.class);
+        when(resp.getData()).thenReturn(new HashMap<>());
+        when(resp.getRestResponse()).thenReturn(rest);
+        when(rest.getStatus()).thenReturn(403);
+        return resp;
+    }
+
     class TestWrapper extends VaultBuildWrapper {
 
-        VaultAccessor mockAccessor;
         VaultConfiguration vaultConfig = new VaultConfiguration();
 
-        public TestWrapper(List<VaultSecret> vaultSecrets) {
+        public TestWrapper(List<VaultSecret> vaultSecrets, VaultAccessor mockAccessor) {
             super(vaultSecrets);
 
             vaultConfig.setVaultUrl("testmock");
             vaultConfig.setVaultCredentialId("credId");
             vaultConfig.setFailIfNotFound(false);
-            mockAccessor = mock(VaultAccessor.class);
-            doReturn(mockAccessor).when(mockAccessor).init();
-            LogicalResponse response = getNotFoundResponse();
-            when(mockAccessor.read("not/existing", 2)).thenReturn(response);
             setVaultAccessor(mockAccessor);
             setConfiguration(vaultConfig);
         }
@@ -104,10 +139,5 @@ public void run(Context context, Run build, EnvVars envVars, PrintStream logger)
             this.logger = logger;
             provideEnvironmentVariablesFromVault(context, build, envVars);
         }
-
-        public void verifyCalls() {
-            verify(mockAccessor, times(2)).init();
-            verify(mockAccessor, times(2)).read("not/existing", 2);
-        }
     }
 }