Merge pull request #1039 from eloycoto/THREESCALE-2236

[THREESCALE-2236] Add methods option on Keycloak policy.

Author: davidor

CHANGELOG.md

@@ -12,8 +12,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - "Upstream Connection" policy. It allows to configure several options for the connections to the upstream [PR #1025](https://github.com/3scale/APIcast/pull/1025), [THREESCALE-2166](https://issues.jboss.org/browse/THREESCALE-2166)
 - Enable APICAST_EXTENDED_METRICS environment variable to provide additional details [PR #1024](https://github.com/3scale/APIcast/pull/1024) [THREESCALE-2150](https://issues.jboss.org/browse/THREESCALE-2150)
 - Add the option to obtain client_id from any JWT claim [THREESCALE-2264](https://issues.jboss.org/browse/THREESCALE-2264) [PR #1034](https://github.com/3scale/APIcast/pull/1034)
-
 - Added `APICAST_PATH_ROUTING_ONLY` variable that allows to perform path-based routing without falling back to the default host-based routing [PR #1035](https://github.com/3scale/APIcast/pull/1035), [THREESCALE-1150](https://issues.jboss.org/browse/THREESCALE-1150)
+- Added the option to manage access based on method on Keycloak Policy. [THREESCALE-2236](https://issues.jboss.org/browse/THREESCALE-2236) [PR #1039](https://github.com/3scale/APIcast/pull/1039)
 
 ### Fixed
 

gateway/src/apicast/mapping_rule.lua

@@ -13,7 +13,9 @@ local re_match = ngx.re.match
 local insert = table.insert
 local re_gsub = ngx.re.gsub
 
-local _M = {}
+local _M = {
+  any_method = "ANY"
+}
 
 local mt = { __index = _M }
 
@@ -132,7 +134,7 @@ end
 -- @tparam table args Table with the args and values of an HTTP request.
 -- @treturn boolean Whether the mapping rule matches the given request.
 function _M:matches(method, uri, args)
-  local match = self.method == method and
+  local match = (self.method == self.any_method or self.method == method) and
       matches_uri(self.regexpified_pattern, uri) and
       self.querystring_params(args)
 

gateway/src/apicast/policy/keycloak_role_check/README.md

@@ -104,3 +104,17 @@
     ]
   }
   ```
+
+- When you want to allow those who have the realm role `role1` to access `/resource1` and only methods GET and POST.
+
+  ```json
+  {
+    "scopes": [
+      {
+        "realm_roles": [ { "name": "role1" } ],
+        "resource": "/resource1",
+        "methods": ["GET", "POST"]
+      }
+    ]
+  }
+  ```

gateway/src/apicast/policy/keycloak_role_check/apicast-policy.json

@@ -77,6 +77,26 @@
             "resource_type": {
               "description": "How to evaluate 'resource'",
               "$ref": "#/definitions/value_type"
+            },
+            "methods": {
+                "description": "Allowed methods",
+                "type": "array",
+                "default": ["ANY"],
+                "items": {
+                    "type": "string",
+                    "enum": [
+                        "ANY",
+                        "GET",
+                        "HEAD",
+                        "POST",
+                        "PUT",
+                        "DELETE",
+                        "PATCH",
+                        "OPTIONS",
+                        "TRACE",
+                        "CONNECT"
+                    ]
+                }
             }
           }
         }

gateway/src/apicast/policy/keycloak_role_check/keycloak_role_check.lua

@@ -60,11 +60,13 @@ local default_type = 'plain'
 
 local new = _M.new
 
+local any_method = MappingRule.any_method
+
 local function create_template(value, value_type)
   return TemplateString.new(value, value_type or default_type)
 end
 
-local function build_templates(scopes)
+local function build_scopes(scopes)
   for _, scope in ipairs(scopes) do
 
     if scope.realm_roles then
@@ -86,16 +88,20 @@ local function build_templates(scopes)
 
     scope.resource_template_string = create_template(
       scope.resource, scope.resource_type)
+    if (not scope.methods) or (scope.methods and #scope.methods == 0 ) then
+      scope.methods = { any_method }
+    end
 
   end
+
 end
 
 function _M.new(config)
   local self = new()
   self.type = config.type or "whitelist"
   self.scopes = config.scopes or {}
 
-  build_templates(self.scopes)
+  build_scopes(self.scopes)
 
   return self
 end
@@ -152,38 +158,47 @@ local function match_client_roles(scope, context)
   return true
 end
 
-local function scope_check(scopes, context)
-  local uri = ngx.var.uri
-
-  if not context.jwt then
-    return false
-  end
-
-  for _, scope in ipairs(scopes) do
+local function validate_scope_access(scope, context, uri, request_method)
+  for _, method  in ipairs(scope.methods) do
 
     local resource = scope.resource_template_string:render(context)
 
     local mapping_rule = MappingRule.from_proxy_rule({
-      http_method = 'ANY',
+      http_method = method,
       pattern = resource,
       querystring_parameters = {},
       -- the name of the metric is irrelevant
       metric_system_name = 'hits'
     })
 
-    if mapping_rule:matches('ANY', uri) then
+    if mapping_rule:matches(request_method, uri) then
       if match_realm_roles(scope, context) and match_client_roles(scope, context) then
         return true
       end
     end
+  end
+  return false
+end
+
+local function scopes_check(scopes, context)
+  local uri = ngx.var.uri
+  local request_method =  ngx.req.get_method()
+
+  if not context.jwt then
+    return false
+  end
 
+  for _, scope in ipairs(scopes) do
+    if validate_scope_access(scope, context, uri, request_method) then
+      return true
+    end
   end
 
   return false
 end
 
 function _M:access(context)
-  if scope_check(self.scopes, context) then
+  if scopes_check(self.scopes, context) then
     if self.type == "blacklist" then
       return errors.authorization_failed(context.service)
     end

spec/mapping_rule_spec.lua

@@ -120,6 +120,26 @@ describe('mapping_rule', function()
       assert.is_true(mapping_rule:matches('GET', '/foo/a:b/bar'))
       assert.is_true(mapping_rule:matches('GET', "/foo/a%b/bar"))
     end)
+  end)
+
+  describe('.any_method', function()
+
+    it("Allow connections when any method is defined", function()
 
+      local mapping_rule = MappingRule.from_proxy_rule({
+        http_method = MappingRule.any_method,
+        pattern = '/foo/',
+        querystring_parameters = { },
+        metric_system_name = 'hits',
+        delta = 1
+      })
+
+      assert.is_true(mapping_rule:matches('GET', '/foo/'))
+      assert.is_true(mapping_rule:matches('POST', '/foo/'))
+      assert.is_true(mapping_rule:matches('PUT', "/foo/"))
+      assert.is_true(mapping_rule:matches('DELETE', "/foo/"))
+      assert.is_true(mapping_rule:matches('PATCH', "/foo/"))
+    end)
   end)
+
 end)

spec/policy/keycloak_role_check/keycloak_role_check_spec.lua

@@ -7,6 +7,7 @@ describe('Keycloak Role check policy', function()
     ngx.header = {}
     stub(ngx, 'print')
 
+    stub(ngx.req, 'get_method', function() return 'GET' end)
     -- avoid stubbing all the ngx.var.* and ngx.req.* in the available context
     stub(ngx_variable, 'available_context', function(context) return context end)
   end)
@@ -572,6 +573,95 @@ describe('Keycloak Role check policy', function()
           assert.not_same(ngx.status, 403)
         end)
       end)
+
+      describe("validate method roles", function()
+
+        before_each(function()
+          ngx.var = {
+            uri = '/foo'
+          }
+        end)
+
+        local context = {
+          jwt = {
+            realm_access = {
+              roles = { "known_role" }
+            },
+            resource_access = {
+              known_client = {
+                roles = { "role_of_known_client" }
+              }
+            }
+          },
+          service = {
+            auth_failed_status = 403,
+            error_auth_failed = "auth failed"
+          }
+        }
+
+        it("Validates that an empty array still make a default any method", function()
+          local role_check_policy = KeycloakRoleCheckPolicy.new({
+            scopes = {{
+                client_roles = { { name = "role_of_known_client", client = "known_client" } },
+                resource = "/foo",
+                methods = {}
+              }}})
+          role_check_policy:access(context)
+          assert.same(ngx.status, nil)
+        end)
+
+        it("when jwt role matches and one method also match", function()
+          local role_check_policy = KeycloakRoleCheckPolicy.new({
+            scopes = {{
+                client_roles = { { name = "role_of_known_client", client = "known_client" } },
+                resource = "/foo",
+                methods = {"GET", "POST", "PUT"}
+              }},
+            type = "whitelist"})
+
+          role_check_policy:access(context)
+          assert.same(ngx.status, nil)
+        end)
+
+        it("when jwt role matches and one method also match with blacklist mode", function()
+          local role_check_policy = KeycloakRoleCheckPolicy.new({
+            scopes = {{
+                client_roles = { { name = "role_of_known_client", client = "known_client" } },
+                resource = "/foo",
+                methods = {"GET", "POST", "PUT"}
+              }},
+            type = "blacklist"})
+
+          role_check_policy:access(context)
+          assert.same(ngx.status, 403)
+        end)
+
+        it("when jwt role matches and no methods matches", function()
+          local role_check_policy = KeycloakRoleCheckPolicy.new({
+            scopes = {{
+                client_roles = { { name = "role_of_known_client", client = "known_client" } },
+                resource = "/foo",
+                methods = {"POST", "PUT"}
+              }},
+            type = "whitelist"})
+            role_check_policy:access(context)
+          assert.same(ngx.status, 403)
+        end)
+
+        it("when jwt role matches and no methods matches with blacklist mode", function()
+          local role_check_policy = KeycloakRoleCheckPolicy.new({
+            scopes = {{
+                client_roles = { { name = "role_of_known_client", client = "known_client" } },
+                resource = "/foo",
+                methods = {"POST", "PUT"}
+              }},
+            type = "blacklist"})
+          role_check_policy:access(context)
+          assert.same(ngx.status, nil)
+        end)
+
+      end)
+
     end)
   end)
 end)