Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Trashbin Expected Failures #2294

Merged
merged 8 commits into from
Jul 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ocs/pkg/server/http/svc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
Expand Down
6 changes: 3 additions & 3 deletions ocs/pkg/service/v0/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 2 additions & 1 deletion proxy/pkg/middleware/account_resolver.go
Original file line number Diff line number Diff line change
@@ -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"
Expand Down
25 changes: 21 additions & 4 deletions proxy/pkg/middleware/basic_auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ package middleware

import (
"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"
"github.com/owncloud/ocis/proxy/pkg/webdav"
)

const publicFilesEndpoint = "/remote.php/dav/public-files/"
Expand Down Expand Up @@ -61,7 +63,22 @@ 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)

if webdav.IsWebdavRequest(req) {
b, err := webdav.Marshal(webdav.Exception{
Code: webdav.SabredavPermissionDenied,
Message: "Authentication error",
})

webdav.HandleWebdavError(w, b, err)
return
}

return
}

Expand Down Expand Up @@ -90,6 +107,6 @@ 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
}
77 changes: 77 additions & 0 deletions proxy/pkg/webdav/response.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package webdav

import (
"encoding/xml"
"net/http"
)

type code int

const (
// SabredavBadRequest maps to HTTP 400
SabredavBadRequest code = iota
butonic marked this conversation as resolved.
Show resolved Hide resolved
// 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
}
_, _ = w.Write(b)
}
18 changes: 18 additions & 0 deletions proxy/pkg/webdav/webdav.go
Original file line number Diff line number Diff line change
@@ -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
}
6 changes: 0 additions & 6 deletions tests/acceptance/expected-failures-API-on-OCIS-storage.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 0 additions & 6 deletions tests/acceptance/expected-failures-API-on-OWNCLOUD-storage.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down