From 52b7f41624cb28ea8707676d3ebaff584095b13d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 18 Jan 2023 15:51:53 +0100 Subject: [PATCH] Populate expanded properties (#5421) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer Signed-off-by: Jörn Friedrich Dreyer --- .../populate-expanded-properties.md | 6 ++ services/graph/pkg/identity/ldap.go | 14 +--- services/graph/pkg/identity/ldap_group.go | 10 ++- services/graph/pkg/service/v0/drives.go | 20 +++--- services/graph/pkg/service/v0/drives_test.go | 2 +- .../graph/pkg/service/v0/educationuser.go | 2 +- services/graph/pkg/service/v0/graph_test.go | 3 + services/graph/pkg/service/v0/users.go | 65 ++++++++++--------- 8 files changed, 62 insertions(+), 60 deletions(-) create mode 100644 changelog/unreleased/populate-expanded-properties.md diff --git a/changelog/unreleased/populate-expanded-properties.md b/changelog/unreleased/populate-expanded-properties.md new file mode 100644 index 0000000000..b3ea06a435 --- /dev/null +++ b/changelog/unreleased/populate-expanded-properties.md @@ -0,0 +1,6 @@ +Bugfix: Populate expanded properties + +We now return an empty array when an expanded relation has no entries. This makes consuming the responses a little easier. + +https://github.com/owncloud/ocis/pull/5421 +https://github.com/owncloud/ocis/issues/5419 \ No newline at end of file diff --git a/services/graph/pkg/identity/ldap.go b/services/graph/pkg/identity/ldap.go index a7a34ec932..ade4a9e2d0 100644 --- a/services/graph/pkg/identity/ldap.go +++ b/services/graph/pkg/identity/ldap.go @@ -387,10 +387,7 @@ func (i *LDAP) GetUser(ctx context.Context, nameOrID string, queryParam url.Valu if err != nil { return nil, err } - if len(userGroups) > 0 { - groups := i.groupsFromLDAPEntries(userGroups) - u.MemberOf = groups - } + u.MemberOf = i.groupsFromLDAPEntries(userGroups) } return u, nil } @@ -454,14 +451,7 @@ func (i *LDAP) GetUsers(ctx context.Context, queryParam url.Values) ([]*libregra if err != nil { return nil, err } - if len(userGroups) > 0 { - expand := ldap.EscapeFilter(queryParam.Get("expand")) - if expand == "" { - expand = "false" - } - groups := i.groupsFromLDAPEntries(userGroups) - u.MemberOf = groups - } + u.MemberOf = i.groupsFromLDAPEntries(userGroups) } users = append(users, u) } diff --git a/services/graph/pkg/identity/ldap_group.go b/services/graph/pkg/identity/ldap_group.go index 49a0d1e639..70027ed1f6 100644 --- a/services/graph/pkg/identity/ldap_group.go +++ b/services/graph/pkg/identity/ldap_group.go @@ -41,14 +41,13 @@ func (i *LDAP) GetGroup(ctx context.Context, nameOrID string, queryParam url.Val if err != nil { return nil, err } + g.Members = make([]libregraph.User, 0, len(members)) if len(members) > 0 { - m := make([]libregraph.User, 0, len(members)) for _, ue := range members { if u := i.createUserModelFromLDAP(ue); u != nil { - m = append(m, *u) + g.Members = append(g.Members, *u) } } - g.Members = m } } return g, nil @@ -120,14 +119,13 @@ func (i *LDAP) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregr if err != nil { return nil, err } + g.Members = make([]libregraph.User, 0, len(members)) if len(members) > 0 { - m := make([]libregraph.User, 0, len(members)) for _, ue := range members { if u := i.createUserModelFromLDAP(ue); u != nil { - m = append(m, *u) + g.Members = append(g.Members, *u) } } - g.Members = m } } groups = append(groups, g) diff --git a/services/graph/pkg/service/v0/drives.go b/services/graph/pkg/service/v0/drives.go index c106d2ed78..29a4e27ad8 100644 --- a/services/graph/pkg/service/v0/drives.go +++ b/services/graph/pkg/service/v0/drives.go @@ -478,8 +478,10 @@ func (g Graph) formatDrives(ctx context.Context, baseURL *url.URL, storageSpaces // can't access disabled space if utils.ReadPlainFromOpaque(storageSpace.Opaque, "trashed") != "trashed" { res.Special = g.getExtendedSpaceProperties(ctx, baseURL, storageSpace) - res.Quota, err = g.getDriveQuota(ctx, storageSpace) + quota, err := g.getDriveQuota(ctx, storageSpace) + res.Quota = "a if err != nil { + //logger.Debug().Err(err).Interface("id", sp.Id).Msg("error calling get quota on drive") return nil, err } } @@ -718,6 +720,8 @@ func (g Graph) cs3StorageSpaceToDrive(ctx context.Context, baseURL *url.URL, spa lastModified := cs3TimestampToTime(space.Mtime) drive.LastModifiedDateTime = &lastModified } + + drive.Quota = &libregraph.Quota{} if space.Quota != nil { var t int64 if space.Quota.QuotaMaxBytes > math.MaxInt64 { @@ -725,15 +729,13 @@ func (g Graph) cs3StorageSpaceToDrive(ctx context.Context, baseURL *url.URL, spa } else { t = int64(space.Quota.QuotaMaxBytes) } - drive.Quota = &libregraph.Quota{ - Total: &t, - } + drive.Quota.Total = &t } return drive, nil } -func (g Graph) getDriveQuota(ctx context.Context, space *storageprovider.StorageSpace) (*libregraph.Quota, error) { +func (g Graph) getDriveQuota(ctx context.Context, space *storageprovider.StorageSpace) (libregraph.Quota, error) { logger := g.logger.SubloggerWithRequestID(ctx) client := g.GetGatewayClient() @@ -747,13 +749,13 @@ func (g Graph) getDriveQuota(ctx context.Context, space *storageprovider.Storage switch { case err != nil: logger.Error().Err(err).Interface("ref", req.Ref).Msg("could not call GetQuota: transport error") - return nil, nil + return libregraph.Quota{}, nil case res.GetStatus().GetCode() == cs3rpc.Code_CODE_UNIMPLEMENTED: logger.Debug().Msg("get quota is not implemented on the storage driver") - return nil, nil + return libregraph.Quota{}, nil case res.GetStatus().GetCode() != cs3rpc.Code_CODE_OK: logger.Debug().Str("grpc", res.GetStatus().GetMessage()).Msg("error sending get quota grpc request") - return nil, errors.New(res.GetStatus().GetMessage()) + return libregraph.Quota{}, errors.New(res.GetStatus().GetMessage()) } var remaining int64 @@ -783,7 +785,7 @@ func (g Graph) getDriveQuota(ctx context.Context, space *storageprovider.Storage state := calculateQuotaState(t, used) qta.State = &state - return &qta, nil + return qta, nil } func calculateQuotaState(total int64, used int64) (state string) { diff --git a/services/graph/pkg/service/v0/drives_test.go b/services/graph/pkg/service/v0/drives_test.go index 28bcb7cf56..9abacf8403 100644 --- a/services/graph/pkg/service/v0/drives_test.go +++ b/services/graph/pkg/service/v0/drives_test.go @@ -111,7 +111,7 @@ var sortTests = []sortTest{ } func drive(ID string, dType string, name string, lastModified *time.Time) *libregraph.Drive { - return &libregraph.Drive{Id: libregraph.PtrString(ID), DriveType: libregraph.PtrString(dType), Name: name, LastModifiedDateTime: lastModified} + return &libregraph.Drive{Id: libregraph.PtrString(ID), DriveType: libregraph.PtrString(dType), Name: name, LastModifiedDateTime: lastModified, Quota: &libregraph.Quota{}} } // TestSort tests the available orderby queries diff --git a/services/graph/pkg/service/v0/educationuser.go b/services/graph/pkg/service/v0/educationuser.go index 8a5cdb3c72..1ce9dae2a2 100644 --- a/services/graph/pkg/service/v0/educationuser.go +++ b/services/graph/pkg/service/v0/educationuser.go @@ -257,7 +257,7 @@ func (g Graph) GetEducationUser(w http.ResponseWriter, r *http.Request) { if err != nil { logger.Debug().Err(err).Interface("id", sp.Id).Msg("error calling get quota on drive") } - d.Quota = quota + d.Quota = "a if slices.Contains(sel, "drive") || slices.Contains(exp, "drive") { if *d.DriveType == _spaceTypePersonal { user.Drive = d diff --git a/services/graph/pkg/service/v0/graph_test.go b/services/graph/pkg/service/v0/graph_test.go index 82a896c282..e7e6d86fe5 100644 --- a/services/graph/pkg/service/v0/graph_test.go +++ b/services/graph/pkg/service/v0/graph_test.go @@ -143,6 +143,7 @@ var _ = Describe("Graph", func() { "driveType":"aspacetype", "id":"pro-1$sameID", "name":"aspacename", + "quota": {}, "root":{ "id":"pro-1$sameID", "webDavUrl":"https://localhost:9200/dav/spaces/pro-1$sameID" @@ -213,6 +214,7 @@ var _ = Describe("Graph", func() { "driveType":"aspacetype", "id":"pro-1$asameID", "name":"aspacename", + "quota": {}, "root":{ "eTag":"101112131415", "id":"pro-1$asameID", @@ -225,6 +227,7 @@ var _ = Describe("Graph", func() { "driveType":"bspacetype", "id":"pro-1$bsameID", "name":"bspacename", + "quota": {}, "root":{ "eTag":"123456789", "id":"pro-1$bsameID", diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 201d64af17..c0bddc7775 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -1,6 +1,7 @@ package svc import ( + "context" "encoding/json" "errors" "fmt" @@ -57,13 +58,15 @@ func (g Graph) GetMe(w http.ResponseWriter, r *http.Request) { } return } + if me.MemberOf == nil { + me.MemberOf = []libregraph.Group{} + } } // expand appRoleAssignments if requested if slices.Contains(exp, "appRoleAssignments") { - lrar, err := g.roleService.ListRoleAssignments(r.Context(), &settings.ListRoleAssignmentsRequest{ - AccountUuid: me.GetId(), - }) + var err error + me.AppRoleAssignments, err = g.fetchAppRoleAssignments(r.Context(), me.GetId()) if err != nil { logger.Debug().Err(err).Str("userid", me.GetId()).Msg("could not get appRoleAssignments for self") var errcode errorcode.Error @@ -74,18 +77,27 @@ func (g Graph) GetMe(w http.ResponseWriter, r *http.Request) { } return } - - values := make([]libregraph.AppRoleAssignment, 0, len(lrar.GetAssignments())) - for _, assignment := range lrar.GetAssignments() { - values = append(values, g.assignmentToAppRoleAssignment(assignment)) - } - me.AppRoleAssignments = values } render.Status(r, http.StatusOK) render.JSON(w, r, me) } +func (g Graph) fetchAppRoleAssignments(ctx context.Context, accountuuid string) ([]libregraph.AppRoleAssignment, error) { + lrar, err := g.roleService.ListRoleAssignments(ctx, &settings.ListRoleAssignmentsRequest{ + AccountUuid: accountuuid, + }) + if err != nil { + return []libregraph.AppRoleAssignment{}, err + } + + values := make([]libregraph.AppRoleAssignment, 0, len(lrar.Assignments)) + for _, assignment := range lrar.GetAssignments() { + values = append(values, g.assignmentToAppRoleAssignment(assignment)) + } + return values, nil +} + // GetUsers implements the Service interface. func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) { logger := g.logger.SubloggerWithRequestID(r.Context()) @@ -113,21 +125,21 @@ func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) { // expand appRoleAssignments if requested exp := strings.Split(r.URL.Query().Get("$expand"), ",") - if slices.Contains(exp, "appRoleAssignments") { - for _, u := range users { - lrar, err := g.roleService.ListRoleAssignments(r.Context(), &settings.ListRoleAssignmentsRequest{ - AccountUuid: u.GetId(), - }) + expandAppRoleAssignments := slices.Contains(exp, "appRoleAssignments") + expandMemberOf := slices.Contains(exp, "memberOf") + for _, u := range users { + if expandAppRoleAssignments { + u.AppRoleAssignments, err = g.fetchAppRoleAssignments(r.Context(), u.GetId()) if err != nil { + // TODO I think we should not continue here, see http://docs.oasis-open.org/odata/odata/v4.01/odata-v4.01-part1-protocol.html#sec_SystemQueryOptionexpand + // > The $expand system query option indicates the related entities and stream values that MUST be represented inline. The service MUST return the specified content, and MAY choose to return additional information. logger.Debug().Err(err).Str("userid", u.GetId()).Msg("could not get appRoleAssignments when listing user") - continue } - - values := make([]libregraph.AppRoleAssignment, 0, len(lrar.GetAssignments())) - for _, assignment := range lrar.GetAssignments() { - values = append(values, g.assignmentToAppRoleAssignment(assignment)) + } + if expandMemberOf { + if u.MemberOf == nil { + u.MemberOf = []libregraph.Group{} } - u.AppRoleAssignments = values } } @@ -307,7 +319,7 @@ func (g Graph) GetUser(w http.ResponseWriter, r *http.Request) { if err != nil { logger.Debug().Err(err).Interface("id", sp.Id).Msg("error calling get quota on drive") } - d.Quota = quota + d.Quota = "a if slices.Contains(sel, "drive") || slices.Contains(exp, "drive") { if *d.DriveType == "personal" { user.Drive = d @@ -320,10 +332,7 @@ func (g Graph) GetUser(w http.ResponseWriter, r *http.Request) { } // expand appRoleAssignments if requested if slices.Contains(exp, "appRoleAssignments") { - - lrar, err := g.roleService.ListRoleAssignments(r.Context(), &settings.ListRoleAssignmentsRequest{ - AccountUuid: user.GetId(), - }) + user.AppRoleAssignments, err = g.fetchAppRoleAssignments(r.Context(), user.GetId()) if err != nil { logger.Debug().Err(err).Str("userid", user.GetId()).Msg("could not get appRoleAssignments for user") var errcode errorcode.Error @@ -334,12 +343,6 @@ func (g Graph) GetUser(w http.ResponseWriter, r *http.Request) { } return } - - values := make([]libregraph.AppRoleAssignment, 0, len(lrar.GetAssignments())) - for _, assignment := range lrar.GetAssignments() { - values = append(values, g.assignmentToAppRoleAssignment(assignment)) - } - user.AppRoleAssignments = values } render.Status(r, http.StatusOK)