From 4e6fe6c77b42f6edbf4236eed9a7af13c0cf10ee Mon Sep 17 00:00:00 2001 From: Rohith Date: Tue, 3 Jul 2018 16:15:27 +0100 Subject: [PATCH 1/4] - adding the require-any-roles attributes to the resource --- doc.go | 2 ++ resource.go | 6 ++++++ resource_test.go | 5 +++++ 3 files changed, 13 insertions(+) diff --git a/doc.go b/doc.go index bd469648b..d16f27b64 100644 --- a/doc.go +++ b/doc.go @@ -130,6 +130,8 @@ type Resource struct { Methods []string `json:"methods" yaml:"methods"` // WhiteListed permits the prefix through WhiteListed bool `json:"white-listed" yaml:"white-listed"` + // RequireAnyRole indicates that ANY of the roles are required, the default is all + RequireAnyRole bool `json:"require-any-role yaml:"require-any-role"` // Roles the roles required to access this url Roles []string `json:"roles" yaml:"roles"` // Groups is a list of groups the user is in diff --git a/resource.go b/resource.go index e89c6f788..2b1b1d63b 100644 --- a/resource.go +++ b/resource.go @@ -51,6 +51,12 @@ func (r *Resource) parse(resource string) (*Resource, error) { r.Methods = allHTTPMethods } } + case "require-any-role": + v, err := strconv.ParseBool(kp[1]) + if err != nil { + return nil, err + } + r.RequireAnyRole = v case "roles": r.Roles = strings.Split(kp[1], ",") case "groups": diff --git a/resource_test.go b/resource_test.go index d01354749..71f240113 100644 --- a/resource_test.go +++ b/resource_test.go @@ -30,6 +30,7 @@ func TestDecodeResourceBad(t *testing.T) { {Option: "uri"}, {Option: "uri=hello"}, {Option: "uri=/|white-listed=ERROR"}, + {Option: "uri=/|require-any-role=BAD"}, } for i, c := range cs { if _, err := newResource().parse(c.Option); err == nil { @@ -79,6 +80,10 @@ func TestResourceParseOk(t *testing.T) { Option: "uri=/*|groups=admin", Resource: &Resource{URL: "/*", Methods: allHTTPMethods, Groups: []string{"admin"}}, }, + { + Option: "uri=/*|require-any-role=true", + Resource: &Resource{URL: "/*", Methods: allHTTPMethods, RequireAnyRole: true}, + }, } for i, x := range cs { r, err := newResource().parse(x.Option) From 92bdbd0d29634d32f1a5e1f40e2777aa9bd81eb4 Mon Sep 17 00:00:00 2001 From: Rohith Date: Tue, 3 Jul 2018 16:25:09 +0100 Subject: [PATCH 2/4] Require Any Roles - adding the inclusion of a 'require-any-role' on the resource to waive the default operation and permit as only as one of the roles is present --- CHANGELOG.md | 5 +++++ middleware.go | 2 +- middleware_test.go | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3afdb47f..c949d2fd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,9 @@ +#### **2.2.3 (Unreleased)** + +FEATURES: +* Added the ability to use a "any" operation on the roles rather then just "and" with the inclusion of a `require-any-role` [#PR387](https://github.com/gambol99/keycloak-proxy/pull/387) + #### **2.2.2** FEATURES: diff --git a/middleware.go b/middleware.go index 0488f3c8f..1c12ecb8b 100644 --- a/middleware.go +++ b/middleware.go @@ -289,7 +289,7 @@ func (r *oauthProxy) admissionMiddleware(resource *Resource) func(http.Handler) user := scope.Identity // @step: we need to check the roles - if !hasAccess(resource.Roles, user.roles, true) { + if !hasAccess(resource.Roles, user.roles, !resource.RequireAnyRole) { r.log.Warn("access denied, invalid roles", zap.String("access", "denied"), zap.String("email", user.email), diff --git a/middleware_test.go b/middleware_test.go index ea0ad3103..9d808354c 100644 --- a/middleware_test.go +++ b/middleware_test.go @@ -577,6 +577,38 @@ func TestWhiteListedRequests(t *testing.T) { newFakeProxy(cfg).RunTests(t, requests) } +func TestRequireAnyRoles(t *testing.T) { + cfg := newFakeKeycloakConfig() + cfg.Resources = []*Resource{ + { + URL: "/require_any_role/*", + Methods: allHTTPMethods, + RequireAnyRole: true, + Roles: []string{"admin", "guest"}, + }, + } + requests := []fakeRequest{ + { + URI: "/require_any_role/test", + ExpectedCode: http.StatusUnauthorized, + }, + { + URI: "/require_any_role/test", + HasToken: true, + Roles: []string{"guest"}, + ExpectedCode: http.StatusOK, + ExpectedProxy: true, + }, + { + URI: "/require_any_role/test", + HasToken: true, + Roles: []string{"guest1"}, + ExpectedCode: http.StatusForbidden, + }, + } + newFakeProxy(cfg).RunTests(t, requests) +} + func TestGroupPermissionsMiddleware(t *testing.T) { cfg := newFakeKeycloakConfig() cfg.Resources = []*Resource{ From aa7c72178397deccc8a5d20465d27f876a635d44 Mon Sep 17 00:00:00 2001 From: Rohith Date: Tue, 3 Jul 2018 16:30:44 +0100 Subject: [PATCH 3/4] - updating the README to reflect the changes --- CHANGELOG.md | 2 +- README.md | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c949d2fd8..606f18137 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ #### **2.2.3 (Unreleased)** FEATURES: -* Added the ability to use a "any" operation on the roles rather then just "and" with the inclusion of a `require-any-role` [#PR387](https://github.com/gambol99/keycloak-proxy/pull/387) +* Added the ability to use a "any" operation on the roles rather then just "and" with the inclusion of a `require-any-role` [#PR389](https://github.com/gambol99/keycloak-proxy/pull/389) #### **2.2.2** diff --git a/README.md b/README.md index 955b67e3e..45261bfa1 100644 --- a/README.md +++ b/README.md @@ -217,6 +217,7 @@ resources: roles: - client:test1 - client:test2 + require-any-role: true groups: - admins - users @@ -253,7 +254,9 @@ bin/keycloak-proxy \ --resources="uri=/public/*|white-listed=true" ``` -Note from release 2.2.0 the `--enable-default-deny` is true by default and should explicityly allow what you want through. +Note from release 2.2.0 the `--enable-default-deny` is true by default and should explicitly allow what you want through. + +By default the roles defined on a resource perform a logical `AND` so all roles specified must be present in the claims, this behavior can be altered by the `require-any-role` option however so as long as one role is present the permission is granted. #### **HTTP Routing** From 48e5faab13817c5b9f303d1084b0e3f7fb09f618 Mon Sep 17 00:00:00 2001 From: Rohith Date: Tue, 3 Jul 2018 16:40:35 +0100 Subject: [PATCH 4/4] - fixed up typo on the json tags --- doc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc.go b/doc.go index d16f27b64..22320d6e4 100644 --- a/doc.go +++ b/doc.go @@ -131,7 +131,7 @@ type Resource struct { // WhiteListed permits the prefix through WhiteListed bool `json:"white-listed" yaml:"white-listed"` // RequireAnyRole indicates that ANY of the roles are required, the default is all - RequireAnyRole bool `json:"require-any-role yaml:"require-any-role"` + RequireAnyRole bool `json:"require-any-role" yaml:"require-any-role"` // Roles the roles required to access this url Roles []string `json:"roles" yaml:"roles"` // Groups is a list of groups the user is in