Merge pull request #3284 from rhafer/ocisissue/4703

prevent nil pointer exceptions when requesting user or groups
cs3org · Sep 29, 2022 · b1e0403 · b1e0403
2 parents 5e157d8 + 1d94178
commit b1e0403
Show file tree

Hide file tree

Showing 13 changed files with 56 additions and 27 deletions.
diff --git a/changelog/unreleased/users-prevent-nil-pointer.md b/changelog/unreleased/users-prevent-nil-pointer.md
@@ -0,0 +1,6 @@
+Bugfix: prevent nil pointer when requesting user
+
+We added additional nil pointer checks in the user and groups providers.
+
+https://github.com/cs3org/reva/pull/3284
+https://github.com/owncloud/ocis/issues/4703
diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go
@@ -245,7 +245,7 @@ func (s *svc) ListStorageSpaces(ctx context.Context, req *provider.ListStorageSp
 			filters["user_id"] = f.GetUser().GetOpaqueId()
 		default:
 			return &provider.ListStorageSpacesResponse{
-				Status: status.NewInvalidArg(ctx, fmt.Sprintf("unknown filter %v", f.Type)),
+				Status: status.NewInvalid(ctx, fmt.Sprintf("unknown filter %v", f.Type)),
 			}, nil
 		}
 	}

