Sonarfixes (#4)

* sonar fixes * sonar pr fix maybe
hoo29 · Jul 11, 2021 · d71409a · d71409a
1 parent 7b9fc67
commit d71409a
Show file tree

Hide file tree

Showing 8 changed files with 151 additions and 67 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -109,6 +109,11 @@ jobs:
             - name: update pom
               run: sed -i "s#\${env.KEYCLOAK_VERSION}#${KEYCLOAK_VERSION}#g" pom.xml
 
+            # not sure we can get sonar to update a PR due to disconnected workflow but we'll try!
+            - name: save PR head sha
+              if: ${{ github.event_name == 'pull_request' }}
+              run: echo "${{ github.event.pull_request.head.sha }}" >> git.sha
+
             - name: archive jar
               uses: actions/upload-artifact@v2
               with:
@@ -118,6 +123,7 @@ jobs:
                       src/
                       pom.xml
                       .git/
+                      git.sha
                   retention-days: 7
 
     publish:

diff --git a/.github/workflows/sonar.yml b/.github/workflows/sonar.yml
@@ -9,13 +9,11 @@ on:
 
 jobs:
     analyse:
-        environment: analysis
-        name: Build
-        runs-on: ubuntu-latest
-        if: ${{ github.event.workflow_run.conclusion == 'success' }}
         permissions:
             contents: read
             pull-requests: read
+        runs-on: ubuntu-latest
+        if: ${{ github.event.workflow_run.conclusion == 'success' }}
         steps:
             - name: setup jdk
               uses: actions/setup-java@v2
@@ -49,6 +47,11 @@ jobs:
                       const matchArtifact = artifacts.data.artifacts.filter((artifact) => {
                         return artifact.name == "dist"
                       })[0];
+
+                      if (!matchArtifact) {
+                        process.exit(0);
+                      }
+
                       const download = await github.actions.downloadArtifact({
                         owner: context.repo.owner,
                         repo: context.repo.repo,
@@ -58,14 +61,28 @@ jobs:
                       const fs = require('fs');
                       fs.writeFileSync('${{github.workspace}}/dist.zip', Buffer.from(download.data));
 
+            - name: check if analyse needed
+              id: check
+              run: |
+                  if [ -f "dist.zip" ]; then
+                      echo "::set-output name=check::true"
+                  else 
+                      echo "::set-output name=check::false"
+                  fi
+
             - name: extract artifact
+              if: ${{ steps.check.outputs.check == 'true' }}
               run: unzip dist.zip
 
-            - name: check dir listing
-              run: find -maxdepth 2 -type f -ls
-
             - name: Build and analyze
+              if: ${{ steps.check.outputs.check == 'true' }}
               env:
                   GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # Needed to get PR information, if any
                   SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
-              run: mvn -B org.sonarsource.scanner.maven:sonar-maven-plugin:sonar
+              run: |
+                  if [ -f "git.sha" ]; then
+                      mvn -B org.sonarsource.scanner.maven:sonar-maven-plugin:sonar  -Dsonar.scm.revision=$(cat git.sha)
+                  else 
+                      mvn -B org.sonarsource.scanner.maven:sonar-maven-plugin:sonar
+                  fi
+                  SHA=$(cat git.sha)
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 See notes about versioning in the README.
 
+## `x.x.x`-1.0.1
+
+-   sonarcloud bug fixes
+
 ## `x.x.x`-1.0.0
 
 -   initial release
diff --git a/README.md b/README.md
@@ -1,6 +1,7 @@
 # keycloak-client-authz
 
 [![build](https://github.com/hoo29/keycloak-client-authz/actions/workflows/build.yml/badge.svg)](https://github.com/hoo29/keycloak-client-authz/actions/workflows/build.yml)
+[![SonarCloud](https://sonarcloud.io/images/project_badges/sonarcloud-orange.svg)](https://sonarcloud.io/dashboard?id=hoo29_keycloak-client-authz)
 [![Open in Visual Studio Code](https://open.vscode.dev/badges/open-in-vscode.svg)](https://open.vscode.dev/hoo29/keycloak-client-authz)
 
 Keycloak authenticator plugin for client authorisation. Allows using membership of client roles to enforce authorisation during Keycloak login.

diff --git a/pom.xml b/pom.xml
@@ -3,14 +3,20 @@
 
     <groupId>spaces.huws.apps</groupId>
     <artifactId>keycloak-client-authz</artifactId>
-    <version>1.0.0</version>
+    <version>1.0.1</version>
 
     <properties>
         <keycloakVersion>${env.KEYCLOAK_VERSION}</keycloakVersion>
         <maven.compiler.source>11</maven.compiler.source>
         <maven.compiler.target>${maven.compiler.source}</maven.compiler.target>
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
         <project.reporting.outputEncoding>${project.build.sourceEncoding}</project.reporting.outputEncoding>
+        <sonar.projectKey>hoo29_keycloak-client-authz</sonar.projectKey>
+        <sonar.organization>hoo29</sonar.organization>
+        <sonar.host.url>https://sonarcloud.io</sonar.host.url>
+        <sonar.java.coveragePlugin>jacoco</sonar.java.coveragePlugin>
+        <sonar.dynamicAnalysis>reuseReports</sonar.dynamicAnalysis>
+        <jacocoVersion>0.8.7</jacocoVersion>
     </properties>
 
     <name>keycloak-client-authz</name>
@@ -54,6 +60,12 @@
             <version>3.11.2</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.jacoco</groupId>
+            <artifactId>jacoco-maven-plugin</artifactId>
+            <version>${jacocoVersion}</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>
@@ -75,6 +87,26 @@
                     <skip>false</skip>
                 </configuration>
             </plugin>
+            <plugin>
+                <groupId>org.jacoco</groupId>
+                <artifactId>jacoco-maven-plugin</artifactId>
+                <version>${jacocoVersion}</version>
+                <executions>
+                    <execution>
+                        <id>prepare-agent</id>
+                        <goals>
+                            <goal>prepare-agent</goal>
+                        </goals>
+                    </execution>
+                    <execution>
+                        <id>report</id>
+                        <phase>test</phase>
+                        <goals>
+                            <goal>report</goal>
+                        </goals>
+                    </execution>
+                </executions>
+            </plugin>
         </plugins>
     </build>
 

diff --git a/src/main/java/space/huws/apps/ClientRoleAuthenticator.java b/src/main/java/space/huws/apps/ClientRoleAuthenticator.java
@@ -23,23 +23,16 @@ public class ClientRoleAuthenticator implements Authenticator {
 
     @Override
     public void close() {
+        // NOP
     }
 
     @Override
     public void authenticate(AuthenticationFlowContext context) {
-        final ClientModel client = context.getAuthenticationSession().getClient();
-        final UserModel user = context.getUser();
-
-        // keycloak should throw an error before this code is invoked as requiresUser() is true
-        if (user == null) {
-            throw new ClientRoleAuthenticatorException("user was null during flow");
-        }
 
-        final String clientRoleName = safeGetConfigValue(CONFIG_ROLE_NAME_NAME, context);
-        final boolean failOpen = safeGetConfigValue(CONFIG_FAIL_OPEN_NAME, context) == "true";
+        // authentication has already occurred with a previous provider, authorise here
+        final boolean authorised = authorise(context);
 
-        final RoleModel role = client.getRole(clientRoleName);
-        if ((role == null && failOpen) || user.hasRole(role)) {
+        if (authorised) {
             context.success();
         } else {
             /**
@@ -58,6 +51,7 @@ public void authenticate(AuthenticationFlowContext context) {
             final LoginFormsProvider forms = context.form();
             forms.setError(Messages.ACCESS_DENIED);
             final Response errorResponse = forms.createErrorPage(Response.Status.FORBIDDEN);
+
             context.failure(AuthenticationFlowError.ACCESS_DENIED, errorResponse);
         }
     }
@@ -79,6 +73,23 @@ public boolean configuredFor(KeycloakSession session, RealmModel realm, UserMode
 
     @Override
     public void setRequiredActions(KeycloakSession session, RealmModel realm, UserModel user) {
+        // NOP
+    }
+
+    private boolean authorise(AuthenticationFlowContext context) {
+        final ClientModel client = context.getAuthenticationSession().getClient();
+        final UserModel user = context.getUser();
+
+        // keycloak should throw an error before this code is invoked as requiresUser() is true
+        if (user == null) {
+            throw new ClientRoleAuthenticatorException("user was null during flow");
+        }
+
+        final String clientRoleName = safeGetConfigValue(CONFIG_ROLE_NAME_NAME, context);
+        final boolean failOpen = safeGetConfigValue(CONFIG_FAIL_OPEN_NAME, context).equals("true");
+
+        final RoleModel role = client.getRole(clientRoleName);
+        return (role == null && failOpen) || user.hasRole(role);
     }
 
     private String safeGetConfigValue(String key, AuthenticationFlowContext context) {
@@ -90,7 +101,7 @@ private String safeGetConfigValue(String key, AuthenticationFlowContext context)
         if (confModel == null) {
             // given there are so few elements and no class constructor to nicely convert it to a map this seems fine...
             for (final ProviderConfigProperty prop : ClientRoleAuthenticatorFactory.CONFIG_PROPERTIES) {
-                if (prop.getName() == key) {
+                if (prop.getName().equals(key)) {
                     value = prop.getDefaultValue() instanceof String ? (String) prop.getDefaultValue() : null;
                     break;
                 }

diff --git a/src/main/java/space/huws/apps/ClientRoleAuthenticatorFactory.java b/src/main/java/space/huws/apps/ClientRoleAuthenticatorFactory.java
@@ -17,7 +17,8 @@ public class ClientRoleAuthenticatorFactory implements AuthenticatorFactory {
     // Using static blocks and not constructors to match the rest of the keycloak codebase.
 
     public static final String PROVIDER_ID = "auth-client-role";
-    public static final List<ProviderConfigProperty> CONFIG_PROPERTIES = new ArrayList<>();
+
+    protected static final List<ProviderConfigProperty> CONFIG_PROPERTIES = new ArrayList<>();
 
     private static final Authenticator SINGLETON = new ClientRoleAuthenticator();
     private static final AuthenticationExecutionModel.Requirement[] REQUIREMENTS = {
@@ -50,15 +51,17 @@ public Authenticator create(KeycloakSession session) {
 
     @Override
     public void init(Scope config) {
+        // NOP
     }
 
     @Override
     public void postInit(KeycloakSessionFactory factory) {
+        // NOP
     }
 
     @Override
     public void close() {
-
+        // NOP
     }
 
     @Override

diff --git a/src/test/java/space/huws/apps/ClientRoleAuthenticatorTest.java b/src/test/java/space/huws/apps/ClientRoleAuthenticatorTest.java
@@ -9,6 +9,7 @@
 import org.apache.log4j.Logger;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.keycloak.authentication.AuthenticationFlowContext;
@@ -49,64 +50,73 @@ public void before(@Mock AuthenticationSessionModel asm) {
         when(asm.getClient()).thenReturn(client);
     }
 
-    public void mockErrorPath() {
-        // if we do this in before method mockito complains at me for writing bad code :(
-        EventBuilder eb = mock(EventBuilder.class);
-        LoginFormsProvider lfp = mock(LoginFormsProvider.class);
+    // separate failure and success tests so we only mock when necessary
 
-        when(context.getEvent()).thenReturn(eb);
-        when(context.form()).thenReturn(lfp);
-    }
+    @Nested
+    class ClientRoleAuthenticatorSuccessTests {
+        @Test
+        void defaultConfigNoClientRole() {
+            when(client.getRole(anyString())).thenReturn(null);
+
+            ClientRoleAuthenticator cra = new ClientRoleAuthenticator();
+            cra.authenticate(context);
+
+            verify(context, times(1)).success();
+        }
+
+        @Test
+        void defaultConfigMemberClientRole(@Mock RoleModel role) {
+            when(client.getRole(anyString())).thenReturn(role);
+            when(user.hasRole(any(RoleModel.class))).thenReturn(true);
 
-    @Test
-    public void defaultConfigNoClientRole() {
-        when(client.getRole(anyString())).thenReturn(null);
+            ClientRoleAuthenticator cra = new ClientRoleAuthenticator();
+            cra.authenticate(context);
 
-        ClientRoleAuthenticator cra = new ClientRoleAuthenticator();
-        cra.authenticate(context);
+            verify(context, times(1)).success();
+        }
 
-        verify(context, times(1)).success();
     }
 
-    @Test
-    public void failSafeNoClientRole() {
-        Map<String, String> config = new HashMap<>();
-        config.put(ClientRoleAuthenticator.CONFIG_FAIL_OPEN_NAME, "false");
-        config.put(ClientRoleAuthenticator.CONFIG_ROLE_NAME_NAME, "access");
-        AuthenticatorConfigModel confModel = mock(AuthenticatorConfigModel.class);
-        when(context.getAuthenticatorConfig()).thenReturn(confModel);
-        when(confModel.getConfig()).thenReturn(config);
+    @Nested
+    class ClientRoleAuthenticatorFailureTests {
 
-        when(client.getRole(anyString())).thenReturn(null);
-        mockErrorPath();
+        @BeforeEach
+        public void mockErrorPath() {
+            EventBuilder eb = mock(EventBuilder.class);
+            LoginFormsProvider lfp = mock(LoginFormsProvider.class);
 
-        ClientRoleAuthenticator cra = new ClientRoleAuthenticator();
-        cra.authenticate(context);
+            when(context.getEvent()).thenReturn(eb);
+            when(context.form()).thenReturn(lfp);
+        }
 
-        verify(context, times(1)).failure(any(), any());
-    }
+        @Test
+        void failSafeNoClientRole() {
+            Map<String, String> config = new HashMap<>();
+            config.put(ClientRoleAuthenticator.CONFIG_FAIL_OPEN_NAME, "false");
+            config.put(ClientRoleAuthenticator.CONFIG_ROLE_NAME_NAME, "access");
+            AuthenticatorConfigModel confModel = mock(AuthenticatorConfigModel.class);
+            when(context.getAuthenticatorConfig()).thenReturn(confModel);
+            when(confModel.getConfig()).thenReturn(config);
 
-    @Test
-    public void defaultConfigMemberClientRole(@Mock RoleModel role) {
-        when(client.getRole(anyString())).thenReturn(role);
-        when(user.hasRole(any(RoleModel.class))).thenReturn(true);
+            when(client.getRole(anyString())).thenReturn(null);
+            mockErrorPath();
 
-        ClientRoleAuthenticator cra = new ClientRoleAuthenticator();
-        cra.authenticate(context);
+            ClientRoleAuthenticator cra = new ClientRoleAuthenticator();
+            cra.authenticate(context);
 
-        verify(context, times(1)).success();
-    }
+            verify(context, times(1)).failure(any(), any());
+        }
 
-    @Test
-    public void defaultConfigNotMemberClientRole(@Mock RoleModel role) {
-        when(client.getRole(anyString())).thenReturn(role);
-        when(user.hasRole(any(RoleModel.class))).thenReturn(false);
-        mockErrorPath();
+        @Test
+        void defaultConfigNotMemberClientRole(@Mock RoleModel role) {
+            when(client.getRole(anyString())).thenReturn(role);
+            when(user.hasRole(any(RoleModel.class))).thenReturn(false);
+            mockErrorPath();
 
-        ClientRoleAuthenticator cra = new ClientRoleAuthenticator();
-        cra.authenticate(context);
+            ClientRoleAuthenticator cra = new ClientRoleAuthenticator();
+            cra.authenticate(context);
 
-        verify(context, times(1)).failure(any(), any());
+            verify(context, times(1)).failure(any(), any());
+        }
     }
-
 }