Fix null pointer crash with some Authentication implementations (#10)

* javadoc: cross reference Spring and opa-java docs
* fix null pointer crash when building opa input

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## v0.0.6 (unreleased)
 
+* Fixed a null pointer exception while constructing the input to OPA with some Authentication implementations.
+
 ## v0.0.5
 
 * Add `OPAAuthorizationManager` constructor that accepts a path and a `ContextDataProvider`, but not an `OPAClient`.

build.gradle

@@ -27,6 +27,13 @@ repositories {
     mavenCentral()
 }
 
+javadoc {
+    options.links += [
+        "https://styrainc.github.io/opa-java/javadoc/",
+        "https://docs.spring.io/spring-security/site/docs/current/api/",
+    ]
+}
+
 dependencies {
     implementation 'org.springframework.boot:spring-boot-starter-web'
     implementation 'org.springframework.security:spring-security-web'

src/main/java/com/styra/opa/springboot/OPAAuthorizationManager.java

@@ -175,22 +175,39 @@ public void setReasonKey(String newReasonKey) {
         this.reasonKey = newReasonKey;
     }
 
+    /**
+     * If v is null, this function is a NO-OP, otherwise, it calls m.put(k, v).
+     *
+     * @return
+     */
+    private void nullablePut(Map<String, Object> m, String k, Object v) {
+        if (v == null) {
+            return;
+        }
+
+        m.put(k, v);
+    }
+
     private Map<String, Object> makeRequestInput(
         Supplier<Authentication> authentication,
         RequestAuthorizationContext object
     ) {
-        Object subjectId = authentication.get().getPrincipal();
-        Object subjectDetails = authentication.get().getDetails();
-        Collection<? extends GrantedAuthority> subjectAuthorities =
-            authentication.get().getAuthorities();
+        Object subjectId = null;
+        Object subjectDetails = null;
+        Collection<? extends GrantedAuthority> subjectAuthorities = null;
+        if (authentication.get() != null) {
+            subjectId = authentication.get().getPrincipal();
+            subjectDetails = authentication.get().getDetails();
+            subjectAuthorities = authentication.get().getAuthorities();
+        }
 
         HttpServletRequest request = object.getRequest();
         String resourceId = request.getServletPath();
 
         String actionName = request.getMethod();
         String actionProtocol = request.getProtocol();
         Enumeration<String> headerNamesEnumeration = request.getHeaderNames();
-        HashMap<String, String> headers = new HashMap<String, String>();
+        Map<String, String> headers = new HashMap<String, String>();
         while (headerNamesEnumeration.hasMoreElements()) {
             String headerName = headerNamesEnumeration.nextElement();
             String headerValue = request.getHeader(headerName);
@@ -204,27 +221,25 @@ private Map<String, Object> makeRequestInput(
         String contextRemoteHost = request.getRemoteHost();
         Integer contextRemotePort = request.getRemotePort();
 
-        HashMap<String, Object> ctx = new HashMap<String, Object>();
-        ctx.put("type", RequestContextType);
-        ctx.put("host", contextRemoteHost);
-        ctx.put("ip", contextRemoteAddr);
-        ctx.put("port", contextRemotePort);
+        Map<String, Object> ctx = new HashMap<String, Object>();
+        nullablePut(ctx, "type", RequestContextType);
+        nullablePut(ctx, "host", contextRemoteHost);
+        nullablePut(ctx, "ip", contextRemoteAddr);
+        nullablePut(ctx, "port", contextRemotePort);
+
+        Map<String, Object> subjectInfo = new HashMap<String, Object>();
+        nullablePut(subjectInfo, "type", SubjectType);
+        nullablePut(subjectInfo, "id", subjectId);
+        nullablePut(subjectInfo, "details", subjectDetails);
+        nullablePut(subjectInfo, "authorities", subjectAuthorities);
 
         if (this.ctxProvider != null) {
             Object contextData = this.ctxProvider.getContextData(authentication, object);
             ctx.put("data", contextData);
         }
 
         java.util.Map<String, Object> iMap = java.util.Map.ofEntries(
-            entry(
-                "subject",
-                java.util.Map.ofEntries(
-                    entry("type", SubjectType),
-                    entry("id", subjectId),
-                    entry("details", subjectDetails),
-                    entry("authorities", subjectAuthorities)
-                )
-            ),
+            entry("subject", subjectInfo),
             entry(
                 "resource",
                 java.util.Map.ofEntries(

src/test/java/com/styra/opa/springboot/OPAAuthorizationManagerTest.java

@@ -114,6 +114,30 @@ private Authentication createMockAuthentication() {
         return mockAuth;
     }
 
+    /**
+     * This is used to create a mock auth object where most of the fields are
+     * null, to suss out exceptions when optional fields are omitted.
+     *
+     * @return
+     */
+    private Authentication createNullMockAuthentication() {
+        Authentication mockAuth = mock(Authentication.class);
+        when(mockAuth.getPrincipal()).thenReturn("testuser");
+        when(mockAuth.getCredentials()).thenReturn(null);
+
+        WebAuthenticationDetails details = new WebAuthenticationDetails(httpServletRequest);
+        when(mockAuth.getDetails()).thenReturn(null);
+
+        GrantedAuthority authority1 = new SimpleGrantedAuthority("ROLE_USER");
+        GrantedAuthority authority2 = new SimpleGrantedAuthority("ROLE_ADMIN");
+        Collection<? extends GrantedAuthority> authorities = Arrays.asList(authority1, authority2);
+        doReturn(null).when(mockAuth).getAuthorities();
+
+        when(mockAuth.isAuthenticated()).thenReturn(true);
+
+        return mockAuth;
+    }
+
     // Convert the value to JSON and then retrieve the value at the specified
     // path.
     //
@@ -403,4 +427,78 @@ public void testOPAAuthorizationManagerEcho() {
 
     }
 
+    @Test
+    public void testOPAAuthorizationManagerNullMetadata() {
+        // By reading back the input, we can make sure the OPA input has the
+        // right structure and content.
+
+        Map<String, Object> expectData = Map.ofEntries(
+                entry("action", Map.ofEntries(
+                    entry("headers", Map.ofEntries(
+                        entry("UnitTestHeader", "123abc")
+                    )),
+                    entry("name", "GET"),
+                    entry("protocol", "HTTP/1.1")
+                )),
+                entry("context", Map.ofEntries(
+                    entry("host", "example.com"),
+                    entry("ip", "192.0.2.123"),
+                    entry("port", 0),
+                    entry("type", "http"),
+                    entry("data", Map.ofEntries(
+                        entry("hello", "world")
+                    ))
+                )),
+                entry("resource", Map.ofEntries(
+                    entry("id", "unit/test"),
+                    entry("type", "endpoint")
+                )),
+                entry("subject", Map.ofEntries(
+                    entry("id", "testuser"),
+                    entry("type", "java_authentication")
+                ))
+        );
+
+        OPAResponseContext expectCtx = new OPAResponseContext();
+        expectCtx.setReasonUser(Map.ofEntries(
+            entry("en", "echo rule always allows"),
+            entry("other", "other reason key")
+        ));
+        expectCtx.setId("0");
+        expectCtx.setData(expectData);
+
+        OPAResponse expect = new OPAResponse();
+        expect.setDecision(true);
+        expect.setContext(expectCtx);
+
+        ContextDataProvider prov = new ConstantContextDataProvider(java.util.Map.ofEntries(
+            entry("hello", "world")
+        ));
+        Authentication mockAuth = this.createNullMockAuthentication();
+        when(authenticationSupplier.get()).thenReturn(mockAuth);
+        OPAClient opa = new OPAClient(address, headers);
+        OPAAuthorizationManager am = new OPAAuthorizationManager(opa, "policy/echo", prov);
+        OPAResponse actual = am.opaRequest(this.authenticationSupplier, this.context);
+
+        assertEquals(expect.getDecision(), actual.getDecision());
+        assertEquals(expect.getContext().getId(), actual.getContext().getId());
+        assertEquals(expect.getContext().getReasonUser(), actual.getContext().getReasonUser());
+
+        List<String> datadiff = jsonDiff(expect.getContext().getData(), actual.getContext().getData());
+
+        System.out.printf("#### expected context data\n%s\n", jsonPretty(expectData));
+        System.out.printf("#### actual context data\n%s\n", jsonPretty(actual.getContext().getData()));
+
+        for (int i = 0; i < datadiff.size(); i++) {
+            System.out.printf("diff mismatch: %s\n", datadiff.get(i));
+        }
+
+        assertEquals(0, datadiff.size());
+
+        assertEquals("echo rule always allows", actual.getReasonForDecision("en"));
+        assertEquals("other reason key", actual.getReasonForDecision("other"));
+        assertEquals("echo rule always allows", actual.getReasonForDecision("nonexistant"));
+
+    }
+
 }