diff --git a/internal/grpc/services/gateway/usershareprovider.go b/internal/grpc/services/gateway/usershareprovider.go
@@ -188,19 +188,19 @@ func (s *svc) UpdateReceivedShare(ctx context.Context, req *collaboration.Update
 	switch {
 	case req.GetShare() == nil:
 		return &collaboration.UpdateReceivedShareResponse{
-			Status: status.NewInvalidArg(ctx, "updating requires a received share object"),
+			Status: status.NewInvalid(ctx, "updating requires a received share object"),
 		}, nil
 	case req.GetShare().GetShare() == nil:
 		return &collaboration.UpdateReceivedShareResponse{
-			Status: status.NewInvalidArg(ctx, "share missing"),
+			Status: status.NewInvalid(ctx, "share missing"),
 		}, nil
 	case req.GetShare().GetShare().GetId() == nil:
 		return &collaboration.UpdateReceivedShareResponse{
-			Status: status.NewInvalidArg(ctx, "share id missing"),
+			Status: status.NewInvalid(ctx, "share id missing"),
 		}, nil
 	case req.GetShare().GetShare().GetId().GetOpaqueId() == "":
 		return &collaboration.UpdateReceivedShareResponse{
-			Status: status.NewInvalidArg(ctx, "share id empty"),
+			Status: status.NewInvalid(ctx, "share id empty"),
 		}, nil
 	}
 

diff --git a/internal/grpc/services/groupprovider/groupprovider.go b/internal/grpc/services/groupprovider/groupprovider.go
@@ -101,6 +101,12 @@ func (s *service) Register(ss *grpc.Server) {
 }
 
 func (s *service) GetGroup(ctx context.Context, req *grouppb.GetGroupRequest) (*grouppb.GetGroupResponse, error) {
+	if req.GroupId == nil {
+		res := &grouppb.GetGroupResponse{
+			Status: status.NewInvalid(ctx, "groupid missing"),
+		}
+		return res, nil
+	}
 	group, err := s.groupmgr.GetGroup(ctx, req.GroupId, req.SkipFetchingMembers)
 	if err != nil {
 		res := &grouppb.GetGroupResponse{}
@@ -156,6 +162,12 @@ func (s *service) FindGroups(ctx context.Context, req *grouppb.FindGroupsRequest
 }
 
 func (s *service) GetMembers(ctx context.Context, req *grouppb.GetMembersRequest) (*grouppb.GetMembersResponse, error) {
+	if req.GroupId == nil {
+		res := &grouppb.GetMembersResponse{
+			Status: status.NewInvalid(ctx, "groupid missing"),
+		}
+		return res, nil
+	}
 	members, err := s.groupmgr.GetMembers(ctx, req.GroupId)
 	if err != nil {
 		return &grouppb.GetMembersResponse{
@@ -170,6 +182,12 @@ func (s *service) GetMembers(ctx context.Context, req *grouppb.GetMembersRequest
 }
 
 func (s *service) HasMember(ctx context.Context, req *grouppb.HasMemberRequest) (*grouppb.HasMemberResponse, error) {
+	if req.GroupId == nil || req.UserId == nil {
+		res := &grouppb.HasMemberResponse{
+			Status: status.NewInvalid(ctx, "groupid or userid missing"),
+		}
+		return res, nil
+	}
 	ok, err := s.groupmgr.HasMember(ctx, req.GroupId, req.UserId)
 	if err != nil {
 		return &grouppb.HasMemberResponse{

diff --git a/internal/grpc/services/publicshareprovider/publicshareprovider.go b/internal/grpc/services/publicshareprovider/publicshareprovider.go
@@ -136,7 +136,7 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS
 
 	if !s.isPathAllowed(req.ResourceInfo.Path) {
 		return &link.CreatePublicShareResponse{
-			Status: status.NewInvalidArg(ctx, "share creation is not allowed for the specified path"),
+			Status: status.NewInvalid(ctx, "share creation is not allowed for the specified path"),
 		}, nil
 	}
 

diff --git a/internal/grpc/services/publicstorageprovider/publicstorageprovider.go b/internal/grpc/services/publicstorageprovider/publicstorageprovider.go
@@ -652,7 +652,7 @@ func (s *service) Move(ctx context.Context, req *provider.MoveRequest) (*provide
 
 	if tknSource != tknDest {
 		return &provider.MoveResponse{
-			Status: status.NewInvalidArg(ctx, "Source and destination token must be the same"),
+			Status: status.NewInvalid(ctx, "Source and destination token must be the same"),
 		}, nil
 	}
 

diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go
@@ -919,7 +919,7 @@ func (s *service) resolveAcceptedShare(ctx context.Context, ref *provider.Refere
 		ref.Path = "."
 	}
 	if !utils.IsRelativeReference(ref) {
-		return nil, status.NewInvalidArg(ctx, "sharesstorageprovider: can only handle relative references"), nil
+		return nil, status.NewInvalid(ctx, "sharesstorageprovider: can only handle relative references"), nil
 	}
 
 	if ref.ResourceId.SpaceId != utils.ShareStorageSpaceID {

diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go
@@ -383,7 +383,7 @@ func (s *service) InitiateFileUpload(ctx context.Context, req *provider.Initiate
 		case errtypes.IsNotFound:
 			st = status.NewNotFound(ctx, "path not found when initiating upload")
 		case errtypes.IsBadRequest, errtypes.IsChecksumMismatch:
-			st = status.NewInvalidArg(ctx, err.Error())
+			st = status.NewInvalid(ctx, err.Error())
 			// TODO TUS uses a custom ChecksumMismatch 460 http status which is in an unassigned range in
 			// https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml
 			// maybe 409 conflict is good enough
@@ -582,7 +582,7 @@ func (s *service) DeleteStorageSpace(ctx context.Context, req *provider.DeleteSt
 		case errtypes.PermissionDenied:
 			st = status.NewPermissionDenied(ctx, err, "permission denied")
 		case errtypes.BadRequest:
-			st = status.NewInvalidArg(ctx, err.Error())
+			st = status.NewInvalid(ctx, err.Error())
 		default:
 			st = status.NewInternal(ctx, "error deleting space: "+req.Id.String())
 		}
@@ -1088,7 +1088,7 @@ func (s *service) CreateReference(ctx context.Context, req *provider.CreateRefer
 	if err != nil {
 		log.Error().Err(err).Msg("invalid target uri")
 		return &provider.CreateReferenceResponse{
-			Status: status.NewInvalidArg(ctx, "target uri is invalid: "+err.Error()),
+			Status: status.NewInvalid(ctx, "target uri is invalid: "+err.Error()),
 		}, nil
 	}
 

diff --git a/internal/grpc/services/storageregistry/storageregistry.go b/internal/grpc/services/storageregistry/storageregistry.go
@@ -124,7 +124,7 @@ func (s *service) GetStorageProviders(ctx context.Context, req *registrypb.GetSt
 	space, err := decodeSpace(req.Opaque)
 	if err != nil {
 		return &registrypb.GetStorageProvidersResponse{
-			Status: status.NewInvalidArg(ctx, err.Error()),
+			Status: status.NewInvalid(ctx, err.Error()),
 		}, nil
 	}
 	p, err := s.reg.GetProvider(ctx, space)

diff --git a/internal/grpc/services/userprovider/userprovider.go b/internal/grpc/services/userprovider/userprovider.go
@@ -126,6 +126,13 @@ func (s *service) Register(ss *grpc.Server) {
 }
 
 func (s *service) GetUser(ctx context.Context, req *userpb.GetUserRequest) (*userpb.GetUserResponse, error) {
+	if req.UserId == nil {
+		res := &userpb.GetUserResponse{
+			Status: status.NewInvalid(ctx, "userid missing"),
+		}
+		return res, nil
+	}
+
 	user, err := s.usermgr.GetUser(ctx, req.UserId, req.SkipFetchingUserGroups)
 	if err != nil {
 		res := &userpb.GetUserResponse{}
@@ -186,6 +193,12 @@ func (s *service) FindUsers(ctx context.Context, req *userpb.FindUsersRequest) (
 
 func (s *service) GetUserGroups(ctx context.Context, req *userpb.GetUserGroupsRequest) (*userpb.GetUserGroupsResponse, error) {
 	log := appctx.GetLogger(ctx)
+	if req.UserId == nil {
+		res := &userpb.GetUserGroupsResponse{
+			Status: status.NewInvalid(ctx, "userid missing"),
+		}
+		return res, nil
+	}
 	groups, err := s.usermgr.GetUserGroups(ctx, req.UserId)
 	if err != nil {
 		log.Warn().Err(err).Interface("userid", req.UserId).Msg("error getting user groups")

diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go
@@ -143,7 +143,7 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar
 
 	if !s.isPathAllowed(req.ResourceInfo.Path) {
 		return &collaboration.CreateShareResponse{
-			Status: status.NewInvalidArg(ctx, "share creation is not allowed for the specified path"),
+			Status: status.NewInvalid(ctx, "share creation is not allowed for the specified path"),
 		}, nil
 	}
 
@@ -266,22 +266,22 @@ func (s *service) UpdateReceivedShare(ctx context.Context, req *collaboration.Up
 
 	if req.Share == nil {
 		return &collaboration.UpdateReceivedShareResponse{
-			Status: status.NewInvalidArg(ctx, "updating requires a received share object"),
+			Status: status.NewInvalid(ctx, "updating requires a received share object"),
 		}, nil
 	}
 	if req.Share.Share == nil {
 		return &collaboration.UpdateReceivedShareResponse{
-			Status: status.NewInvalidArg(ctx, "share missing"),
+			Status: status.NewInvalid(ctx, "share missing"),
 		}, nil
 	}
 	if req.Share.Share.Id == nil {
 		return &collaboration.UpdateReceivedShareResponse{
-			Status: status.NewInvalidArg(ctx, "share id missing"),
+			Status: status.NewInvalid(ctx, "share id missing"),
 		}, nil
 	}
 	if req.Share.Share.Id.OpaqueId == "" {
 		return &collaboration.UpdateReceivedShareResponse{
-			Status: status.NewInvalidArg(ctx, "share id empty"),
+			Status: status.NewInvalid(ctx, "share id empty"),
 		}, nil
 	}
 

diff --git a/pkg/rgrpc/status/status.go b/pkg/rgrpc/status/status.go
@@ -131,14 +131,6 @@ func NewAlreadyExists(ctx context.Context, err error, msg string) *rpc.Status {
 	}
 }
 
-// NewInvalidArg returns a Status with CODE_INVALID_ARGUMENT.
-func NewInvalidArg(ctx context.Context, msg string) *rpc.Status {
-	return &rpc.Status{Code: rpc.Code_CODE_INVALID_ARGUMENT,
-		Message: msg,
-		Trace:   getTrace(ctx),
-	}
-}
-
 // NewConflict returns a Status with Code_CODE_ABORTED.
 //
 // Deprecated: NewConflict exists for historical compatibility
@@ -177,7 +169,7 @@ func NewStatusFromErrType(ctx context.Context, msg string, err error) *rpc.Statu
 	case errtypes.IsNotSupported:
 		return NewUnimplemented(ctx, err, msg+":"+err.Error())
 	case errtypes.BadRequest:
-		return NewInvalidArg(ctx, msg+":"+err.Error())
+		return NewInvalid(ctx, msg+":"+err.Error())
 	}
 
 	// map GRPC status codes coming from the auth middleware

diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go
@@ -663,7 +663,7 @@ func (fs *Decomposedfs) DeleteStorageSpace(ctx context.Context, req *provider.De
 
 	if purge {
 		if !n.IsDisabled() {
-			return errtypes.NewErrtypeFromStatus(status.NewInvalidArg(ctx, "can't purge enabled space"))
+			return errtypes.NewErrtypeFromStatus(status.NewInvalid(ctx, "can't purge enabled space"))
 		}
 
 		spaceType, err := n.GetMetadata(xattrs.SpaceTypeAttr)