From d9aa33525ab2b641dc98cffb5f3d4fe0c8a9ccaf Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 26 Jan 2022 15:51:49 +0100 Subject: [PATCH] Add support for adding multiple members to a group at once Adding multiple members at once is done via PATCH on `groups/{group-oid}` with a body containing a list of refernces to the members. --- graph/pkg/identity/backend.go | 7 +-- graph/pkg/identity/cs3.go | 4 +- graph/pkg/identity/ldap.go | 69 ++++++++++++++--------- graph/pkg/service/v0/groups.go | 88 +++++++++++++++++++++++++++--- graph/pkg/service/v0/instrument.go | 5 ++ graph/pkg/service/v0/logging.go | 5 ++ graph/pkg/service/v0/service.go | 2 + graph/pkg/service/v0/tracing.go | 5 ++ 8 files changed, 143 insertions(+), 42 deletions(-) diff --git a/graph/pkg/identity/backend.go b/graph/pkg/identity/backend.go index 6e80046862..49e71709b2 100644 --- a/graph/pkg/identity/backend.go +++ b/graph/pkg/identity/backend.go @@ -11,13 +11,10 @@ import ( type Backend interface { // CreateUser creates a given user in the identity backend. CreateUser(ctx context.Context, user libregraph.User) (*libregraph.User, error) - // DeleteUser deletes a given user, identified by username or id, from the backend DeleteUser(ctx context.Context, nameOrID string) error - // UpdateUser applies changes to given user, identified by username or id UpdateUser(ctx context.Context, nameOrID string, user libregraph.User) (*libregraph.User, error) - GetUser(ctx context.Context, nameOrID string) (*libregraph.User, error) GetUsers(ctx context.Context, queryParam url.Values) ([]*libregraph.User, error) @@ -28,8 +25,8 @@ type Backend interface { GetGroup(ctx context.Context, nameOrID string) (*libregraph.Group, error) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregraph.Group, error) GetGroupMembers(ctx context.Context, id string) ([]*libregraph.User, error) - // AddMemberToGroup adds a new member (reference by ID) to supplied group in the identity backend. - AddMemberToGroup(ctx context.Context, groupID string, memberID string) error + // AddMembersToGroup adds new members (reference by a slice of IDs) to supplied group in the identity backend. + AddMembersToGroup(ctx context.Context, groupID string, memberID []string) error // RemoveMemberFromGroup removes a single member (by ID) from a group RemoveMemberFromGroup(ctx context.Context, groupID string, memberID string) error } diff --git a/graph/pkg/identity/cs3.go b/graph/pkg/identity/cs3.go index f05a5cb1e8..e4b7c20549 100644 --- a/graph/pkg/identity/cs3.go +++ b/graph/pkg/identity/cs3.go @@ -184,8 +184,8 @@ func (i *CS3) GetGroupMembers(ctx context.Context, groupID string) ([]*libregrap return nil, errorcode.New(errorcode.NotSupported, "not implemented") } -// AddMemberToGroup implements the Backend Interface. It's currently not supported for the CS3 backend -func (i *CS3) AddMemberToGroup(ctx context.Context, groupID string, memberID string) error { +// AddMembersToGroup implements the Backend Interface. It's currently not supported for the CS3 backend +func (i *CS3) AddMembersToGroup(ctx context.Context, groupID string, memberID []string) error { return errorcode.New(errorcode.NotSupported, "not implemented") } diff --git a/graph/pkg/identity/ldap.go b/graph/pkg/identity/ldap.go index fb03594484..71f402eae3 100644 --- a/graph/pkg/identity/ldap.go +++ b/graph/pkg/identity/ldap.go @@ -568,19 +568,14 @@ func (i *LDAP) DeleteGroup(ctx context.Context, id string) error { return nil } -// AddMemberToGroup implements the Backend Interface for the LDAP backend. +// AddMembersToGroup implements the Backend Interface for the LDAP backend. // Currently it is limited to adding Users as Group members. Adding other groups // as members is not yet implemented -func (i *LDAP) AddMemberToGroup(ctx context.Context, groupID string, memberID string) error { +func (i *LDAP) AddMembersToGroup(ctx context.Context, groupID string, memberIDs []string) error { ge, err := i.getLDAPGroupByID(groupID, true) if err != nil { return err } - me, err := i.getLDAPUserByID(memberID) - if err != nil { - return err - } - i.logger.Debug().Str("backend", "ldap").Str("groupdn", ge.DN).Str("member", me.DN).Msg("Add Member") mr := ldap.ModifyRequest{DN: ge.DN} // Handle empty groups (using the empty member attribute) @@ -588,29 +583,47 @@ func (i *LDAP) AddMemberToGroup(ctx context.Context, groupID string, memberID st if len(current) == 1 && current[0] == "" { mr.Delete(i.groupAttributeMap.member, []string{""}) } - nUserDN, err := ldapdn.ParseNormalize(me.DN) - for _, member := range current { - if member == "" { - continue - } - if nMember, err := ldapdn.ParseNormalize(member); err != nil { - // We couldn't parse the member value as a DN. Let's keep it - // as it is but log a warning - i.logger.Warn().Str("memberDN", member).Err(err).Msg("Couldn't parse DN") - continue - } else { - if nMember == nUserDN { - i.logger.Info().Str("memberDN", member).Msg("User already present. Nothing to do") - return nil - } - } + // Create a Set of current members for faster lookups + currentSet := make(map[string]struct{}, len(current)) + for _, currentMember := range current { + // We can ignore any empty member value here + if currentMember == "" { + continue + } + nCurrentMember, err := ldapdn.ParseNormalize(currentMember) + if err != nil { + // We couldn't parse the member value as a DN. Let's skip it, but log a warning + i.logger.Warn().Str("memberDN", currentMember).Err(err).Msg("Couldn't parse DN") + continue + } + currentSet[nCurrentMember] = struct{}{} } - mr.Add(i.groupAttributeMap.member, []string{me.DN}) + var newMemberDNs []string + for _, memberID := range memberIDs { + me, err := i.getLDAPUserByID(memberID) + if err != nil { + return err + } + nDN, err := ldapdn.ParseNormalize(me.DN) + if err != nil { + i.logger.Error().Str("new member", me.DN).Err(err).Msg("Couldn't parse DN") + return err + } + if _, present := currentSet[nDN]; !present { + newMemberDNs = append(newMemberDNs, me.DN) + } else { + i.logger.Debug().Str("memberDN", me.DN).Msg("Member already present in group. Skipping") + } + } - if err := i.conn.Modify(&mr); err != nil { - return err + if len(newMemberDNs) > 0 { + mr.Add(i.groupAttributeMap.member, newMemberDNs) + + if err := i.conn.Modify(&mr); err != nil { + return err + } } return nil } @@ -630,6 +643,10 @@ func (i *LDAP) RemoveMemberFromGroup(ctx context.Context, groupID string, member i.logger.Debug().Str("backend", "ldap").Str("groupdn", ge.DN).Str("member", me.DN).Msg("remove member") nOldMemberDN, err := ldapdn.ParseNormalize(me.DN) + if err != nil { + i.logger.Error().Str("old member", me.DN).Err(err).Msg("Couldn't parse DN") + return err + } members := ge.GetEqualFoldAttributeValues(i.groupAttributeMap.member) found := false for _, member := range members { diff --git a/graph/pkg/service/v0/groups.go b/graph/pkg/service/v0/groups.go index ea1ae43bd3..f261cfc106 100644 --- a/graph/pkg/service/v0/groups.go +++ b/graph/pkg/service/v0/groups.go @@ -3,6 +3,7 @@ package svc import ( "encoding/json" "errors" + "fmt" "net/http" "net/url" "strings" @@ -14,6 +15,8 @@ import ( "github.com/go-chi/render" ) +const memberRefsLimit = 20 + // GetGroups implements the Service interface. func (g Graph) GetGroups(w http.ResponseWriter, r *http.Request) { groups, err := g.identityBackend.GetGroups(r.Context(), r.URL.Query()) @@ -61,6 +64,66 @@ func (g Graph) PostGroup(w http.ResponseWriter, r *http.Request) { render.JSON(w, r, grp) } +// PatchGroup implements the Service interface. +func (g Graph) PatchGroup(w http.ResponseWriter, r *http.Request) { + g.logger.Debug().Msg("Calling PatchGroup") + groupID := chi.URLParam(r, "groupID") + groupID, err := url.PathUnescape(groupID) + if err != nil { + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "unescaping group id failed") + return + } + + if groupID == "" { + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "missing group id") + return + } + changes := libregraph.NewGroup() + err = json.NewDecoder(r.Body).Decode(changes) + if err != nil { + errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) + return + } + + if memberRefs, ok := changes.GetMembersodataBindOk(); ok { + // The spec defines a limit of 20 members maxium per Request + if len(memberRefs) > memberRefsLimit { + errorcode.NotAllowed.Render(w, r, http.StatusInternalServerError, + fmt.Sprintf("Request is limited to %d members", memberRefsLimit)) + return + } + memberIDs := make([]string, 0, len(memberRefs)) + for _, memberRef := range memberRefs { + memberType, id, err := g.parseMemberRef(memberRef) + if err != nil { + errorcode.InvalidRequest.Render(w, r, http.StatusInternalServerError, "Error parsing member@odata.bind values") + return + } + g.logger.Debug().Str("memberType", memberType).Str("memberid", id).Msg("Add Member") + // The MS Graph spec allows "directoryObject", "user", "group" and "organizational Contact" + // we restrict this to users for now. Might add Groups as members later + if memberType != "users" { + errorcode.InvalidRequest.Render(w, r, http.StatusInternalServerError, "Only user are allowed as group members") + return + } + memberIDs = append(memberIDs, id) + } + err = g.identityBackend.AddMembersToGroup(r.Context(), groupID, memberIDs) + } + + if err != nil { + var errcode errorcode.Error + if errors.As(err, &errcode) { + errcode.Render(w, r) + } else { + errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) + } + return + } + render.Status(r, http.StatusNoContent) + render.NoContent(w, r) +} + // GetGroup implements the Service interface. func (g Graph) GetGroup(w http.ResponseWriter, r *http.Request) { groupID := chi.URLParam(r, "groupID") @@ -171,18 +234,11 @@ func (g Graph) PostGroupMember(w http.ResponseWriter, r *http.Request) { errorcode.InvalidRequest.Render(w, r, http.StatusInternalServerError, "@odata.id refernce is missing") return } - memberURL, err := url.ParseRequestURI(*memberRefURL) + memberType, id, err := g.parseMemberRef(*memberRefURL) if err != nil { errorcode.InvalidRequest.Render(w, r, http.StatusInternalServerError, "Error parsing @odata.id url") return } - segments := strings.Split(memberURL.Path, "/") - if len(segments) < 2 { - errorcode.InvalidRequest.Render(w, r, http.StatusInternalServerError, "Error parsing @odata.id url path") - return - } - id := segments[len(segments)-1] - memberType := segments[len(segments)-2] // The MS Graph spec allows "directoryObject", "user", "group" and "organizational Contact" // we restrict this to users for now. Might add Groups as members later if memberType != "users" { @@ -191,7 +247,7 @@ func (g Graph) PostGroupMember(w http.ResponseWriter, r *http.Request) { } g.logger.Debug().Str("memberType", memberType).Str("id", id).Msg("Add Member") - err = g.identityBackend.AddMemberToGroup(r.Context(), groupID, id) + err = g.identityBackend.AddMembersToGroup(r.Context(), groupID, []string{id}) if err != nil { var errcode errorcode.Error @@ -248,3 +304,17 @@ func (g Graph) DeleteGroupMember(w http.ResponseWriter, r *http.Request) { render.Status(r, http.StatusNoContent) render.NoContent(w, r) } + +func (g Graph) parseMemberRef(ref string) (string, string, error) { + memberURL, err := url.ParseRequestURI(ref) + if err != nil { + return "", "", err + } + segments := strings.Split(memberURL.Path, "/") + if len(segments) < 2 { + return "", "", errors.New("invalid member reference") + } + id := segments[len(segments)-1] + memberType := segments[len(segments)-2] + return memberType, id, nil +} diff --git a/graph/pkg/service/v0/instrument.go b/graph/pkg/service/v0/instrument.go index b3088ccddf..96b0183e55 100644 --- a/graph/pkg/service/v0/instrument.go +++ b/graph/pkg/service/v0/instrument.go @@ -69,6 +69,11 @@ func (i instrument) PostGroup(w http.ResponseWriter, r *http.Request) { i.next.PostGroup(w, r) } +// PatchGroup implements the Service interface. +func (i instrument) PatchGroup(w http.ResponseWriter, r *http.Request) { + i.next.PatchGroup(w, r) +} + // DeleteGroup implements the Service interface. func (i instrument) DeleteGroup(w http.ResponseWriter, r *http.Request) { i.next.DeleteGroup(w, r) diff --git a/graph/pkg/service/v0/logging.go b/graph/pkg/service/v0/logging.go index ac5b56a629..3ee85ff250 100644 --- a/graph/pkg/service/v0/logging.go +++ b/graph/pkg/service/v0/logging.go @@ -69,6 +69,11 @@ func (l logging) PostGroup(w http.ResponseWriter, r *http.Request) { l.next.PostGroup(w, r) } +// PatchGroup implements the Service interface. +func (l logging) PatchGroup(w http.ResponseWriter, r *http.Request) { + l.next.PatchGroup(w, r) +} + // DeleteGroup implements the Service interface. func (l logging) DeleteGroup(w http.ResponseWriter, r *http.Request) { l.next.DeleteGroup(w, r) diff --git a/graph/pkg/service/v0/service.go b/graph/pkg/service/v0/service.go index 1cbbba55c3..79859e070a 100644 --- a/graph/pkg/service/v0/service.go +++ b/graph/pkg/service/v0/service.go @@ -34,6 +34,7 @@ type Service interface { GetGroups(http.ResponseWriter, *http.Request) GetGroup(http.ResponseWriter, *http.Request) PostGroup(http.ResponseWriter, *http.Request) + PatchGroup(http.ResponseWriter, *http.Request) DeleteGroup(http.ResponseWriter, *http.Request) GetGroupMembers(http.ResponseWriter, *http.Request) PostGroupMember(http.ResponseWriter, *http.Request) @@ -121,6 +122,7 @@ func NewService(opts ...Option) Service { r.Route("/{groupID}", func(r chi.Router) { r.Get("/", svc.GetGroup) r.Delete("/", svc.DeleteGroup) + r.Patch("/", svc.PatchGroup) r.Route("/members", func(r chi.Router) { r.Get("/", svc.GetGroupMembers) r.Post("/$ref", svc.PostGroupMember) diff --git a/graph/pkg/service/v0/tracing.go b/graph/pkg/service/v0/tracing.go index 10e9e6c310..0d2581ba98 100644 --- a/graph/pkg/service/v0/tracing.go +++ b/graph/pkg/service/v0/tracing.go @@ -65,6 +65,11 @@ func (t tracing) PostGroup(w http.ResponseWriter, r *http.Request) { t.next.PostGroup(w, r) } +// PatchGroup implements the Service interface. +func (t tracing) PatchGroup(w http.ResponseWriter, r *http.Request) { + t.next.PatchGroup(w, r) +} + // DeleteGroup implements the Service interface. func (t tracing) DeleteGroup(w http.ResponseWriter, r *http.Request) { t.next.DeleteGroup(w, r)