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.

Sign up for GitHub

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

Jump to bottom

Ocfs lookup userid (update) #1052

Merged
merged 7 commits into from
Aug 10, 2020
Merged

Ocfs lookup userid (update) #1052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8bbbd78
ocfs: lookup user to render template properly
butonic Jul 30, 2020
d4a46c5
refactor, configurable addr
butonic Jul 30, 2020
e8b185b
use correct userprovider service
butonic Jul 31, 2020
ccc3220
OCFS: Change default user layout
Aug 4, 2020
4b0bff3
OCFS update configs for user provider svc
Aug 6, 2020
82f1bd3
Fix Litmus test to use UUID in DAV URL
Aug 6, 2020
0acc7d9
Adjust PR number in changelog
Aug 6, 2020
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
3 changes: 2 additions & 1 deletion .drone.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,8 @@ steps:
- name: litmus-oc-new-webdav
image: owncloud/litmus:latest
environment:
LITMUS_URL: http://revad-services:20080/remote.php/dav/files/einstein
# UUID is einstein user, see https://github.com/owncloud/ocis-accounts/blob/8de0530f31ed5ffb0bbb7f7f3471f87f429cb2ea/pkg/service/v0/service.go#L45
LITMUS_URL: http://revad-services:20080/remote.php/dav/files/4c510ada-c86b-4815-8820-42cdf82c3d51
LITMUS_USERNAME: einstein
LITMUS_PASSWORD: relativity
TESTS: basic http copymove props
Expand Down
5 changes: 5 additions & 0 deletions changelog/unreleased/ocfs-user-lookup.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Bugfix: ocfs: Lookup user to render template properly

Currently, the username is used to construct paths, which breaks when mounting the `owncloud` storage driver at `/oc` and then expecting paths that use the username like `/oc/einstein/foo` to work, because they will mismatch the path that is used from propagation which uses `/oc/u-u-i-d` as the root, giving an `internal path outside root` error

https://github.com/cs3org/reva/pull/1052
1 change: 1 addition & 0 deletions drone/oc-integration-tests/storage-oc.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ data_server_url = "http://localhost:11001/data"
[grpc.services.storageprovider.drivers.owncloud]
datadirectory = "/drone/src/tmp/reva/data"
redis = "redis:6379"
userprovidersvc = "localhost:18000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting this will save a trip to the gateway. When left out it should fall back to the gatewaysvc in the [shared] section and work just as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fallback did not seem to work, I had to add this

there was a strange thing where it was resolving some service, but that service did not implement the UserAPI, see #1033 (comment)

I didn't manage to find out what service it was resolving exactly


[http]
address = "0.0.0.0:11001"
Expand Down
3 changes: 2 additions & 1 deletion examples/oc-phoenix/storage-oc.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ data_server_url = "http://localhost:11001/data"

[grpc.services.storageprovider.drivers.owncloud]
datadirectory = "/var/tmp/reva/data"

redis = "redis:6379"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting redis is optional. it defaults to localhost:6379

userprovidersvc = "localhost:18000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting this will save a trip to the gateway. When left out it should fall back to the gatewaysvc in the [shared] section and work just as well.


[http]
address = "0.0.0.0:11001"
Expand Down
157 changes: 119 additions & 38 deletions pkg/storage/fs/owncloud/owncloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,15 @@ import (
"time"

userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
"github.com/cs3org/reva/pkg/appctx"
"github.com/cs3org/reva/pkg/errtypes"
"github.com/cs3org/reva/pkg/logger"
"github.com/cs3org/reva/pkg/mime"
"github.com/cs3org/reva/pkg/rgrpc/todo/pool"
"github.com/cs3org/reva/pkg/sharedconf"
"github.com/cs3org/reva/pkg/storage"
"github.com/cs3org/reva/pkg/storage/fs/registry"
"github.com/cs3org/reva/pkg/storage/utils/templates"
Expand Down Expand Up @@ -152,13 +155,14 @@ func init() {
}

type config struct {
DataDirectory string `mapstructure:"datadirectory"`
UploadInfoDir string `mapstructure:"upload_info_dir"`
ShareDirectory string `mapstructure:"sharedirectory"`
UserLayout string `mapstructure:"user_layout"`
Redis string `mapstructure:"redis"`
EnableHome bool `mapstructure:"enable_home"`
Scan bool `mapstructure:"scan"`
DataDirectory string `mapstructure:"datadirectory"`
UploadInfoDir string `mapstructure:"upload_info_dir"`
ShareDirectory string `mapstructure:"sharedirectory"`
UserLayout string `mapstructure:"user_layout"`
Redis string `mapstructure:"redis"`
EnableHome bool `mapstructure:"enable_home"`
Scan bool `mapstructure:"scan"`
UserProviderEndpoint string `mapstructure:"userprovidersvc"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the UserProviderEndpoint to look up a user by its username.

}

func parseConfig(m map[string]interface{}) (*config, error) {
Expand All @@ -175,7 +179,7 @@ func (c *config) init(m map[string]interface{}) {
c.Redis = ":6379"
}
if c.UserLayout == "" {
c.UserLayout = "{{.Username}}"
c.UserLayout = "{{.Id.OpaqueId}}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the default, but will prevent username changes from breaking the storage lookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put this there to align to the change in ocis-reva which also changes the default. I hope it's ok.
Otherwise we'll have further discrepancies.

}
if c.UploadInfoDir == "" {
c.UploadInfoDir = "/var/tmp/reva/uploadinfo"
Expand All @@ -187,6 +191,7 @@ func (c *config) init(m map[string]interface{}) {
if _, ok := m["scan"]; !ok {
c.Scan = true
}
c.UserProviderEndpoint = sharedconf.GetGatewaySVC(c.UserProviderEndpoint)
}

// New returns an implementation to of the storage.FS interface that talk to
Expand Down Expand Up @@ -300,19 +305,29 @@ func (fs *ocfs) wrap(ctx context.Context, fn string) (internal string) {
// p = <username>/foo/bar.txt
parts := strings.SplitN(fn, "/", 2)

switch len(parts) {
case 1:
// parts = "" or "<username>"
if parts[0] == "" {
internal = fs.c.DataDirectory
return
}
if len(parts) == 1 && parts[0] == "" {
internal = fs.c.DataDirectory
return
}

// parts[0] contains the username or userid.
u, err := fs.getUser(ctx, parts[0])
if err != nil {
logger.New().Error().Err(err).
Msg("could not get user")
// TODO return invalid internal path?
return
}
layout := templates.WithUser(u, fs.c.UserLayout)

if len(parts) == 1 {
// parts = "<username>"
internal = path.Join(fs.c.DataDirectory, parts[0], "files")
default:
internal = path.Join(fs.c.DataDirectory, layout, "files")
} else {
// parts = "<username>", "foo/bar.txt"
internal = path.Join(fs.c.DataDirectory, parts[0], "files", parts[1])
internal = path.Join(fs.c.DataDirectory, layout, "files", parts[1])
}

}
return
}
Expand All @@ -330,18 +345,27 @@ func (fs *ocfs) wrapShadow(ctx context.Context, fn string) (internal string) {
// p = <username>/foo/bar.txt
parts := strings.SplitN(fn, "/", 2)

switch len(parts) {
case 1:
// parts = "" or "<username>"
if parts[0] == "" {
internal = fs.c.DataDirectory
return
}
if len(parts) == 1 && parts[0] == "" {
internal = fs.c.DataDirectory
return
}

// parts[0] contains the username or userid.
u, err := fs.getUser(ctx, parts[0])
if err != nil {
logger.New().Error().Err(err).
Msg("could not get user")
// TODO return invalid internal path?
return
}
layout := templates.WithUser(u, fs.c.UserLayout)

if len(parts) == 1 {
// parts = "<username>"
internal = path.Join(fs.c.DataDirectory, parts[0], "shadow_files")
default:
internal = path.Join(fs.c.DataDirectory, layout, "shadow_files")
} else {
// parts = "<username>", "foo/bar.txt"
internal = path.Join(fs.c.DataDirectory, parts[0], "shadow_files", parts[1])
internal = path.Join(fs.c.DataDirectory, layout, "shadow_files", parts[1])
}
}
return
Expand All @@ -361,13 +385,23 @@ func (fs *ocfs) getVersionsPath(ctx context.Context, np string) string {
// np = /<username>/files/foo/bar.txt
parts := strings.SplitN(np, "/", 4)

// parts[1] contains the username or userid.
u, err := fs.getUser(ctx, parts[1])
if err != nil {
logger.New().Error().Err(err).
Msg("could not get user")
// TODO return invalid internal path?
return ""
}
layout := templates.WithUser(u, fs.c.UserLayout)

switch len(parts) {
case 3:
// parts = "", "<username>"
return path.Join(fs.c.DataDirectory, parts[1], "files_versions")
return path.Join(fs.c.DataDirectory, layout, "files_versions")
case 4:
// parts = "", "<username>", "foo/bar.txt"
return path.Join(fs.c.DataDirectory, parts[1], "files_versions", parts[3])
return path.Join(fs.c.DataDirectory, layout, "files_versions", parts[3])
default:
return "" // TODO Must not happen?
}
Expand All @@ -381,7 +415,8 @@ func (fs *ocfs) getRecyclePath(ctx context.Context) (string, error) {
err := errors.Wrap(errtypes.UserRequired("userrequired"), "error getting user from ctx")
return "", err
}
return path.Join(fs.c.DataDirectory, u.GetUsername(), "files_trashbin/files"), nil
layout := templates.WithUser(u, fs.c.UserLayout)
return path.Join(fs.c.DataDirectory, layout, "files_trashbin/files"), nil
}

func (fs *ocfs) getVersionRecyclePath(ctx context.Context) (string, error) {
Expand All @@ -390,7 +425,8 @@ func (fs *ocfs) getVersionRecyclePath(ctx context.Context) (string, error) {
err := errors.Wrap(errtypes.UserRequired("userrequired"), "error getting user from ctx")
return "", err
}
return path.Join(fs.c.DataDirectory, u.GetUsername(), "files_trashbin/files_versions"), nil
layout := templates.WithUser(u, fs.c.UserLayout)
return path.Join(fs.c.DataDirectory, layout, "files_trashbin/files_versions"), nil
}

func (fs *ocfs) unwrap(ctx context.Context, internal string) (external string) {
Expand Down Expand Up @@ -473,6 +509,42 @@ func (fs *ocfs) getOwner(internal string) string {
return ""
}

func (fs *ocfs) getUser(ctx context.Context, usernameOrID string) (id *userpb.User, err error) {
u := user.ContextMustGetUser(ctx)
// check if username matches and id is set
if u.Username == usernameOrID && u.Id != nil && u.Id.OpaqueId != "" {
return u, nil
}
// check if userid matches and username is set
if u.Id != nil && u.Id.OpaqueId == usernameOrID && u.Username != "" {
return u, nil
}
// look up at the userprovider

// parts[0] contains the username or userid. use user service to look up id
c, err := pool.GetUserProviderServiceClient(fs.c.UserProviderEndpoint)
if err != nil {
return nil, err
}
res, err := c.GetUser(ctx, &userpb.GetUserRequest{
UserId: &userpb.UserId{OpaqueId: usernameOrID},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When configuring the userprovider with ldap you need to configure a filter that checks both: the username attribute as well as the userid attribute.

})
if err != nil {
return nil, err
}

if res.Status.Code == rpc.Code_CODE_NOT_FOUND {
logger.New().Error().Str("code", string(res.Status.Code)).Msg("user not found")
return nil, fmt.Errorf("user not found")
}

if res.Status.Code != rpc.Code_CODE_OK {
logger.New().Error().Str("code", string(res.Status.Code)).Msg("user lookup failed")
return nil, fmt.Errorf("user lookup failed")
}
return res.User, nil
}

func (fs *ocfs) convertToResourceInfo(ctx context.Context, fi os.FileInfo, np string, fn string, c redis.Conn, mdKeys []string) *provider.ResourceInfo {
id := readOrCreateID(ctx, np, c)

Expand Down Expand Up @@ -545,10 +617,9 @@ func (fs *ocfs) convertToResourceInfo(ctx context.Context, fi os.FileInfo, np st
appctx.GetLogger(ctx).Error().Err(err).Msg("error getting list of extended attributes")
}

return &provider.ResourceInfo{
ri := &provider.ResourceInfo{
Id: &provider.ResourceId{OpaqueId: id},
Path: fn,
Owner: &userpb.UserId{OpaqueId: fs.getOwner(np)},
Type: getResourceType(fi.IsDir()),
Etag: etag,
MimeType: mime.Detect(fi.IsDir(), fn),
Expand All @@ -562,6 +633,14 @@ func (fs *ocfs) convertToResourceInfo(ctx context.Context, fi os.FileInfo, np st
Metadata: metadata,
},
}

if owner, err := fs.getUser(ctx, fs.getOwner(np)); err == nil {
ri.Owner = owner.Id
} else {
appctx.GetLogger(ctx).Error().Err(err).Msg("error getting owner")
}

return ri
}
func getResourceType(isDir bool) provider.ResourceType {
if isDir {
Expand Down Expand Up @@ -881,6 +960,7 @@ func (fs *ocfs) ListGrants(ctx context.Context, ref *provider.Reference) (grants
grants = make([]*provider.Grant, 0, len(aces))
for i := range aces {
grantee := &provider.Grantee{
// TODO lookup uid from principal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The user lookup here needs to be implemented before the sharing tests can pass. at least I think they should fail somewhere without it.

Id: &userpb.UserId{OpaqueId: aces[i].Principal},
Type: fs.getGranteeType(aces[i]),
}
Expand Down Expand Up @@ -1852,7 +1932,8 @@ func (fs *ocfs) RestoreRecycleItem(ctx context.Context, key string) error {
} else {
origin = path.Clean(string(v))
}
tgt := path.Join(fs.wrap(ctx, path.Join("/", u.GetUsername(), origin)), strings.TrimSuffix(path.Base(src), suffix))
layout := templates.WithUser(u, fs.c.UserLayout)
tgt := path.Join(fs.wrap(ctx, path.Join("/", layout, origin)), strings.TrimSuffix(path.Base(src), suffix))
// move back to original location
if err := os.Rename(src, tgt); err != nil {
log.Error().Err(err).Str("path", src).Msg("could not restore item")
Expand All @@ -1873,8 +1954,8 @@ func (fs *ocfs) propagate(ctx context.Context, leafPath string) error {
if fs.c.EnableHome {
root = fs.wrap(ctx, "/")
} else {
u := user.ContextMustGetUser(ctx)
root = fs.wrap(ctx, path.Join("/", u.GetUsername()))
owner := fs.getOwner(leafPath)
root = fs.wrap(ctx, owner)
}
if !strings.HasPrefix(leafPath, root) {
err := errors.New("internal path outside root")
Expand All @@ -1897,7 +1978,7 @@ func (fs *ocfs) propagate(ctx context.Context, leafPath string) error {
}

parts := strings.Split(strings.TrimPrefix(leafPath, root), "/")
// root never ents in / so the split returns an empty first element, which we can skip
// root never ends in / so the split returns an empty first element, which we can skip
// we do not need to chmod the last element because it is the leaf path (< and not <= comparison)
for i := 1; i < len(parts); i++ {
appctx.GetLogger(ctx).Debug().
Expand Down
4 changes: 3 additions & 1 deletion pkg/storage/fs/owncloud/upload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/cs3org/reva/pkg/appctx"
"github.com/cs3org/reva/pkg/errtypes"
"github.com/cs3org/reva/pkg/logger"
"github.com/cs3org/reva/pkg/storage/utils/templates"
"github.com/cs3org/reva/pkg/user"
"github.com/google/uuid"
"github.com/pkg/errors"
Expand Down Expand Up @@ -203,7 +204,8 @@ func (fs *ocfs) getUploadPath(ctx context.Context, uploadID string) (string, err
err := errors.Wrap(errtypes.UserRequired("userrequired"), "error getting user from ctx")
return "", err
}
return filepath.Join(fs.c.DataDirectory, u.Username, "uploads", uploadID), nil
layout := templates.WithUser(u, fs.c.UserLayout)
return filepath.Join(fs.c.DataDirectory, layout, "uploads", uploadID), nil
}

// GetUpload returns the Upload for the given upload id
Expand Down