Skip to content
Closed
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,4 @@ go.work.sum
.envrc
CLAUDE.md
.claude/
.agents/
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ require (
github.com/open-policy-agent/opa v1.12.3
github.com/orcaman/concurrent-map v1.0.0
github.com/owncloud/libre-graph-api-go v1.0.5-0.20260216101009-eeac018af245
github.com/owncloud/reva/v2 v2.0.0-20260223124048-61ee39d95d5f
github.com/owncloud/reva/v2 v2.0.0-20260303153746-059bcdfc4fbb
github.com/pkg/errors v0.9.1
github.com/pkg/xattr v0.4.12
github.com/prometheus/client_golang v1.23.2
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -742,8 +742,8 @@ github.com/orcaman/concurrent-map v1.0.0 h1:I/2A2XPCb4IuQWcQhBhSwGfiuybl/J0ev9HD
github.com/orcaman/concurrent-map v1.0.0/go.mod h1:Lu3tH6HLW3feq74c2GC+jIMS/K2CFcDWnWD9XkenwhI=
github.com/owncloud/libre-graph-api-go v1.0.5-0.20260216101009-eeac018af245 h1:JRidLTAKhnvyLMRtVtSF4lhBa0NSAOs6fof+d6JnKII=
github.com/owncloud/libre-graph-api-go v1.0.5-0.20260216101009-eeac018af245/go.mod h1:z61VMGAJRtR1nbgXWiNoCkxUXP1B3Je9rMuJbnGd+Og=
github.com/owncloud/reva/v2 v2.0.0-20260223124048-61ee39d95d5f h1:M935ztFcjtRbP3ns8Fgb2XL5jalNd3sXYMUXf0kAwTQ=
github.com/owncloud/reva/v2 v2.0.0-20260223124048-61ee39d95d5f/go.mod h1:+rCy6oGYb2/qs5gmQa8y/pHARw634vB73MZGDY2SBIQ=
github.com/owncloud/reva/v2 v2.0.0-20260303153746-059bcdfc4fbb h1:+g9i1ZCb5EAufhjX967R2Zkw3UtgKk6XfXNQ4LQg9iE=
github.com/owncloud/reva/v2 v2.0.0-20260303153746-059bcdfc4fbb/go.mod h1:+rCy6oGYb2/qs5gmQa8y/pHARw634vB73MZGDY2SBIQ=
github.com/oxtoacart/bpool v0.0.0-20190530202638-03653db5a59c h1:rp5dCmg/yLR3mgFuSOe4oEnDDmGLROTvMragMUXpTQw=
github.com/oxtoacart/bpool v0.0.0-20190530202638-03653db5a59c/go.mod h1:X07ZCGwUbLaax7L0S3Tw4hpejzu63ZrrQiUe6W0hcy0=
github.com/pablodz/inotifywaitgo v0.0.9 h1:njquRbBU7fuwIe5rEvtaniVBjwWzcpdUVptSgzFqZsw=
Expand Down
8 changes: 8 additions & 0 deletions services/gateway/pkg/revaconfig/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,14 @@ func spacesProviders(cfg *config.Config, logger log.Logger) map[string]map[strin
"mount_point": "/projects",
"path_template": "/projects/{{.Space.Name}}",
},
"protected-personal": map[string]interface{}{
"mount_point": "/protected-users",
"path_template": "/protected-users/{{.Space.Name}}",
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
"path_template": "/protected-users/{{.Space.Name}}",
"path_template": "/protected-users/{{.Space.Owner.Id.OpaqueId}}",

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a "special" personal folder, so it makes sense to have the same formatting as with personal folders

},
"protected-project": map[string]interface{}{
"mount_point": "/protected-projects",
"path_template": "/protected-projects/{{.Space.Name}}",
},
},
},
cfg.StorageSharesEndpoint: {
Expand Down
67 changes: 45 additions & 22 deletions services/graph/pkg/service/v0/drives.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,13 @@ import (
)

const (
_spaceTypePersonal = "personal"
_spaceTypeProject = "project"
_spaceTypeVirtual = "virtual"
_spaceTypeMountpoint = "mountpoint"
_spaceStateTrashed = "trashed"
_spaceTypePersonal = "personal"
_spaceTypeProject = "project"
_spaceTypeProtectedPersonal = "protected-personal"
_spaceTypeProtectedProject = "protected-project"
_spaceTypeVirtual = "virtual"
_spaceTypeMountpoint = "mountpoint"
_spaceStateTrashed = "trashed"

_sortDescending = "desc"
)
Expand Down Expand Up @@ -78,7 +80,7 @@ func (g Graph) GetDrives(version APIVersion) http.HandlerFunc {
// GetDrivesV1 attempts to retrieve the current users drives;
// it lists all drives the current user has access to.
func (g Graph) GetDrivesV1(w http.ResponseWriter, r *http.Request) {
spaces, errCode := g.getDrives(r, false, APIVersion_1)
spaces, errCode := g.getDrives(w, r, false, APIVersion_1)
if errCode != nil {
errorcode.RenderError(w, r, errCode)
return
Expand All @@ -99,7 +101,7 @@ func (g Graph) GetDrivesV1(w http.ResponseWriter, r *http.Request) {
// it includes the grantedtoV2 property
// it uses unified roles instead of the cs3 representations
func (g Graph) GetDrivesV1Beta1(w http.ResponseWriter, r *http.Request) {
spaces, errCode := g.getDrives(r, false, APIVersion_1_Beta_1)
spaces, errCode := g.getDrives(w, r, false, APIVersion_1_Beta_1)
if errCode != nil {
errorcode.RenderError(w, r, errCode)
return
Expand Down Expand Up @@ -133,14 +135,11 @@ func (g Graph) GetAllDrives(version APIVersion) http.HandlerFunc {
// GetAllDrivesV1 attempts to retrieve the current users drives;
// it includes another user's drives, if the current user has the permission.
func (g Graph) GetAllDrivesV1(w http.ResponseWriter, r *http.Request) {
if !mfa.Has(r.Context()) {
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) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

return
}

spaces, errCode := g.getDrives(r, true, APIVersion_1)
spaces, errCode := g.getDrives(w, r, true, APIVersion_1)
if errCode != nil {
errorcode.RenderError(w, r, errCode)
return
Expand All @@ -160,14 +159,11 @@ func (g Graph) GetAllDrivesV1(w http.ResponseWriter, r *http.Request) {
// it includes the grantedtoV2 property
// it uses unified roles instead of the cs3 representations
func (g Graph) GetAllDrivesV1Beta1(w http.ResponseWriter, r *http.Request) {
if !mfa.Has(r.Context()) {
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) {
return
}

drives, errCode := g.getDrives(r, true, APIVersion_1_Beta_1)
drives, errCode := g.getDrives(w, r, true, APIVersion_1_Beta_1)
if errCode != nil {
errorcode.RenderError(w, r, errCode)
return
Expand All @@ -184,7 +180,7 @@ func (g Graph) GetAllDrivesV1Beta1(w http.ResponseWriter, r *http.Request) {
}

// 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) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

logger := g.logger.SubloggerWithRequestID(r.Context())
logger.Info().
Interface("query", r.URL.Query()).
Expand All @@ -197,6 +193,10 @@ func (g Graph) getDrives(r *http.Request, unrestricted bool, apiVersion APIVersi
logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get drives: query error")
return nil, errorcode.New(errorcode.InvalidRequest, err.Error())
}

if !g.validateQueryMFA(r, w, odataReq.Query, DriveTypeValidator{}) {
return nil, errorcode.New(errorcode.AccessDenied, "")
}
ctx := r.Context()

filters, err := generateCs3Filters(odataReq)
Expand Down Expand Up @@ -409,6 +409,8 @@ func (g Graph) createDrive(w http.ResponseWriter, r *http.Request, apiVersion AP
switch driveType {
case "", _spaceTypeProject:
driveType = _spaceTypeProject
case _spaceTypeProtectedProject:
driveType = _spaceTypeProtectedProject
default:
logger.Debug().Str("type", driveType).Msg("could not create drive: drives of this type cannot be created via this api")
errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "drives of this type cannot be created via this api")
Expand Down Expand Up @@ -468,7 +470,7 @@ func (g Graph) createDrive(w http.ResponseWriter, r *http.Request, apiVersion AP
}

space := resp.GetStorageSpace()
if t := r.URL.Query().Get(TemplateParameter); t != "" && driveType == _spaceTypeProject {
if t := r.URL.Query().Get(TemplateParameter); t != "" && (driveType == _spaceTypeProject || driveType == _spaceTypeProtectedProject) {
loc := l10n.MustGetUserLocale(ctx, us.GetId().GetOpaqueId(), r.Header.Get(HeaderAcceptLanguage), g.valueService)
if err := g.applySpaceTemplate(ctx, gatewayClient, space.GetRoot(), t, loc); err != nil {
logger.Error().Err(err).Msg("could not apply template to space")
Expand Down Expand Up @@ -596,7 +598,7 @@ func (g Graph) updateDrive(w http.ResponseWriter, r *http.Request, apiVersion AP
for _, sp := range res.StorageSpaces {
id, _ := storagespace.ParseID(sp.GetId().GetOpaqueId())
if id.GetSpaceId() == rid.GetSpaceId() {
dt = _spaceTypeProject
dt = sp.SpaceType
}
}
}
Expand Down Expand Up @@ -781,7 +783,25 @@ func (g Graph) ListStorageSpacesWithFilters(ctx context.Context, filters []*stor
return nil, err
}
res, err := gatewayClient.ListStorageSpaces(ctx, lReq)
return res, err
if err != nil {
return nil, err
}
if res.GetStatus().GetCode() != cs3rpc.Code_CODE_OK {
return res, nil
}

// Filter out protected spaces if MFA is not enabled
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

if !mfa.Has(ctx) {
var filtered []*storageprovider.StorageSpace
for _, s := range res.StorageSpaces {
if s.SpaceType != _spaceTypeProtectedPersonal && s.SpaceType != _spaceTypeProtectedProject {
filtered = append(filtered, s)
}
}
res.StorageSpaces = filtered
}

return res, nil
}

func (g Graph) cs3StorageSpaceToDrive(ctx context.Context, baseURL *url.URL, space *storageprovider.StorageSpace, apiVersion APIVersion) (*libregraph.Drive, error) {
Expand Down Expand Up @@ -1016,9 +1036,12 @@ func getQuota(quota *libregraph.Quota, defaultQuota string) *storageprovider.Quo

func (g Graph) canSetSpaceQuota(ctx context.Context, _ *userv1beta1.User, typ string) (bool, error) {
permID := settingsServiceExt.SetPersonalSpaceQuotaPermission(0).Id
if typ == _spaceTypeProject {
if typ == _spaceTypeProject || typ == _spaceTypeProtectedProject {
permID = settingsServiceExt.SetProjectSpaceQuotaPermission(0).Id
}
if typ == _spaceTypeProtectedPersonal {
permID = settingsServiceExt.SetPersonalSpaceQuotaPermission(0).Id
}
_, err := g.permissionsService.GetPermissionByID(ctx, &settingssvc.GetPermissionByIDRequest{PermissionId: permID})
if err != nil {
merror := merrors.FromError(err)
Expand Down
124 changes: 111 additions & 13 deletions services/graph/pkg/service/v0/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/owncloud/reva/v2/pkg/storagespace"

"github.com/owncloud/ocis/v2/ocis-pkg/keycloak"
"github.com/owncloud/ocis/v2/ocis-pkg/mfa"
ehsvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/eventhistory/v0"
searchsvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/search/v0"
settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0"
Expand Down Expand Up @@ -154,38 +155,135 @@ func parseIDParam(r *http.Request, param string) (storageprovider.ResourceId, er
return id, nil
}

// regular users can only search for terms with a minimum length
func hasAcceptableSearch(query *godata.GoDataQuery, minSearchLength int) bool {
// QueryValidator defines a strategy for validating godata queries.
type QueryValidator interface {
Validate(query *godata.GoDataQuery) error
}

// SearchValidator validates the search query.
type SearchValidator struct {
MinLength int
}

// Validate checks if the search term is acceptable.
func (v SearchValidator) Validate(query *godata.GoDataQuery) error {
if query == nil || query.Search == nil {
return false
return errors.New("search term too short")
}

minSearchLength := v.MinLength
if strings.HasPrefix(query.Search.RawValue, "\"") {
// if search starts with double quotes then it must finish with double quotes
// add +2 to the minimum search length in this case
minSearchLength += 2
}

return len(query.Search.RawValue) >= minSearchLength
if len(query.Search.RawValue) < minSearchLength {
return errors.New("search term too short")
}
return nil
}

// regular users can only filter by userType
func hasAcceptableFilter(query *godata.GoDataQuery) bool {
// FilterValidator validates the filter query.
type FilterValidator struct{}

// Validate checks if the filter applies forbidden elements.
func (v FilterValidator) Validate(query *godata.GoDataQuery) error {
switch {
case query == nil || query.Filter == nil:
return true
return nil
case query.Filter.Tree.Token.Type != godata.ExpressionTokenLogical:
return false
return errors.New("filter has forbidden elements for regular users")
case query.Filter.Tree.Token.Value != "eq":
return false
return errors.New("filter has forbidden elements for regular users")
case query.Filter.Tree.Children[0].Token.Value != "userType":
return false
return errors.New("filter has forbidden elements for regular users")
}

return nil
}

// BasicQueryValidator validates basic queries.
type BasicQueryValidator struct{}

// Validate checks if the query contains unsupported operations like apply, expand, or compute.
func (v BasicQueryValidator) Validate(query *godata.GoDataQuery) error {
if query != nil && (query.Apply != nil || query.Expand != nil || query.Compute != nil) {
return errors.New("query has forbidden elements for regular users")
}
return nil
}

// DriveTypeValidator validates if the driveType filter contains protected types.
type DriveTypeValidator struct{}

// Validate checks if the driveType filter contains protected types.
func (v DriveTypeValidator) Validate(query *godata.GoDataQuery) error {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

if query == nil || query.Filter == nil {
return nil
}

if query.Filter.Tree.Token.Value == "eq" && query.Filter.Tree.Children[0].Token.Value == "driveType" {
driveType := strings.Trim(query.Filter.Tree.Children[1].Token.Value, "'")
if driveType == _spaceTypeProtectedProject || driveType == _spaceTypeProtectedPersonal {
return errors.New("mfa required for protected spaces")
}
}

return nil
}

// validateQuery validates the godata query based on provided validators.
// It handles user permissions and MFA requirements centrally.
func (g Graph) validateQuery(r *http.Request, w http.ResponseWriter, query *godata.GoDataQuery, validators ...QueryValidator) bool {
ctxHasFullPerms := g.contextUserHasFullAccountPerms(r.Context())
hasMFA := mfa.Has(r.Context())
logger := g.logger.SubloggerWithRequestID(r.Context())

for _, v := range validators {
if err := v.Validate(query); err != nil {
if !ctxHasFullPerms {
logger.Debug().Interface("query", r.URL.Query()).Msg(err.Error())
errorcode.AccessDenied.Render(w, r, http.StatusForbidden, err.Error())
return false
}
if !hasMFA {
logger.Error().Str("path", r.URL.Path).Msg("MFA required but not satisfied")
mfa.SetRequiredStatus(w)
return false
}
}
}
return true
}

// validateQueryMFA validates the godata query based on provided validators.
// It only handles MFA requirements and does not check for user permissions.
func (g Graph) validateQueryMFA(r *http.Request, w http.ResponseWriter, query *godata.GoDataQuery, validators ...QueryValidator) bool {
hasMFA := mfa.Has(r.Context())
logger := g.logger.SubloggerWithRequestID(r.Context())

for _, v := range validators {
if err := v.Validate(query); err != nil {
if !hasMFA {
logger.Error().Str("path", r.URL.Path).Msg("MFA required but not satisfied")
mfa.SetRequiredStatus(w)
return false
}
}
}
return true
}

// regular users can only use basic queries without any expansions, computes or applies
func hasAcceptableQuery(query *godata.GoDataQuery) bool {
return query != nil && query.Apply == nil && query.Expand == nil && query.Compute == nil
// validateMFA validates the MFA requirement.
func (g Graph) validateMFA(r *http.Request, w http.ResponseWriter) bool {
hasMFA := mfa.Has(r.Context())
logger := g.logger.SubloggerWithRequestID(r.Context())

if !hasMFA {
logger.Error().Str("path", r.URL.Path).Msg("MFA required but not satisfied")
mfa.SetRequiredStatus(w)
return false
}
return true
}
Loading