From ac49348b417111753621275922e2d36ce16d1082 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 13 Jul 2021 14:29:26 +0200 Subject: [PATCH 1/7] fix 215, 216 - first draft --- proxy/pkg/middleware/account_resolver.go | 3 +- proxy/pkg/middleware/basic_auth.go | 90 +++++++++++++++++++++++- 2 files changed, 90 insertions(+), 3 deletions(-) diff --git a/proxy/pkg/middleware/account_resolver.go b/proxy/pkg/middleware/account_resolver.go index 6aa829c49..f7379eda4 100644 --- a/proxy/pkg/middleware/account_resolver.go +++ b/proxy/pkg/middleware/account_resolver.go @@ -1,9 +1,10 @@ package middleware import ( + "net/http" + "github.com/cs3org/reva/pkg/auth/scope" "github.com/owncloud/ocis/proxy/pkg/user/backend" - "net/http" tokenPkg "github.com/cs3org/reva/pkg/token" "github.com/cs3org/reva/pkg/token/manager/jwt" diff --git a/proxy/pkg/middleware/basic_auth.go b/proxy/pkg/middleware/basic_auth.go index df710c6e4..3a63b8684 100644 --- a/proxy/pkg/middleware/basic_auth.go +++ b/proxy/pkg/middleware/basic_auth.go @@ -1,12 +1,14 @@ package middleware import ( + "encoding/xml" "fmt" + "net/http" + "strings" + "github.com/owncloud/ocis/ocis-pkg/log" "github.com/owncloud/ocis/ocis-pkg/oidc" "github.com/owncloud/ocis/proxy/pkg/user/backend" - "net/http" - "strings" ) const publicFilesEndpoint = "/remote.php/dav/public-files/" @@ -61,7 +63,18 @@ func BasicAuth(optionSetters ...Option) func(next http.Handler) http.Handler { writeSupportedAuthenticateHeader(w, req) } + // if the request is a PROPFIND return a WebDAV error code. + // TODO: The proxy has to be smart enough to detect when a request is directed towards a webdav server + // and react accordingly. + w.WriteHeader(http.StatusUnauthorized) + + b, err := Marshal(exception{ + code: SabredavPermissionDenied, + message: "Authentication error", + }) + + HandleWebdavError(w, b, err) return } @@ -93,3 +106,76 @@ func (m basicAuth) isBasicAuth(req *http.Request) bool { login, password, ok := req.BasicAuth() return m.enabled && ok && login != "" && password != "" } + +type code int + +const ( + // SabredavBadRequest maps to HTTP 400 + SabredavBadRequest code = iota + // SabredavMethodNotAllowed maps to HTTP 405 + SabredavMethodNotAllowed + // SabredavNotAuthenticated maps to HTTP 401 + SabredavNotAuthenticated + // SabredavPreconditionFailed maps to HTTP 412 + SabredavPreconditionFailed + // SabredavPermissionDenied maps to HTTP 403 + SabredavPermissionDenied + // SabredavNotFound maps to HTTP 404 + SabredavNotFound + // SabredavConflict maps to HTTP 409 + SabredavConflict +) + +var ( + codesEnum = []string{ + "Sabre\\DAV\\Exception\\BadRequest", + "Sabre\\DAV\\Exception\\MethodNotAllowed", + "Sabre\\DAV\\Exception\\NotAuthenticated", + "Sabre\\DAV\\Exception\\PreconditionFailed", + "Sabre\\DAV\\Exception\\PermissionDenied", + "Sabre\\DAV\\Exception\\NotFound", + "Sabre\\DAV\\Exception\\Conflict", + } +) + +type exception struct { + code code + message string + header string +} + +// Marshal just calls the xml marshaller for a given exception. +func Marshal(e exception) ([]byte, error) { + xmlstring, err := xml.Marshal(&errorXML{ + Xmlnsd: "DAV", + Xmlnss: "http://sabredav.org/ns", + Exception: codesEnum[e.code], + Message: e.message, + Header: e.header, + }) + if err != nil { + return []byte(""), err + } + return []byte(xml.Header + string(xmlstring)), err +} + +// http://www.webdav.org/specs/rfc4918.html#ELEMENT_error +type errorXML struct { + XMLName xml.Name `xml:"d:error"` + Xmlnsd string `xml:"xmlns:d,attr"` + Xmlnss string `xml:"xmlns:s,attr"` + Exception string `xml:"s:exception"` + Message string `xml:"s:message"` + InnerXML []byte `xml:",innerxml"` + Header string `xml:"s:header,omitempty"` +} + +func HandleWebdavError(w http.ResponseWriter, b []byte, err error) { + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + _, err = w.Write(b) + if err != nil { + } +} From bfdcc0180c73e6d0f14fa129d236f8d18d77d255 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 13 Jul 2021 14:33:24 +0200 Subject: [PATCH 2/7] fix 230, 231 - first draft --- proxy/pkg/middleware/basic_auth.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/proxy/pkg/middleware/basic_auth.go b/proxy/pkg/middleware/basic_auth.go index 3a63b8684..83e562da5 100644 --- a/proxy/pkg/middleware/basic_auth.go +++ b/proxy/pkg/middleware/basic_auth.go @@ -103,8 +103,8 @@ func (m basicAuth) isPublicLink(req *http.Request) bool { } func (m basicAuth) isBasicAuth(req *http.Request) bool { - login, password, ok := req.BasicAuth() - return m.enabled && ok && login != "" && password != "" + _, _, ok := req.BasicAuth() + return m.enabled && ok } type code int From 7ab586b2abe24b42abff21d237f730a345532bc4 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 13 Jul 2021 15:38:44 +0200 Subject: [PATCH 3/7] proxy is now aware of webdav responses --- ocs/pkg/server/http/svc_test.go | 2 +- ocs/pkg/service/v0/users.go | 6 +- proxy/pkg/middleware/basic_auth.go | 90 ++++-------------------------- proxy/pkg/webdav/response.go | 79 ++++++++++++++++++++++++++ proxy/pkg/webdav/webdav.go | 18 ++++++ 5 files changed, 112 insertions(+), 83 deletions(-) create mode 100644 proxy/pkg/webdav/response.go create mode 100644 proxy/pkg/webdav/webdav.go diff --git a/ocs/pkg/server/http/svc_test.go b/ocs/pkg/server/http/svc_test.go index 9a284d0c1..428550352 100644 --- a/ocs/pkg/server/http/svc_test.go +++ b/ocs/pkg/server/http/svc_test.go @@ -650,7 +650,7 @@ func mintToken(ctx context.Context, su *User, roleIds []string) (token string, e }, }, }, - Groups: []string{}, + Groups: []string{}, UidNumber: int64(su.UIDNumber), GidNumber: int64(su.GIDNumber), } diff --git a/ocs/pkg/service/v0/users.go b/ocs/pkg/service/v0/users.go index 3cabff977..eef8ed535 100644 --- a/ocs/pkg/service/v0/users.go +++ b/ocs/pkg/service/v0/users.go @@ -486,9 +486,9 @@ func (o Ocs) mintTokenForUser(ctx context.Context, account *accounts.Account) (s OpaqueId: account.Id, Idp: o.config.IdentityManagement.Address, }, - Groups: []string{}, - UidNumber: account.UidNumber, - GidNumber: account.GidNumber, + Groups: []string{}, + UidNumber: account.UidNumber, + GidNumber: account.GidNumber, } s, err := scope.GetOwnerScope() if err != nil { diff --git a/proxy/pkg/middleware/basic_auth.go b/proxy/pkg/middleware/basic_auth.go index 83e562da5..f7a107f6a 100644 --- a/proxy/pkg/middleware/basic_auth.go +++ b/proxy/pkg/middleware/basic_auth.go @@ -1,7 +1,6 @@ package middleware import ( - "encoding/xml" "fmt" "net/http" "strings" @@ -9,6 +8,7 @@ import ( "github.com/owncloud/ocis/ocis-pkg/log" "github.com/owncloud/ocis/ocis-pkg/oidc" "github.com/owncloud/ocis/proxy/pkg/user/backend" + "github.com/owncloud/ocis/proxy/pkg/webdav" ) const publicFilesEndpoint = "/remote.php/dav/public-files/" @@ -69,12 +69,17 @@ func BasicAuth(optionSetters ...Option) func(next http.Handler) http.Handler { w.WriteHeader(http.StatusUnauthorized) - b, err := Marshal(exception{ - code: SabredavPermissionDenied, - message: "Authentication error", - }) + if webdav.IsWebdavRequest(req) { + b, err := webdav.Marshal(webdav.Exception{ + Code: webdav.SabredavPermissionDenied, + Message: "Authentication error", + }) - HandleWebdavError(w, b, err) + webdav.HandleWebdavError(w, b, err) + return + } + + w.WriteHeader(http.StatusUnauthorized) return } @@ -106,76 +111,3 @@ func (m basicAuth) isBasicAuth(req *http.Request) bool { _, _, ok := req.BasicAuth() return m.enabled && ok } - -type code int - -const ( - // SabredavBadRequest maps to HTTP 400 - SabredavBadRequest code = iota - // SabredavMethodNotAllowed maps to HTTP 405 - SabredavMethodNotAllowed - // SabredavNotAuthenticated maps to HTTP 401 - SabredavNotAuthenticated - // SabredavPreconditionFailed maps to HTTP 412 - SabredavPreconditionFailed - // SabredavPermissionDenied maps to HTTP 403 - SabredavPermissionDenied - // SabredavNotFound maps to HTTP 404 - SabredavNotFound - // SabredavConflict maps to HTTP 409 - SabredavConflict -) - -var ( - codesEnum = []string{ - "Sabre\\DAV\\Exception\\BadRequest", - "Sabre\\DAV\\Exception\\MethodNotAllowed", - "Sabre\\DAV\\Exception\\NotAuthenticated", - "Sabre\\DAV\\Exception\\PreconditionFailed", - "Sabre\\DAV\\Exception\\PermissionDenied", - "Sabre\\DAV\\Exception\\NotFound", - "Sabre\\DAV\\Exception\\Conflict", - } -) - -type exception struct { - code code - message string - header string -} - -// Marshal just calls the xml marshaller for a given exception. -func Marshal(e exception) ([]byte, error) { - xmlstring, err := xml.Marshal(&errorXML{ - Xmlnsd: "DAV", - Xmlnss: "http://sabredav.org/ns", - Exception: codesEnum[e.code], - Message: e.message, - Header: e.header, - }) - if err != nil { - return []byte(""), err - } - return []byte(xml.Header + string(xmlstring)), err -} - -// http://www.webdav.org/specs/rfc4918.html#ELEMENT_error -type errorXML struct { - XMLName xml.Name `xml:"d:error"` - Xmlnsd string `xml:"xmlns:d,attr"` - Xmlnss string `xml:"xmlns:s,attr"` - Exception string `xml:"s:exception"` - Message string `xml:"s:message"` - InnerXML []byte `xml:",innerxml"` - Header string `xml:"s:header,omitempty"` -} - -func HandleWebdavError(w http.ResponseWriter, b []byte, err error) { - if err != nil { - w.WriteHeader(http.StatusInternalServerError) - return - } - _, err = w.Write(b) - if err != nil { - } -} diff --git a/proxy/pkg/webdav/response.go b/proxy/pkg/webdav/response.go new file mode 100644 index 000000000..0eb9a573e --- /dev/null +++ b/proxy/pkg/webdav/response.go @@ -0,0 +1,79 @@ +package webdav + +import ( + "encoding/xml" + "net/http" +) + +type code int + +const ( + // SabredavBadRequest maps to HTTP 400 + SabredavBadRequest code = iota + // SabredavMethodNotAllowed maps to HTTP 405 + SabredavMethodNotAllowed + // SabredavNotAuthenticated maps to HTTP 401 + SabredavNotAuthenticated + // SabredavPreconditionFailed maps to HTTP 412 + SabredavPreconditionFailed + // SabredavPermissionDenied maps to HTTP 403 + SabredavPermissionDenied + // SabredavNotFound maps to HTTP 404 + SabredavNotFound + // SabredavConflict maps to HTTP 409 + SabredavConflict +) + +var ( + codesEnum = []string{ + "Sabre\\DAV\\Exception\\BadRequest", + "Sabre\\DAV\\Exception\\MethodNotAllowed", + "Sabre\\DAV\\Exception\\NotAuthenticated", + "Sabre\\DAV\\Exception\\PreconditionFailed", + "Sabre\\DAV\\Exception\\PermissionDenied", + "Sabre\\DAV\\Exception\\NotFound", + "Sabre\\DAV\\Exception\\Conflict", + } +) + +type Exception struct { + Code code + Message string + Header string +} + +// Marshal just calls the xml marshaller for a given Exception. +func Marshal(e Exception) ([]byte, error) { + xmlstring, err := xml.Marshal(&errorXML{ + Xmlnsd: "DAV", + Xmlnss: "http://sabredav.org/ns", + Exception: codesEnum[e.Code], + Message: e.Message, + Header: e.Header, + }) + if err != nil { + return []byte(""), err + } + return []byte(xml.Header + string(xmlstring)), err +} + +// http://www.webdav.org/specs/rfc4918.html#ELEMENT_error +type errorXML struct { + XMLName xml.Name `xml:"d:error"` + Xmlnsd string `xml:"xmlns:d,attr"` + Xmlnss string `xml:"xmlns:s,attr"` + Exception string `xml:"s:Exception"` + Message string `xml:"s:Message"` + InnerXML []byte `xml:",innerxml"` + Header string `xml:"s:Header,omitempty"` +} + +func HandleWebdavError(w http.ResponseWriter, b []byte, err error) { + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + _, err = w.Write(b) + if err != nil { + } +} diff --git a/proxy/pkg/webdav/webdav.go b/proxy/pkg/webdav/webdav.go new file mode 100644 index 000000000..cfb59329a --- /dev/null +++ b/proxy/pkg/webdav/webdav.go @@ -0,0 +1,18 @@ +package webdav + +import "net/http" + +var methods = []string{"PROPFIND", "DELETE", "PROPPATCH", "MKCOL", "COPY", "MOVE", "LOCK", "UNLOCK"} + +// This is a non exhaustive way to detect if a request is directed to a webdav server. This naïve implementation +// only deals with the set of methods exclusive to WebDAV. Since WebDAV is a superset of HTTP, GET, POST and so on +// are valid methods, but this implementation would require a larger effort than we can build upon in this file. +// This is needed because the proxy might need to create a response with a webdav body; such as unauthorized. +func IsWebdavRequest(r *http.Request) bool { + for i := range methods { + if methods[i] == r.Method { + return true + } + } + return false +} From 6123512367fd610f88d72ad74cc4912a30da29a6 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Mon, 19 Jul 2021 10:15:26 +0200 Subject: [PATCH 4/7] adjust expected failures --- tests/acceptance/expected-failures-API-on-OCIS-storage.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/acceptance/expected-failures-API-on-OCIS-storage.md b/tests/acceptance/expected-failures-API-on-OCIS-storage.md index a73b9c521..dadb25844 100644 --- a/tests/acceptance/expected-failures-API-on-OCIS-storage.md +++ b/tests/acceptance/expected-failures-API-on-OCIS-storage.md @@ -3,12 +3,6 @@ ### File Basic file management like up and download, move, copy, properties, trash, versions and chunking. -#### [invalid webdav responses for unauthorized requests.](https://github.com/owncloud/product/issues/273) -- [apiTrashbin/trashbinFilesFolders.feature:215](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L215) -- [apiTrashbin/trashbinFilesFolders.feature:216](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L216) -- [apiTrashbin/trashbinFilesFolders.feature:230](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L230) -- [apiTrashbin/trashbinFilesFolders.feature:231](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L231) - #### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122) - [apiTrashbinRestore/trashbinRestore.feature:309](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbinRestore/trashbinRestore.feature#L309) - [apiTrashbinRestore/trashbinRestore.feature:310](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbinRestore/trashbinRestore.feature#L310) From 7f8e5d0229f8f4ed17d3a075880627daebb47a6d Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Mon, 19 Jul 2021 10:42:19 +0200 Subject: [PATCH 5/7] adjust expected failures for owncloud storage --- .../acceptance/expected-failures-API-on-OWNCLOUD-storage.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/acceptance/expected-failures-API-on-OWNCLOUD-storage.md b/tests/acceptance/expected-failures-API-on-OWNCLOUD-storage.md index 2b794c769..c913e53ce 100644 --- a/tests/acceptance/expected-failures-API-on-OWNCLOUD-storage.md +++ b/tests/acceptance/expected-failures-API-on-OWNCLOUD-storage.md @@ -13,12 +13,6 @@ The following scenarios fail on OWNCLOUD storage but not on OCIS storage: - [apiTrashbin/trashbinFilesFolders.feature:104](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L104) - [apiTrashbin/trashbinFilesFolders.feature:105](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L105) -#### [invalid webdav responses for unauthorized requests.](https://github.com/owncloud/product/issues/273) -- [apiTrashbin/trashbinFilesFolders.feature:215](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L215) -- [apiTrashbin/trashbinFilesFolders.feature:216](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L216) -- [apiTrashbin/trashbinFilesFolders.feature:230](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L230) -- [apiTrashbin/trashbinFilesFolders.feature:231](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L231) - #### [PROPFIND on trashbin with Depth: infinity only shows the first level](https://github.com/owncloud/ocis/issues/1116) - [apiTrashbin/trashbinFilesFolders.feature:278](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L278) - [apiTrashbin/trashbinFilesFolders.feature:279](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L279) From ebb67470dc0cd653ec9cbc50813b828d1aa425c5 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Mon, 19 Jul 2021 11:15:37 +0200 Subject: [PATCH 6/7] ignore error explicitly --- proxy/pkg/webdav/response.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/proxy/pkg/webdav/response.go b/proxy/pkg/webdav/response.go index 0eb9a573e..027904271 100644 --- a/proxy/pkg/webdav/response.go +++ b/proxy/pkg/webdav/response.go @@ -73,7 +73,5 @@ func HandleWebdavError(w http.ResponseWriter, b []byte, err error) { w.WriteHeader(http.StatusInternalServerError) return } - _, err = w.Write(b) - if err != nil { - } + _, _ = w.Write(b) } From 74696dbd3603548e6c85dc96ae843c5570148351 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Mon, 19 Jul 2021 13:29:12 +0200 Subject: [PATCH 7/7] remove redundant line --- proxy/pkg/middleware/basic_auth.go | 1 - 1 file changed, 1 deletion(-) diff --git a/proxy/pkg/middleware/basic_auth.go b/proxy/pkg/middleware/basic_auth.go index f7a107f6a..1f8d0a807 100644 --- a/proxy/pkg/middleware/basic_auth.go +++ b/proxy/pkg/middleware/basic_auth.go @@ -79,7 +79,6 @@ func BasicAuth(optionSetters ...Option) func(next http.Handler) http.Handler { return } - w.WriteHeader(http.StatusUnauthorized) return }