Merge pull request #2825 from owncloud/fix-create-group-no-name

[full-ci] Bugfix: Disallow creation of a group with empty name via the OCS api

changelog/unreleased/fix-create-group-without-name.md

@@ -0,0 +1,10 @@
+Bugfix: Disallow creation of a group with empty name via the OCS api
+
+We've fixed the behavior for group creation on the OCS api, where it was
+possible to create a group with an empty name. This was is not possible
+on oC10 and is therefore also forbidden on oCIS to keep compatibility.
+This PR forbids the creation and also ensures the correct status code
+for both OCS v1 and OCS v2 apis.
+
+https://github.com/owncloud/ocis/pull/2825
+https://github.com/owncloud/ocis/issues/2823

ocs/pkg/middleware/requireadmin.go

@@ -14,13 +14,19 @@ import (
 func RequireAdmin(opts ...Option) func(next http.Handler) http.Handler {
 	opt := newOptions(opts...)
 
+	mustRender := func(w http.ResponseWriter, r *http.Request, renderer render.Renderer) {
+		if err := render.Render(w, r, renderer); err != nil {
+			opt.Logger.Err(err).Msgf("failed to write response for ocs request %s on %s", r.Method, r.URL)
+		}
+	}
+
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 
 			// get roles from context
 			roleIDs, ok := roles.ReadRoleIDsFromContext(r.Context())
 			if !ok {
-				mustNotFail(render.Render(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized")))
+				mustRender(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized"))
 				return
 			}
 
@@ -30,13 +36,7 @@ func RequireAdmin(opts ...Option) func(next http.Handler) http.Handler {
 				return
 			}
 
-			mustNotFail(render.Render(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized")))
+			mustRender(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized"))
 		})
 	}
 }
-
-func mustNotFail(err error) {
-	if err != nil {
-		panic(err)
-	}
-}

ocs/pkg/middleware/requireselforadmin.go

@@ -17,22 +17,28 @@ import (
 func RequireSelfOrAdmin(opts ...Option) func(next http.Handler) http.Handler {
 	opt := newOptions(opts...)
 
+	mustRender := func(w http.ResponseWriter, r *http.Request, renderer render.Renderer) {
+		if err := render.Render(w, r, renderer); err != nil {
+			opt.Logger.Err(err).Msgf("failed to write response for ocs request %s on %s", r.Method, r.URL)
+		}
+	}
+
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 
 			u, ok := revactx.ContextGetUser(r.Context())
 			if !ok {
-				mustNotFail(render.Render(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized")))
+				mustRender(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized"))
 				return
 			}
 			if u.Id == nil || u.Id.OpaqueId == "" {
-				mustNotFail(render.Render(w, r, response.ErrRender(data.MetaBadRequest.StatusCode, "user is missing an id")))
+				mustRender(w, r, response.ErrRender(data.MetaBadRequest.StatusCode, "user is missing an id"))
 				return
 			}
 			// get roles from context
 			roleIDs, ok := roles.ReadRoleIDsFromContext(r.Context())
 			if !ok {
-				mustNotFail(render.Render(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized")))
+				mustRender(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized"))
 				return
 			}
 
@@ -47,7 +53,7 @@ func RequireSelfOrAdmin(opts ...Option) func(next http.Handler) http.Handler {
 				userid := chi.URLParam(r, "userid")
 				var err error
 				if userid, err = url.PathUnescape(userid); err != nil {
-					mustNotFail(render.Render(w, r, response.ErrRender(data.MetaBadRequest.StatusCode, "malformed username")))
+					mustRender(w, r, response.ErrRender(data.MetaBadRequest.StatusCode, "malformed username"))
 				}
 
 				if userid == "" || userid == u.Id.OpaqueId || userid == u.Username {
@@ -56,7 +62,7 @@ func RequireSelfOrAdmin(opts ...Option) func(next http.Handler) http.Handler {
 				}
 			}
 
-			mustNotFail(render.Render(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized")))
+			mustRender(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized"))
 
 		})
 	}

ocs/pkg/middleware/requireuser.go

@@ -10,18 +10,25 @@ import (
 )
 
 // RequireUser middleware is used to require a user in context
-func RequireUser() func(next http.Handler) http.Handler {
+func RequireUser(opts ...Option) func(next http.Handler) http.Handler {
+	opt := newOptions(opts...)
+
+	mustRender := func(w http.ResponseWriter, r *http.Request, renderer render.Renderer) {
+		if err := render.Render(w, r, renderer); err != nil {
+			opt.Logger.Err(err).Msgf("failed to write response for ocs request %s on %s", r.Method, r.URL)
+		}
+	}
 
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 
 			u, ok := revactx.ContextGetUser(r.Context())
 			if !ok {
-				mustNotFail(render.Render(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized")))
+				mustRender(w, r, response.ErrRender(data.MetaUnauthorized.StatusCode, "Unauthorized"))
 				return
 			}
 			if u.Id == nil || u.Id.OpaqueId == "" {
-				mustNotFail(render.Render(w, r, response.ErrRender(data.MetaBadRequest.StatusCode, "user is missing an id")))
+				mustRender(w, r, response.ErrRender(data.MetaBadRequest.StatusCode, "user is missing an id"))
 				return
 			}
 

ocs/pkg/service/v0/config.go

@@ -3,18 +3,17 @@ package svc
 import (
 	"net/http"
 
-	"github.com/go-chi/render"
 	"github.com/owncloud/ocis/ocs/pkg/service/v0/data"
 	"github.com/owncloud/ocis/ocs/pkg/service/v0/response"
 )
 
 // GetConfig renders the ocs config endpoint
 func (o Ocs) GetConfig(w http.ResponseWriter, r *http.Request) {
-	mustNotFail(render.Render(w, r, response.DataRender(&data.ConfigData{
+	o.mustRender(w, r, response.DataRender(&data.ConfigData{
 		Version: "1.7",  // TODO get from env
 		Website: "ocis", // TODO get from env
 		Host:    "",     // TODO get from FRONTEND config
 		Contact: "",     // TODO get from env
 		SSL:     "true", // TODO get from env
-	})))
+	}))
 }