[full-ci] feat: [OCISDEV-533] Provide the protected-* spaces#12069
[full-ci] feat: [OCISDEV-533] Provide the protected-* spaces#12069
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
a15017d to
538b3f5
Compare
added MFA enforcement for the ocdav and arciver
bbde382 to
b44091a
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces new “protected” storage space types (protected-personal, protected-project) and adds MFA-based access restrictions around them across the stack (reva/WebDAV/archiver + oCIS Graph/search/proxy + gateway config).
Changes:
- Bump
github.com/owncloud/reva/v2and extend reva space handling/config to include protected space types. - Enforce MFA for accessing protected spaces in WebDAV (and archiver) and prevent protected→unprotected copy operations.
- Add automatic creation for
protected-personalon home creation, and update Graph/search behavior for protected spaces.
Reviewed changes
Copilot reviewed 9 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vendor/modules.txt | Updates vendored module list for the reva bump. |
| vendor/github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/spaces.go | Adds protected space types to decomposedfs space creation/listing/deletion logic. |
| vendor/github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/permissions/spacepermissions.go | Extends quota permission checks to protected space types. |
| vendor/github.com/owncloud/reva/v2/pkg/storage/registry/spaces/spaces.go | Adds default registry mapping for protected space mountpoints/path templates. |
| vendor/github.com/owncloud/reva/v2/internal/http/services/owncloud/ocdav/spaces.go | Adds MFA enforcement when addressing a space by ID. |
| vendor/github.com/owncloud/reva/v2/internal/http/services/owncloud/ocdav/get.go | Enforces MFA when downloading from protected spaces. |
| vendor/github.com/owncloud/reva/v2/internal/http/services/owncloud/ocdav/dav.go | Introduces protected/unprotected helpers + MFA header constants for ocdav. |
| vendor/github.com/owncloud/reva/v2/internal/http/services/owncloud/ocdav/copy.go | Adds copy restriction from protected→unprotected spaces. |
| vendor/github.com/owncloud/reva/v2/internal/http/services/archiver/handler.go | Adds MFA enforcement before allowing archive downloads of protected space resources. |
| vendor/github.com/owncloud/reva/v2/internal/grpc/services/storageprovider/storageprovider.go | Improves status propagation when CreateHome fails. |
| vendor/github.com/owncloud/reva/v2/internal/grpc/services/gateway/storageprovidercache.go | Ensures cached CreateHome/CreateStorageSpace responses return ALREADY_EXISTS status code. |
| services/search/pkg/search/service.go | Excludes protected spaces from search scope. |
| services/proxy/pkg/middleware/create_home.go | Attempts to create protected-personal after successful home creation. |
| services/graph/pkg/service/v0/users.go | Refactors query/MFA validation usage for users listing. |
| services/graph/pkg/service/v0/groups.go | Refactors query/MFA validation usage for groups listing. |
| services/graph/pkg/service/v0/graph.go | Introduces reusable query/MFA validators and driveType MFA gating. |
| services/graph/pkg/service/v0/drives.go | Adds protected space types to drives handling and filters protected spaces when MFA is missing. |
| services/graph/pkg/service/v0/graph_test.go | Adds tests for protected space filtering and protected-project creation. |
| services/gateway/pkg/revaconfig/config.go | Adds protected space registry config for the gateway-generated reva config. |
| go.mod / go.sum | Updates reva dependency version + sums. |
| .gitignore | Ignores .agents/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| "protected-personal": map[string]interface{}{ | ||
| "mount_point": "/protected-users", | ||
| "path_template": "/protected-users/{{.Space.Name}}", |
There was a problem hiding this comment.
The new mount config for "protected-personal" uses path_template: /protected-users/{{.Space.Name}}. Since protected personal spaces are created with a fixed name ("Protected Personal"), this will cause path collisions across users and won’t match the default reva template (which uses the owner id). Use the owner id in the path template (similar to the existing personal space config).
| "path_template": "/protected-users/{{.Space.Name}}", | |
| "path_template": "/protected-users/{{.Space.Owner.Id.OpaqueId}}", |
There was a problem hiding this comment.
This is a "special" personal folder, so it makes sense to have the same formatting as with personal folders
| }, | ||
| "protected-personal": map[string]interface{}{ | ||
| "mount_point": "/protected-users", | ||
| "path_template": "/protected-users/{{.Space.Name}}", |
There was a problem hiding this comment.
This is a "special" personal folder, so it makes sense to have the same formatting as with personal folders
| logger := g.logger.SubloggerWithRequestID(r.Context()) | ||
| logger.Error().Str("path", r.URL.Path).Msg("MFA required but not satisfied") | ||
| mfa.SetRequiredStatus(w) | ||
| if !g.validateMFA(r, w) { |
There was a problem hiding this comment.
I'm not entirely sure about this. There are a couple of thing to notice:
- Most of the responses will be written in the public function. In this particular case, there will be information that will be written in a private function. This might be confusing in the long run because you might not expect the function to write an HTTP response.
- Reusing the function might be limited to this particular use case. Basically, you'll be forced to write a wrong response if MFA fails to validate. You won't be able to add more information to the log or change the log message. In addition, the log will likely point to the private function, so knowing what function have failed will be more difficult (you'll likely need to check and map the request instead of getting the information from the log directly).
I'm not against the change, but I'm not sure if it outweighs the disadvantages.
|
|
||
| // getDrives implements the Service interface. | ||
| func (g Graph) getDrives(r *http.Request, unrestricted bool, apiVersion APIVersion) ([]*libregraph.Drive, error) { | ||
| func (g Graph) getDrives(w http.ResponseWriter, r *http.Request, unrestricted bool, apiVersion APIVersion) ([]*libregraph.Drive, error) { |
There was a problem hiding this comment.
I'm not fond of passing the ResponseWriter around. Ideally, there should be only one place allowed for writing the HTTP response, and that should be the public service's method.
This might get problematic, not only because several places will write a HTTP response, but also because the message will be the same in all of them. Figuring out who wrote the HTTP response will be annoying.
| return res, nil | ||
| } | ||
|
|
||
| // Filter out protected spaces if MFA is not enabled |
There was a problem hiding this comment.
Can we request first the non-protected folders and then the protected ones if we have MFA? Or check the MFA first and request only the non-protected folders if we don't have MFA?
I'm not sure how it's implemented in reva, but at least there should be less network traffic (less data we need to transfer) and no need to filter results, so it should perform faster.
| type DriveTypeValidator struct{} | ||
|
|
||
| // Validate checks if the driveType filter contains protected types. | ||
| func (v DriveTypeValidator) Validate(query *godata.GoDataQuery) error { |
There was a problem hiding this comment.
I don't think this should be needed... Any chance to move the drive check into reva?
Reva should be able to check somehow whether request has MFA or not, and then decide if the send the protected spaces or not.
I mean, if you are under MFA, why can't you list or show protected spaces?
|
We might need to discuss about the protected personal space. It seems this PR implements it as a new type of space... in my head I thought it would be implemented as part of the home creation: when you create the home, you create both the personal and the protected personal spaces. I haven't check in depth, so there might be problems with that approach, mostly incompatibilities with the storage providers, but on the other hand it could be fine if some providers don't implement the feature and don't create the protected personal space as part of the home creation. |
|
I have tried another approach. Maybe we can discuss it to. The idea was to use the |
|
Closed in favor #12108 |
Description
The
protected-personalspace will be created automaticallyTo create the
protected-projectuse:The
graph/v1beta1/me/drivesAcceptance Criteria
New space types: protected-personal and protected-project
All users get a protected-personal space
The protected-project spaces can be created via API
Accessing (WebDAV, Downloading, Archiver) resources on protected spaces requires MFA
Screenshots (if appropriate):
Types of changes
Checklist: