Skip to content

Commit

Permalink
fix enforce-password issue for public link ocis#issue-6476
Browse files Browse the repository at this point in the history
  • Loading branch information
@2403905
2403905 committed Jun 20, 2023
1 parent 9bd3d73 commit 6f01bde
Show file tree
Hide file tree
Showing 14 changed files with 537 additions and 58 deletions.
6 changes: 6 additions & 0 deletions changelog/unreleased/fix-enforce-password.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Bugfix: fix enforce-password issue

Fix updating public share without password when enforce-password is enabled

https://github.com/cs3org/reva/pull/3970
https://github.com/owncloud/ocis/issues/6476
1 change: 0 additions & 1 deletion changelog/unreleased/fix-panic.md
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
Bugfix: fix panic

https://github.com/cs3org/reva/pull/3955

83 changes: 64 additions & 19 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/public.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,21 @@ import (
"fmt"
"net/http"
"strconv"
"strings"

permissionsv1beta1 "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1"
rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1"
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
"github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool"
"github.com/rs/zerolog/log"

"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocs/conversions"
"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocs/response"
"github.com/cs3org/reva/v2/pkg/appctx"
ctxpkg "github.com/cs3org/reva/v2/pkg/ctx"
"github.com/cs3org/reva/v2/pkg/publicshare"
"github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -127,24 +128,26 @@ func (h *Handler) createPublicLinkShare(w http.ResponseWriter, r *http.Request,
}
}

permissions, err := ocPublicPermToCs3(permKey, h)
// default perms: read-only
// TODO: the default might change depending on allowed permissions and configs
if permKey == nil {
permKey = &_defaultPublicLinkPermission
}
permissions, err := ocPublicPermToCs3(permKey)
if err != nil {
return nil, &ocsError{
Code: response.MetaBadRequest.StatusCode,
Message: "Could not create permission from permission key",
Error: err,
}
}
if permissions == nil {
// default perms: read-only
// TODO: the default might change depending on allowed permissions and configs
permissions, err = ocPublicPermToCs3(&_defaultPublicLinkPermission, h)
if err != nil {
return nil, &ocsError{
Code: response.MetaServerError.StatusCode,
Message: "Could not convert default permissions",
Error: err,
}

password := strings.TrimSpace(r.FormValue("password"))
if h.enforcePassword(permKey) && len(password) == 0 {
return nil, &ocsError{
Code: response.MetaBadRequest.StatusCode,
Message: "missing required password",
Error: errors.New("missing required password"),
}
}

Expand Down Expand Up @@ -172,7 +175,7 @@ func (h *Handler) createPublicLinkShare(w http.ResponseWriter, r *http.Request,
Permissions: &link.PublicSharePermissions{
Permissions: permissions,
},
Password: r.FormValue("password"),
Password: password,
},
}

Expand All @@ -198,7 +201,7 @@ func (h *Handler) createPublicLinkShare(w http.ResponseWriter, r *http.Request,
Metadata: map[string]string{
"name": r.FormValue("name"),
"quicklink": r.FormValue("quicklink"),
// "password": r.FormValue("password"),
// "password": password,
},
}

Expand Down Expand Up @@ -275,9 +278,10 @@ func (h *Handler) listPublicShares(r *http.Request, filters []*link.ListPublicSh

func (h *Handler) isPublicShare(r *http.Request, oid string) (*link.PublicShare, bool) {
logger := appctx.GetLogger(r.Context())
client, err := pool.GetGatewayServiceClient(h.gatewayAddr)
client, err := h.getClient()
if err != nil {
logger.Err(err)
return nil, false
}

psRes, err := client.GetPublicShare(r.Context(), &link.GetPublicShareRequest{
Expand All @@ -301,7 +305,7 @@ func (h *Handler) updatePublicShare(w http.ResponseWriter, r *http.Request, shar
updates := []*link.UpdatePublicShareRequest_Update{}
logger := appctx.GetLogger(r.Context())

gwC, err := pool.GetGatewayServiceClient(h.gatewayAddr)
gwC, err := h.getClient()
if err != nil {
log.Err(err).Str("shareID", share.GetId().GetOpaqueId()).Msg("updatePublicShare")
response.WriteOCSError(w, r, response.MetaBadRequest.StatusCode, "error getting a connection to the gateway service", nil)
Expand Down Expand Up @@ -377,7 +381,7 @@ func (h *Handler) updatePublicShare(w http.ResponseWriter, r *http.Request, shar
}

// Permissions
newPermissions, err := ocPublicPermToCs3(permKey, h)
newPermissions, err := ocPublicPermToCs3(permKey)
logger.Debug().Interface("newPermissions", newPermissions).Msg("Parsed permissions")
if err != nil {
response.WriteOCSError(w, r, response.MetaBadRequest.StatusCode, "invalid permissions", err)
Expand Down Expand Up @@ -447,6 +451,14 @@ func (h *Handler) updatePublicShare(w http.ResponseWriter, r *http.Request, shar

// Password
newPassword, ok := r.Form["password"]
// enforcePassword
if h.enforcePassword(permKey) {
if (!ok && !share.PasswordProtected) || (ok && len(strings.TrimSpace(newPassword[0])) == 0) {
response.WriteOCSError(w, r, response.MetaBadRequest.StatusCode, "missing required password", err)
return
}
}

// update or clear password
if ok {
updatesFound = true
Expand Down Expand Up @@ -500,7 +512,7 @@ func (h *Handler) updatePublicShare(w http.ResponseWriter, r *http.Request, shar
func (h *Handler) removePublicShare(w http.ResponseWriter, r *http.Request, share *link.PublicShare) {
ctx := r.Context()

c, err := pool.GetGatewayServiceClient(h.gatewayAddr)
c, err := h.getClient()
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error getting grpc gateway client", err)
return
Expand Down Expand Up @@ -548,6 +560,39 @@ func (h *Handler) removePublicShare(w http.ResponseWriter, r *http.Request, shar
response.WriteOCSSuccess(w, r, nil)
}

// enforcePassword validate Password enforce based on configuration
// read_only: 1
// read_write: 3 or 5
// read_write_delete: 15
// upload_only: 4
func (h *Handler) enforcePassword(pk *int) bool {
if pk == nil {
return false
}
p, err := conversions.NewPermissions(decreasePermissionsIfNecessary(*pk))
if err != nil {
return false
}
if h.publicPasswordEnforced.EnforcedForReadOnly &&
p == conversions.PermissionRead {
return true
}
if h.publicPasswordEnforced.EnforcedForReadWrite &&
(p == conversions.PermissionRead|conversions.PermissionWrite ||
p == conversions.PermissionRead|conversions.PermissionCreate) {
return true
}
if h.publicPasswordEnforced.EnforcedForReadWriteDelete &&
p == conversions.PermissionRead|conversions.PermissionWrite|conversions.PermissionCreate|conversions.PermissionDelete {
return true
}
if h.publicPasswordEnforced.EnforcedForUploadOnly &&
p == conversions.PermissionCreate {
return true
}
return false
}

// for public links oc10 api decreases all permissions to read: stay compatible!
func decreasePermissionsIfNecessary(perm int) int {
if perm == int(conversions.PermissionAll) {
Expand All @@ -556,7 +601,7 @@ func decreasePermissionsIfNecessary(perm int) int {
return perm
}

func ocPublicPermToCs3(pk *int, h *Handler) (*provider.ResourcePermissions, error) {
func ocPublicPermToCs3(pk *int) (*provider.ResourcePermissions, error) {
if pk == nil {
return nil, nil
}
Expand Down
103 changes: 103 additions & 0 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/public_test.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
package shares

import (
"testing"
)

func TestHandler_enforcePassword(t *testing.T) {
tests := []struct {
name string
h *Handler
permKey int
exp bool
}{
{
name: "enforce permission 1",
h: &Handler{
publicPasswordEnforced: passwordEnforced{
EnforcedForReadOnly: true,
},
},
permKey: 1,
exp: true,
},
{
name: "enforce permission 3",
h: &Handler{
publicPasswordEnforced: passwordEnforced{
EnforcedForReadWrite: true,
},
},
permKey: 3,
exp: true,
},
{
name: "enforce permission 4",
h: &Handler{
publicPasswordEnforced: passwordEnforced{
EnforcedForUploadOnly: true,
},
},
permKey: 4,
exp: true,
},
{
name: "enforce permission 5",
h: &Handler{
publicPasswordEnforced: passwordEnforced{
EnforcedForReadWrite: true,
},
},
permKey: 5,
exp: true,
},
{
name: "enforce permission 15",
h: &Handler{
publicPasswordEnforced: passwordEnforced{
EnforcedForReadWriteDelete: true,
},
},
permKey: 15,
exp: true,
},
{
name: "enforce permission 3 failed",
h: &Handler{
publicPasswordEnforced: passwordEnforced{
EnforcedForReadOnly: true,
},
},
permKey: 3,
exp: false,
},
{
name: "enforce permission 8 failed",
h: &Handler{
publicPasswordEnforced: passwordEnforced{
EnforcedForReadWriteDelete: true,
EnforcedForUploadOnly: true,
},
},
permKey: 8,
exp: false,
},
{
name: "enforce permission 11 failed",
h: &Handler{
publicPasswordEnforced: passwordEnforced{
EnforcedForReadWriteDelete: true,
},
},
permKey: 11,
exp: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.h.enforcePassword(&tt.permKey) != tt.exp {
t.Errorf("enforcePassword(\"%v\") returned %v instead of expected %v", tt.permKey, tt.h.enforcePassword(&tt.permKey), tt.exp)
}
})
}
}
34 changes: 30 additions & 4 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ type Handler struct {
statCache cache.StatCache
deniable bool
resharing bool
publicPasswordEnforced passwordEnforced

getClient GatewayClientGetter
}
Expand All @@ -103,6 +104,13 @@ type ocsError struct {
Message string
}

type passwordEnforced struct {
EnforcedForReadOnly bool
EnforcedForReadWrite bool
EnforcedForReadWriteDelete bool
EnforcedForUploadOnly bool
}

func getCacheWarmupManager(c *config.Config) (sharecache.Warmup, error) {
if f, ok := warmupreg.NewFuncs[c.CacheWarmupDriver]; ok {
return f(c.CacheWarmupDrivers[c.CacheWarmupDriver])
Expand All @@ -129,6 +137,7 @@ func (h *Handler) Init(c *config.Config) {
_ = h.userIdentifierCache.SetTTL(time.Second * time.Duration(c.UserIdentifierCacheTTL))
h.deniable = c.EnableDenials
h.resharing = resharing(c)
h.publicPasswordEnforced = publicPwdEnforced(c)

h.statCache = cache.GetStatCache(c.StatCacheStore, c.StatCacheNodes, c.StatCacheDatabase, "stat", time.Duration(c.StatCacheTTL)*time.Second, c.StatCacheSize)
if c.CacheWarmupDriver != "" {
Expand Down Expand Up @@ -503,7 +512,7 @@ func (h *Handler) GetShare(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
sublog := appctx.GetLogger(ctx).With().Str("shareID", shareID).Logger()

client, err := pool.GetGatewayServiceClient(h.gatewayAddr)
client, err := h.getClient()
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error getting grpc gateway client", err)
return
Expand Down Expand Up @@ -695,7 +704,7 @@ func (h *Handler) updateShare(w http.ResponseWriter, r *http.Request, share *col
ctx := r.Context()
sublog := appctx.GetLogger(ctx).With().Str("shareID", share.GetId().GetOpaqueId()).Logger()

client, err := pool.GetGatewayServiceClient(h.gatewayAddr)
client, err := h.getClient()
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error getting grpc gateway client", err)
return
Expand Down Expand Up @@ -1110,7 +1119,7 @@ func (h *Handler) addFilters(w http.ResponseWriter, r *http.Request, ref *provid
ctx := r.Context()

// first check if the file exists
client, err := pool.GetGatewayServiceClient(h.gatewayAddr)
client, err := h.getClient()
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error getting grpc gateway client", err)
return nil, nil, err
Expand Down Expand Up @@ -1159,7 +1168,7 @@ func (h *Handler) addFileInfo(ctx context.Context, s *conversions.ShareData, inf
switch {
case h.sharePrefix == "/":
s.FileTarget = info.Path
client, err := pool.GetGatewayServiceClient(h.gatewayAddr)
client, err := h.getClient()
if err == nil {
gpRes, err := client.GetPath(ctx, &provider.GetPathRequest{
ResourceId: info.Id,
Expand Down Expand Up @@ -1538,6 +1547,23 @@ func (h *Handler) granteeExists(ctx context.Context, g *provider.Grantee, rid *p
return false, nil
}

func publicPwdEnforced(c *config.Config) passwordEnforced {
enf := passwordEnforced{}
if c == nil ||
c.Capabilities.Capabilities == nil ||
c.Capabilities.Capabilities.FilesSharing == nil ||
c.Capabilities.Capabilities.FilesSharing.Public == nil ||
c.Capabilities.Capabilities.FilesSharing.Public.Password == nil ||
c.Capabilities.Capabilities.FilesSharing.Public.Password.EnforcedFor == nil {
return enf
}
enf.EnforcedForReadOnly = bool(c.Capabilities.Capabilities.FilesSharing.Public.Password.EnforcedFor.ReadOnly)
enf.EnforcedForReadWrite = bool(c.Capabilities.Capabilities.FilesSharing.Public.Password.EnforcedFor.ReadWrite)
enf.EnforcedForReadWriteDelete = bool(c.Capabilities.Capabilities.FilesSharing.Public.Password.EnforcedFor.ReadWriteDelete)
enf.EnforcedForUploadOnly = bool(c.Capabilities.Capabilities.FilesSharing.Public.Password.EnforcedFor.UploadOnly)
return enf
}

// sufficientPermissions returns true if the `existing` permissions contain the `requested` permissions
func sufficientPermissions(existing, requested *provider.ResourcePermissions, islink bool) bool {
ep := conversions.RoleFromResourcePermissions(existing, islink).OCSPermissions()
Expand Down
Loading

0 comments on commit 6f01bde

Please sign in to comment.