From d46630c7be40a43b1d6c367e8bb325b5d80c089a Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Tue, 8 Dec 2020 07:51:13 +0100 Subject: [PATCH] Implement SCIM PATCH for group (role) membership --- pkg/options/SCIM.gen.go | 2 +- system/scim/group_handler.go | 120 +++++++++++++++++++++++++++++++++- system/scim/http.go | 5 ++ system/scim/patch_payloads.go | 37 +++++++++++ system/scim/routes.go | 6 +- system/scim/user_handler.go | 10 +-- tests/system/main_test.go | 12 ++++ tests/system/role_test.go | 17 +++++ tests/system/scim_test.go | 71 ++++++++++++++++++-- 9 files changed, 263 insertions(+), 17 deletions(-) create mode 100644 system/scim/patch_payloads.go diff --git a/pkg/options/SCIM.gen.go b/pkg/options/SCIM.gen.go index 6f1c02ccd..a93fa5ff4 100644 --- a/pkg/options/SCIM.gen.go +++ b/pkg/options/SCIM.gen.go @@ -22,7 +22,7 @@ type ( func SCIM() (o *SCIMOpt) { o = &SCIMOpt{ BaseURL: "/scim", - ExternalIdValidation: "$[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}^", + ExternalIdValidation: "^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}$", } fill(o) diff --git a/system/scim/group_handler.go b/system/scim/group_handler.go index ccf98b5a2..5fb61ce51 100644 --- a/system/scim/group_handler.go +++ b/system/scim/group_handler.go @@ -4,9 +4,9 @@ import ( "context" "fmt" "github.com/cortezaproject/corteza-server/pkg/errors" + "github.com/cortezaproject/corteza-server/store" "github.com/cortezaproject/corteza-server/system/service" "github.com/cortezaproject/corteza-server/system/types" - "github.com/davecgh/go-spew/spew" "github.com/go-chi/chi" "net/http" "regexp" @@ -19,7 +19,7 @@ type ( externalIdValidator *regexp.Regexp svc service.RoleService - passSvc passwordSetter + userSvc service.UserService sec getSecurityContextFn } ) @@ -111,6 +111,121 @@ func (h groupsHandler) replace(w http.ResponseWriter, r *http.Request) { send(w, status, newGroupResourceResponse(res)) } +// patches group +// +// only supports adding and removing members +func (h groupsHandler) patch(w http.ResponseWriter, r *http.Request) { + defer r.Body.Close() + + var ( + ctx = h.sec(r) + svc = h.svc.With(ctx) + res = h.lookup(ctx, chi.URLParam(r, "id"), w) + payload = &operationsRequest{} + ) + + if res == nil { + return + } + + if err := payload.decodeJSON(r.Body); err != nil { + sendError(w, newErrorResponse(http.StatusBadRequest, err)) + return + } + + var ( + u *types.User + ops = make([]func() error, 0, len(payload.Operations)) + err error + memberships = make(map[uint64]bool) + ) + + { + // collect all existing memberships into a simple map + // to ensure we dont step on our feet (too much) + // + // this is not 100% bulletproof for concurrent modifications + mm, _, err := store.SearchRoleMembers(ctx, service.DefaultStore, types.RoleMemberFilter{RoleID: res.ID}) + if err != nil { + sendError(w, err) + return + } + + for _, m := range mm { + memberships[m.UserID] = true + } + } + + // validate and collect operations + for _, op := range payload.Operations { + if op.Path != "members" { + // allow only "members" path + sendError(w, newErrorfResponse(http.StatusBadRequest, "unsupported path: %q", op.Path)) + return + } + + // iterate through operation's values, load user and schedule op + for _, userExternalId := range op.Value { + u, err = lookupUserByExternalId(ctx, h.userSvc, h.externalIdValidator, userExternalId.Value) + if err != nil { + sendError(w, err) + return + } + + if u == nil { + sendError(w, newErrorfResponse(http.StatusBadRequest, "no such user: %q", userExternalId.Value)) + return + } + + // making sure u is not overwritten + // in the next iteration + memberId := u.ID + + switch op.Operation { + case patchOpAdd: + // support for add operation, + // check if there members already exist + ops = append(ops, func() error { + if memberships[memberId] { + // already added + return nil + } + + memberships[memberId] = true + return svc.MemberAdd(res.ID, memberId) + }) + + case patchOpRemove: + // support for remove operation, + // check if there members are missing + ops = append(ops, func() error { + if !memberships[memberId] { + // already removed + return nil + } + + delete(memberships, memberId) + return svc.MemberRemove(res.ID, memberId) + }) + + default: + sendError(w, newErrorfResponse(http.StatusBadRequest, "unsupported operation: %q", op.Operation)) + return + } + } + } + + // run all scheduled ops + for _, op := range ops { + if err = op(); err != nil { + sendError(w, err) + return + } + } + + send(w, http.StatusNoContent, nil) +} + func (h groupsHandler) save(ctx context.Context, req *groupResourceRequest, existing *types.Role) (res *types.Role, err error) { var ( svc = h.svc.With(ctx) @@ -194,7 +309,6 @@ func (h groupsHandler) lookup(ctx context.Context, id string, w http.ResponseWri } func (h groupsHandler) lookupByExternalId(ctx context.Context, id string) (r *types.Role, err error) { - spew.Dump(id) if h.externalIdValidator != nil && !h.externalIdValidator.MatchString(id) { return nil, newErrorfResponse(http.StatusBadRequest, "invalid external ID") } diff --git a/system/scim/http.go b/system/scim/http.go index cdae99c45..847daf294 100644 --- a/system/scim/http.go +++ b/system/scim/http.go @@ -9,6 +9,11 @@ import ( func send(w http.ResponseWriter, status int, payload interface{}) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) + + if status == http.StatusNoContent || payload == nil { + return + } + if err := json.NewEncoder(w).Encode(payload); err != nil { log.Error("could not encode payload", zap.Error(err)) } diff --git a/system/scim/patch_payloads.go b/system/scim/patch_payloads.go new file mode 100644 index 000000000..7df2ea577 --- /dev/null +++ b/system/scim/patch_payloads.go @@ -0,0 +1,37 @@ +package scim + +import ( + "encoding/json" + "fmt" + "io" +) + +const ( + urnPatchOp = "urn:ietf:params:scim:schemas:core:2.0:PatchOp" + patchOpAdd = "add" + patchOpRemove = "remove" +) + +type ( + // very crud operations support + operationsRequest struct { + Schemas []string `json:"schemas"` + Operations []operationRequest `json:"Operations"` + } + + operationRequest struct { + Operation string `json:"op"` + Path string `json:"path"` + Value []struct { + Value string `json:"value"` + } `json:"value"` + } +) + +func (req *operationsRequest) decodeJSON(r io.Reader) error { + if err := json.NewDecoder(r).Decode(req); err != nil { + return fmt.Errorf("could not decode operations payload: %w", err) + } + + return nil +} diff --git a/system/scim/routes.go b/system/scim/routes.go index 73a21c746..a0cc6afc0 100644 --- a/system/scim/routes.go +++ b/system/scim/routes.go @@ -72,13 +72,15 @@ func Routes(r chi.Router, cfg Config) { externalIdAsPrimary: cfg.ExternalIdAsPrimary, externalIdValidator: cfg.ExternalIdValidator, - svc: service.DefaultRole, - sec: getSecurityContext, + svc: service.DefaultRole, + userSvc: service.DefaultUser, + sec: getSecurityContext, } r.Get("/{id}", gh.get) r.Post("/", gh.create) r.Put("/{id}", gh.replace) + r.Patch("/{id}", gh.patch) r.Delete("/{id}", gh.delete) }) } diff --git a/system/scim/user_handler.go b/system/scim/user_handler.go index d7d020d54..ededd4177 100644 --- a/system/scim/user_handler.go +++ b/system/scim/user_handler.go @@ -6,7 +6,6 @@ import ( "github.com/cortezaproject/corteza-server/pkg/errors" "github.com/cortezaproject/corteza-server/system/service" "github.com/cortezaproject/corteza-server/system/types" - "github.com/davecgh/go-spew/spew" "github.com/go-chi/chi" "net/http" "regexp" @@ -206,12 +205,15 @@ func (h usersHandler) lookup(ctx context.Context, id string, w http.ResponseWrit } func (h usersHandler) lookupByExternalId(ctx context.Context, id string) (r *types.User, err error) { - spew.Dump(id) - if h.externalIdValidator != nil && !h.externalIdValidator.MatchString(id) { + return lookupUserByExternalId(ctx, h.svc, h.externalIdValidator, id) +} + +func lookupUserByExternalId(ctx context.Context, svc service.UserService, v *regexp.Regexp, id string) (r *types.User, err error) { + if v != nil && !v.MatchString(id) { return nil, newErrorfResponse(http.StatusBadRequest, "invalid external ID") } - rr, _, err := h.svc.With(ctx).Find(types.UserFilter{Labels: map[string]string{groupLabel_SCIM_externalId: id}}) + rr, _, err := svc.With(ctx).Find(types.UserFilter{Labels: map[string]string{userLabel_SCIM_externalId: id}}) if err != nil { return nil, newErrorResponse(http.StatusInternalServerError, err) } diff --git a/tests/system/main_test.go b/tests/system/main_test.go index b8ebb69a4..baba1c4c8 100644 --- a/tests/system/main_test.go +++ b/tests/system/main_test.go @@ -9,10 +9,13 @@ import ( "github.com/cortezaproject/corteza-server/pkg/cli" "github.com/cortezaproject/corteza-server/pkg/eventbus" "github.com/cortezaproject/corteza-server/pkg/id" + label "github.com/cortezaproject/corteza-server/pkg/label" + ltype "github.com/cortezaproject/corteza-server/pkg/label/types" "github.com/cortezaproject/corteza-server/pkg/logger" "github.com/cortezaproject/corteza-server/pkg/objstore/plain" "github.com/cortezaproject/corteza-server/pkg/rand" "github.com/cortezaproject/corteza-server/pkg/rbac" + "github.com/cortezaproject/corteza-server/store" "github.com/cortezaproject/corteza-server/store/sqlite3" "github.com/cortezaproject/corteza-server/system/rest" "github.com/cortezaproject/corteza-server/system/service" @@ -165,3 +168,12 @@ func (h helper) noError(err error) { h.a.NoError(err) } + +func (h helper) setLabel(res label.LabeledResource, name, value string) { + h.a.NoError(store.UpsertLabel(h.secCtx(), service.DefaultStore, <ype.Label{ + Kind: res.LabelResourceKind(), + ResourceID: res.LabelResourceID(), + Name: name, + Value: value, + })) +} diff --git a/tests/system/role_test.go b/tests/system/role_test.go index d62fb753f..896a53906 100644 --- a/tests/system/role_test.go +++ b/tests/system/role_test.go @@ -20,6 +20,23 @@ func (h helper) clearRoles() { h.noError(store.TruncateRoles(context.Background(), service.DefaultStore)) } +func (h helper) clearRoleMembers() { + h.noError(store.TruncateRoleMembers(context.Background(), service.DefaultStore)) +} + +func (h helper) createRole(res *types.Role) *types.Role { + if res.ID == 0 { + res.ID = id.Next() + } + + if res.CreatedAt.IsZero() { + res.CreatedAt = time.Now() + } + + h.a.NoError(service.DefaultStore.CreateRole(context.Background(), res)) + return res +} + func (h helper) repoMakeRole(ss ...string) *types.Role { var r = &types.Role{ ID: id.Next(), diff --git a/tests/system/scim_test.go b/tests/system/scim_test.go index 87db631dc..68e796e20 100644 --- a/tests/system/scim_test.go +++ b/tests/system/scim_test.go @@ -4,11 +4,11 @@ import ( "context" "fmt" "github.com/cortezaproject/corteza-server/pkg/api/server" - "github.com/cortezaproject/corteza-server/pkg/label/types" "github.com/cortezaproject/corteza-server/pkg/logger" "github.com/cortezaproject/corteza-server/store" "github.com/cortezaproject/corteza-server/system/scim" "github.com/cortezaproject/corteza-server/system/service" + "github.com/cortezaproject/corteza-server/system/types" "github.com/go-chi/chi" "github.com/steinfletcher/apitest" jsonpath "github.com/steinfletcher/apitest-jsonpath" @@ -299,12 +299,7 @@ func TestScimUserReplaceOnExternalId(t *testing.T) { // creating a new user and assigning an external ID label to it u := h.createUserWithEmail(h.randEmail()) const externalId = `2819c223-7f76-453a-919d-413861904646` - h.a.NoError(store.UpsertLabel(h.secCtx(), service.DefaultStore, &types.Label{ - Kind: u.LabelResourceKind(), - ResourceID: u.LabelResourceID(), - Name: "SCIM_externalId", - Value: externalId, - })) + h.setLabel(u, "SCIM_externalId", externalId) h.scimApiInit(scimSetWithExternalId, scimSetWithUUIDValidator). Put(fmt.Sprintf("/Users/%s", externalId)). @@ -319,6 +314,68 @@ func TestScimUserReplaceOnExternalId(t *testing.T) { h.a.Equal("baz@bar.com", u.Email) } +func TestScimPatchingGroupMembership(t *testing.T) { + h := newHelper(t) + h.clearUsers() + h.clearRoles() + h.clearRoleMembers() + + isMember := func(r *types.Role, u *types.User) bool { + mm, _, err := store.SearchRoleMembers(h.secCtx(), service.DefaultStore, types.RoleMemberFilter{RoleID: r.ID, UserID: u.ID}) + h.a.NoError(err) + return len(mm) > 0 + } + + const ( + user1Id = `00000000-0000-0000-0000-000000000001` + user2Id = `00000000-0000-0000-0000-000000000002` + groupId = `00000000-0000-0000-0001-000000000001` + ) + + // creating a new user and assigning an external ID label to it + u1 := h.createUserWithEmail(h.randEmail()) + h.setLabel(u1, "SCIM_externalId", user1Id) + + u2 := h.createUserWithEmail(h.randEmail()) + h.setLabel(u2, "SCIM_externalId", user2Id) + + r := h.createRole(&types.Role{}) + h.setLabel(r, "SCIM_externalId", groupId) + + h.a.False(isMember(r, u1)) + h.a.False(isMember(r, u2)) + + // add only user #1 + h.scimApiInit(scimSetWithExternalId, scimSetWithUUIDValidator). + Patch(fmt.Sprintf("/Groups/%s", groupId)). + JSON(fmt.Sprintf( + `{"Operations":[{"op":"add","path":"members","value":[{"value":%q}]}],"schemas":["urn:ietf:params:scim:schemas:core:2.0:PatchOp"]}`, + user1Id, + )). + Expect(t). + Status(http.StatusNoContent). + End() + + h.a.True(isMember(r, u1)) + h.a.False(isMember(r, u2)) + + // remove user #1, add user #2 + h.scimApiInit(scimSetWithExternalId, scimSetWithUUIDValidator). + Patch(fmt.Sprintf("/Groups/%s", groupId)). + JSON(fmt.Sprintf( + `{"Operations":[{"op":"add","path":"members","value":[{"value":%q}]},{"op":"remove","path":"members","value":[{"value":%q}]}],"schemas":["urn:ietf:params:scim:schemas:core:2.0:PatchOp"]}`, + user2Id, + user1Id, + )). + Expect(t). + Status(http.StatusNoContent). + End() + + h.a.False(isMember(r, u1)) + h.a.True(isMember(r, u2)) + +} + func scimSetWithExternalId(c *scim.Config) { c.ExternalIdAsPrimary = true }