From 2f5fa959c41eaeadc59164cb84b8de1e564e4fb8 Mon Sep 17 00:00:00 2001
From: Hao Chen <hao@apache.org>
Date: Sun, 12 Mar 2017 21:18:56 +0800
Subject: [PATCH] Fix AuthFilter

---
 .../server/security/BasicAuthRequestFilter.java  | 12 ++++++++----
 .../resource/BasicAuthenticationTestCase.java    | 16 ++++++++++++++--
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/eagle-server/src/main/java/org/apache/eagle/server/security/BasicAuthRequestFilter.java b/eagle-server/src/main/java/org/apache/eagle/server/security/BasicAuthRequestFilter.java
index 81a10c8c7b..ebdc93f0b3 100644
--- a/eagle-server/src/main/java/org/apache/eagle/server/security/BasicAuthRequestFilter.java
+++ b/eagle-server/src/main/java/org/apache/eagle/server/security/BasicAuthRequestFilter.java
@@ -54,6 +54,7 @@ public class BasicAuthRequestFilter implements ContainerRequestFilter {
     private static final Logger LOG = LoggerFactory.getLogger(BasicAuthRequestFilter.class);
     private boolean isSecurityDefined = false;
     private boolean isAuthRequired = false;
+    private boolean isAuthDefined = false;
     private boolean hasPermitAllAnnotation = false;
     private boolean hasDenyAllAnnotation = false;
     private boolean hasRolesAllowedAnnotation = false;
@@ -66,15 +67,16 @@ public BasicAuthRequestFilter(Authenticator<BasicCredentials, User> authenticato
         this.hasRolesAllowedAnnotation = method.isAnnotationPresent(RolesAllowed.class);
         this.isSecurityDefined = this.hasPermitAllAnnotation || this.hasDenyAllAnnotation || this.hasRolesAllowedAnnotation;
         for (Parameter parameter : method.getMethod().getParameters()) {
-            if (isSecurityDefined && isAuthRequired) {
+            if (isAuthRequired && isAuthDefined) {
                 break;
             }
             Auth[] authAnnotations = parameter.getAnnotationsByType(Auth.class);
-            this.isSecurityDefined = this.isSecurityDefined || authAnnotations.length > 0;
+            this.isAuthDefined = authAnnotations.length > 0 || this.isAuthDefined;
             for (Auth auth : authAnnotations) {
-                this.isAuthRequired = this.isAuthRequired || auth.required();
+                this.isAuthRequired = auth.required() || this.isAuthRequired;
             }
         }
+        this.isSecurityDefined = this.isAuthDefined || this.isSecurityDefined;
         Preconditions.checkArgument(!(this.hasDenyAllAnnotation && this.hasPermitAllAnnotation), "Conflict @DenyAll and @PermitAll on method " + this.method.toString());
     }
 
@@ -182,7 +184,9 @@ public String getAuthenticationScheme() {
                     throw new WebApplicationException(INVALID_ACCESS_FORBIDDEN);
                 }
             } else {
-                throw new WebApplicationException(UNAUTHORIZED_ACCESS_DENIED);
+                if (!this.isAuthDefined) {
+                    throw new WebApplicationException(UNAUTHORIZED_ACCESS_DENIED);
+                }
             }
         } catch (WebApplicationException e) {
             throw e;
diff --git a/eagle-server/src/test/java/org/apache/eagle/server/security/resource/BasicAuthenticationTestCase.java b/eagle-server/src/test/java/org/apache/eagle/server/security/resource/BasicAuthenticationTestCase.java
index c848d1987b..b8f2a43cdb 100644
--- a/eagle-server/src/test/java/org/apache/eagle/server/security/resource/BasicAuthenticationTestCase.java
+++ b/eagle-server/src/test/java/org/apache/eagle/server/security/resource/BasicAuthenticationTestCase.java
@@ -54,6 +54,18 @@ public void testAuthUserOnlyWitBadKey() {
             .get(User.class);
     }
 
+    @Test
+    public void testAuthUserOnlyWithoutKey() {
+        try {
+            Client client = new Client();
+            client.resource(String.format("http://localhost:%d/rest/testAuth/userOnly", RULE.getLocalPort()))
+                .get(User.class);
+            Assert.fail();
+        } catch (UniformInterfaceException e) {
+            Assert.assertEquals(401, e.getResponse().getStatus());
+        }
+    }
+
     @Test
     public void testAuthAdminOnly() {
         Client client = new Client();
@@ -142,7 +154,7 @@ public void testAuthPermitAllWithoutKeyShouldPass() {
     }
 
     @Test
-    public void testAuthPermitAllWithBadKeyShouldAccept401() {
+    public void testAuthPermitAllWithBadKeyShouldAccept403() {
         Client client = new Client();
         try {
             client.resource(String.format("http://localhost:%d/rest/testAuth/permitAll", RULE.getLocalPort()))
@@ -150,7 +162,7 @@ public void testAuthPermitAllWithBadKeyShouldAccept401() {
                 .get(User.class);
             Assert.fail();
         } catch (UniformInterfaceException e) {
-            Assert.assertEquals(401, e.getResponse().getStatus());
+            Assert.assertEquals(403, e.getResponse().getStatus());
         }
     }