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

Publicstorageprovider rewrite #2646

Merged
merged 10 commits into from
Mar 23, 2022

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Mar 17, 2022

We cannot use the current publicstorageprovider implementation as it changes the fileid when accessing a file via a public link. This causes problems with the WOPI server when users enter and leave the session. see #2635

This PR uses two types of spaces: grant and mountpoint similar to the sharesstorageprovider. The mountpoint space is however only listed when the scope indicates a public link.

The grant space is used to forward requests to the actual storage provider.

Note that the mountpoint has a different resourceid than the grant.

  • The mountpoint space uses the publicstorageproviderid as the storageid/spaceid and the token as the opaqueid/nodeid.
  • The grant space uses the storageid/spaceid and opaqueid/nodeid of the shared resource. This causes requests to be routed directly to the original storage provider.

This is now in alignment to how the sharesstorageprovider works.

While relative access using the /public/{token} mountpoint works, a fileid based access, eg via /dav/spaces/{spaceid}!{nodeid} would need an additional publicstorage middleware that reduces the permissions to what is granted by the link. Currently, the authentication impersonates the owner of the shared resource, which would report the wrong permissions.

@butonic butonic requested review from a team, labkode, ishank011 and glpatcern as code owners March 17, 2022 21:05
@butonic butonic requested a review from micbar March 17, 2022 21:06
@butonic butonic marked this pull request as draft March 17, 2022 21:25
@glpatcern
Copy link
Member

Interesting finding, thanks @butonic - I guess we did not spot it because of our still hybrid deployment, where the WOPI server bypasses Reva to directly talk to the storage.

@butonic
Copy link
Contributor Author

butonic commented Mar 18, 2022

@glpatcern oh no this was brought to my attention by @wkloucek I still need to find out if the WOPI server works with this implementation or if we need to change the fileid of the mount point ... still trying to make the problem as well as the solution more graspable.

@butonic butonic marked this pull request as ready for review March 21, 2022 09:20
@butonic butonic self-assigned this Mar 21, 2022
@butonic butonic added the bug Something isn't working label Mar 21, 2022
butonic added 7 commits March 21, 2022 09:34
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic butonic force-pushed the publicstorageprovider-rewrite branch from f0d8b7b to c252c1e Compare March 21, 2022 09:35
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
pkg/auth/scope/publicshare.go Outdated Show resolved Hide resolved
Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

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

I can trigger a stack trace by doing the following steps:

  1. Create a folder
  2. Upload an image to that folder
  3. Create a public link of the folder
  4. Open the public link
  5. View image in media viewer
    Then there is a stack trace like this:
2022-03-21T15:20:24+01:00 ERR unary code=Unknown end="21/Mar/2022:15:20:24 +0100" from=tcp://127.0.0.1:50094 pkg=rgrpc service=storage start="21/Mar/2022:15:20:24 +0100" time_ns=1370727 traceid=00000000000000000000000000000000 uri=/cs3.storage.provider.v1beta1.ProviderAPI/ListStorageSpaces user-agent=grpc-go/1.45.0
goroutine 9188 [running]:
runtime/debug.Stack()
	/usr/local/go/src/runtime/debug/stack.go:24 +0x65
runtime/debug.PrintStack()
	/usr/local/go/src/runtime/debug/stack.go:16 +0x19
github.com/cs3org/reva/v2/internal/grpc/interceptors/recovery.recoveryFunc({0x53c7560, 0xc009d600f0}, {0x3841580, 0x5331740})
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/recovery/recovery.go:48 +0x31
github.com/grpc-ecosystem/go-grpc-middleware/recovery.recoverFrom({0x53c7560, 0xc009d600f0}, {0x3841580, 0x5331740}, 0xc0081d8be8)
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/recovery/interceptors.go:61 +0x36
github.com/grpc-ecosystem/go-grpc-middleware/recovery.UnaryServerInterceptor.func1.1()
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/recovery/interceptors.go:29 +0x7b
panic({0x3841580, 0x5331740})
	/usr/local/go/src/runtime/panic.go:1038 +0x215
github.com/cs3org/reva/v2/pkg/ctx.ContextMustGetUser({0x53c7560, 0xc009d60450})
	/home/corby/work/go/src/github.com/c0rby/reva/pkg/ctx/userctx.go:47 +0x65
github.com/cs3org/reva/v2/internal/grpc/services/gateway.(*svc).CreateHome(0x2, {0x53c7560, 0xc009d60450}, 0x0)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/services/gateway/storageprovider.go:107 +0x46
github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1._GatewayAPI_CreateHome_Handler.func1({0x53c7560, 0xc009d60450}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/cs3org/[email protected]/cs3/gateway/v1beta1/gateway_api.pb.go:3101 +0x78
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.UnaryServerInterceptor.func1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000}, 0xc009d13520, 0xc009d5c210)
	/home/corby/work/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/[email protected]/interceptor.go:325 +0x61c
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/chain.go:25 +0x3a
github.com/cs3org/reva/v2/internal/grpc/interceptors/auth.NewUnary.func1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000}, 0xc009d13520, 0xc009d13540)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/auth/auth.go:107 +0x25b
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/chain.go:25 +0x3a
github.com/grpc-ecosystem/go-grpc-middleware/recovery.UnaryServerInterceptor.func1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000}, 0xc0077253f8, 0x14f1b97)
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/recovery/interceptors.go:33 +0xc8
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/chain.go:25 +0x3a
github.com/cs3org/reva/v2/internal/grpc/interceptors/log.NewUnary.func1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000}, 0xc009d13520, 0xc009d13580)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/log/log.go:39 +0x9a
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/chain.go:25 +0x3a
github.com/cs3org/reva/v2/internal/grpc/interceptors/useragent.NewUnary.func1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000}, 0x1, 0xc009d135a0)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/useragent/useragent.go:38 +0xe9
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d600f0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/chain.go:25 +0x3a
github.com/cs3org/reva/v2/internal/grpc/interceptors/token.NewUnary.func1({0x53c7560, 0xc009d60060}, {0x3ca0040, 0xc009d60000}, 0x3df6fc0, 0xc009d135c0)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/token/token.go:44 +0x159
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d60060}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/chain.go:25 +0x3a
github.com/cs3org/reva/v2/internal/grpc/interceptors/appctx.NewUnary.func1({0x53c7560, 0xc009d43fb0}, {0x3ca0040, 0xc009d60000}, 0x18, 0xc009d135e0)
	/home/corby/work/go/src/github.com/c0rby/reva/internal/grpc/interceptors/appctx/appctx.go:42 +0x5ab
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x53c7560, 0xc009d43fb0}, {0x3ca0040, 0xc009d60000})
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/chain.go:25 +0x3a
github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1({0x53c7560, 0xc009d43fb0}, {0x3ca0040, 0xc009d60000}, 0xc000d59bd0, 0x399a460)
	/home/corby/work/go/pkg/mod/github.com/grpc-ecosystem/[email protected]/chain.go:34 +0xbf
github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1._GatewayAPI_CreateHome_Handler({0x3e70700, 0xc0003f1500}, {0x53c7560, 0xc009d43fb0}, 0xc009d15f20, 0xc002698600)
	/home/corby/work/go/pkg/mod/github.com/cs3org/[email protected]/cs3/gateway/v1beta1/gateway_api.pb.go:3103 +0x138
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0015bc000, {0x53f8be0, 0xc000d7e1a0}, 0xc009d58240, 0xc001ba4ab0, 0x6efe440, 0x0)
	/home/corby/work/go/pkg/mod/google.golang.org/[email protected]/server.go:1282 +0xccf
google.golang.org/grpc.(*Server).handleStream(0xc0015bc000, {0x53f8be0, 0xc000d7e1a0}, 0xc009d58240, 0x0)
	/home/corby/work/go/pkg/mod/google.golang.org/[email protected]/server.go:1619 +0xa2a
google.golang.org/grpc.(*Server).serveStreams.func1.2()
	/home/corby/work/go/pkg/mod/google.golang.org/[email protected]/server.go:921 +0x98
created by google.golang.org/grpc.(*Server).serveStreams.func1
	/home/corby/work/go/pkg/mod/google.golang.org/[email protected]/server.go:919 +0x294
2022-03-21T15:20:24+01:00 ERR user not found in context pkg=rgrpc service=storage traceid=00000000000000000000000000000000
2022-03-21T15:20:24+01:00 ERR unary code=Internal end="21/Mar/2022:15:20:24 +0100" from=tcp://127.0.0.1:51036 pkg=rgrpc service=storage start="21/Mar/2022:15:20:24 +0100" time_ns=591242 traceid=00000000000000000000000000000000 uri=/cs3.gateway.v1beta1.GatewayAPI/CreateHome user-agent=grpc-go/1.45.0
2022-03-21T15:20:24+01:00 ERR error calling CreateHome error="rpc error: code = Internal desc = user not found in context" service=proxy

It doesn't seem to affect the functionality but it isn't nice.

@C0rby
Copy link
Contributor

C0rby commented Mar 21, 2022

Also when updating the role of a share, the PROPFIND still returns the old role. Not sure if this is caused by this PR though.

butonic added 2 commits March 21, 2022 15:49
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic
Copy link
Contributor Author

butonic commented Mar 22, 2022

I can trigger a stack trace by doing the following steps:

confirmed. I prevent that now by disallowing CreateHome when in public scope: 20392fb

@butonic butonic requested review from kobergj and C0rby March 22, 2022 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants