From d163c8ed29d46c0c70b343079dfedbb081fa73d9 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 11 Jun 2026 14:31:44 +0200 Subject: [PATCH] fix(graph): translate sharing roles consitently GetRoleDefinition/s does now handle l10n correctly. Previsouly it just returned the non-localized string. What made things worse was that ListPermissions() mutated global list of available roles and replaced some strings with translated values depending on the `accept-language` header. Which resulted in GetRoleDefinition returning results in mixed localization depending on who/what called ListPermissions before. Fixes: #2800 --- .../service/v0/api_driveitem_permissions.go | 31 +--- .../graph/pkg/service/v0/rolemanagement.go | 9 +- .../pkg/service/v0/rolemanagement_test.go | 170 ++++++++++++++++++ services/graph/pkg/unifiedrole/roles.go | 44 ++++- services/graph/pkg/unifiedrole/roles_test.go | 82 +++++++++ 5 files changed, 307 insertions(+), 29 deletions(-) create mode 100644 services/graph/pkg/service/v0/rolemanagement_test.go diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions.go b/services/graph/pkg/service/v0/api_driveitem_permissions.go index 70ee8c236d..63d9ec49af 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions.go @@ -22,7 +22,6 @@ import ( "github.com/go-chi/chi/v5" "github.com/go-chi/render" libregraph "github.com/opencloud-eu/libre-graph-api-go" - revactx "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/publicshare" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" @@ -30,16 +29,14 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/storagespace" "github.com/opencloud-eu/reva/v2/pkg/utils" - "github.com/opencloud-eu/opencloud/pkg/l10n" - l10n_pkg "github.com/opencloud-eu/opencloud/services/graph/pkg/l10n" - "github.com/opencloud-eu/opencloud/services/graph/pkg/odata" - "github.com/opencloud-eu/opencloud/pkg/conversions" + "github.com/opencloud-eu/opencloud/pkg/l10n" "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/graph/pkg/config" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" "github.com/opencloud-eu/opencloud/services/graph/pkg/identity" "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/cache" + "github.com/opencloud-eu/opencloud/services/graph/pkg/odata" "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" "github.com/opencloud-eu/opencloud/services/graph/pkg/validate" ) @@ -755,16 +752,8 @@ func (api DriveItemPermissionsApi) ListPermissions(w http.ResponseWriter, r *htt loc := r.Header.Get(l10n.HeaderAcceptLanguage) w.Header().Add("Content-Language", loc) - if loc != "" && loc != "en" { - err := l10n_pkg.TranslateEntity(loc, "en", permissions, - l10n.TranslateEach("LibreGraphPermissionsRolesAllowedValues", - l10n.TranslateField("Description"), - l10n.TranslateField("DisplayName"), - ), - ) - if err != nil { - api.logger.Error().Err(err).Msg("tranlation error") - } + for i := range permissions.LibreGraphPermissionsRolesAllowedValues { + permissions.LibreGraphPermissionsRolesAllowedValues[i] = unifiedrole.LocalizeRole(&permissions.LibreGraphPermissionsRolesAllowedValues[i], loc) } render.Status(r, http.StatusOK) @@ -806,16 +795,8 @@ func (api DriveItemPermissionsApi) ListSpaceRootPermissions(w http.ResponseWrite loc := r.Header.Get(l10n.HeaderAcceptLanguage) w.Header().Add("Content-Language", loc) - if loc != "" && loc != "en" { - err := l10n_pkg.TranslateEntity(loc, "en", permissions, - l10n.TranslateEach("LibreGraphPermissionsRolesAllowedValues", - l10n.TranslateField("Description"), - l10n.TranslateField("DisplayName"), - ), - ) - if err != nil { - api.logger.Error().Err(err).Msg("tranlation error") - } + for i := range permissions.LibreGraphPermissionsRolesAllowedValues { + permissions.LibreGraphPermissionsRolesAllowedValues[i] = unifiedrole.LocalizeRole(&permissions.LibreGraphPermissionsRolesAllowedValues[i], loc) } render.Status(r, http.StatusOK) diff --git a/services/graph/pkg/service/v0/rolemanagement.go b/services/graph/pkg/service/v0/rolemanagement.go index d5121e7140..0755d18c67 100644 --- a/services/graph/pkg/service/v0/rolemanagement.go +++ b/services/graph/pkg/service/v0/rolemanagement.go @@ -7,14 +7,17 @@ import ( "github.com/go-chi/chi/v5" "github.com/go-chi/render" + "github.com/opencloud-eu/opencloud/pkg/l10n" "github.com/opencloud-eu/opencloud/services/graph/pkg/errorcode" "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" ) // GetRoleDefinitions a list of permission roles than can be used when sharing with users or groups func (g Graph) GetRoleDefinitions(w http.ResponseWriter, r *http.Request) { + loc := r.Header.Get(l10n.HeaderAcceptLanguage) + w.Header().Add("Content-Language", loc) render.Status(r, http.StatusOK) - render.JSON(w, r, g.availableRoles) + render.JSON(w, r, unifiedrole.LocalizeRoles(g.availableRoles, loc)) } // GetRoleDefinition a permission role than can be used when sharing with users or groups @@ -32,6 +35,8 @@ func (g Graph) GetRoleDefinition(w http.ResponseWriter, r *http.Request) { errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, err.Error()) return } + loc := r.Header.Get(l10n.HeaderAcceptLanguage) + w.Header().Add("Content-Language", loc) render.Status(r, http.StatusOK) - render.JSON(w, r, role) + render.JSON(w, r, unifiedrole.LocalizeRole(role, loc)) } diff --git a/services/graph/pkg/service/v0/rolemanagement_test.go b/services/graph/pkg/service/v0/rolemanagement_test.go new file mode 100644 index 0000000000..fcb77f7c6b --- /dev/null +++ b/services/graph/pkg/service/v0/rolemanagement_test.go @@ -0,0 +1,170 @@ +package svc_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + libregraph "github.com/opencloud-eu/libre-graph-api-go" + "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" + cs3mocks "github.com/opencloud-eu/reva/v2/tests/cs3mocks/mocks" + "google.golang.org/grpc" + + "github.com/opencloud-eu/opencloud/pkg/shared" + "github.com/opencloud-eu/opencloud/services/graph/mocks" + "github.com/opencloud-eu/opencloud/services/graph/pkg/config" + "github.com/opencloud-eu/opencloud/services/graph/pkg/config/defaults" + service "github.com/opencloud-eu/opencloud/services/graph/pkg/service/v0" + "github.com/opencloud-eu/opencloud/services/graph/pkg/unifiedrole" +) + +var _ = Describe("RoleManagement", func() { + var ( + svc service.Service + gatewayClient *cs3mocks.GatewayAPIClient + gatewaySelector pool.Selectable[gateway.GatewayAPIClient] + eventsPublisher mocks.Publisher + permSvc mocks.Permissions + cfg *config.Config + rr *httptest.ResponseRecorder + ctx context.Context + ) + + BeforeEach(func() { + rr = httptest.NewRecorder() + ctx = context.Background() + + cfg = defaults.FullDefaultConfig() + cfg.Identity.LDAP.CACert = "" + cfg.TokenManager.JWTSecret = "loremipsum" + cfg.Commons = &shared.Commons{} + cfg.GRPCClientTLS = &shared.GRPCClientTLS{} + + pool.RemoveSelector("GatewaySelector" + "eu.opencloud.api.gateway") + gatewayClient = &cs3mocks.GatewayAPIClient{} + gatewaySelector = pool.GetSelector[gateway.GatewayAPIClient]( + "GatewaySelector", + "eu.opencloud.api.gateway", + func(cc grpc.ClientConnInterface) gateway.GatewayAPIClient { + return gatewayClient + }, + ) + eventsPublisher = mocks.Publisher{} + permSvc = mocks.Permissions{} + + var err error + svc, err = service.NewService( + service.Config(cfg), + service.WithGatewaySelector(gatewaySelector), + service.EventsPublisher(&eventsPublisher), + service.PermissionService(&permSvc), + ) + Expect(err).ToNot(HaveOccurred()) + }) + + Describe("GetRoleDefinitions", func() { + It("returns all available roles in English when no Accept-Language is set", func() { + r := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/roleManagement/permissions/roleDefinitions", nil) + r = r.WithContext(ctx) + svc.ServeHTTP(rr, r) + + Expect(rr.Code).To(Equal(http.StatusOK)) + + var roles []libregraph.UnifiedRoleDefinition + Expect(json.Unmarshal(rr.Body.Bytes(), &roles)).To(Succeed()) + Expect(roles).NotTo(BeEmpty()) + + viewer := findRoleByID(roles, unifiedrole.UnifiedRoleViewerID) + Expect(viewer).NotTo(BeNil()) + Expect(viewer.GetDisplayName()).To(Equal("Can view")) + }) + + It("returns translated roles when Accept-Language is German", func() { + r := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/roleManagement/permissions/roleDefinitions", nil) + r.Header.Set("Accept-Language", "de") + r = r.WithContext(ctx) + svc.ServeHTTP(rr, r) + + Expect(rr.Code).To(Equal(http.StatusOK)) + Expect(rr.Header().Get("Content-Language")).To(Equal("de")) + + var roles []libregraph.UnifiedRoleDefinition + Expect(json.Unmarshal(rr.Body.Bytes(), &roles)).To(Succeed()) + + viewer := findRoleByID(roles, unifiedrole.UnifiedRoleViewerID) + Expect(viewer).NotTo(BeNil()) + Expect(viewer.GetDisplayName()).To(Equal("Kann anzeigen")) + Expect(viewer.GetDescription()).To(Equal("Ansehen und herunterladen.")) + }) + + It("does not mutate the global buildInRoles after a German request", func() { + r := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/roleManagement/permissions/roleDefinitions", nil) + r.Header.Set("Accept-Language", "de") + r = r.WithContext(ctx) + svc.ServeHTTP(rr, r) + Expect(rr.Code).To(Equal(http.StatusOK)) + + // A second request without a locale must still return English + rr2 := httptest.NewRecorder() + r2 := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/roleManagement/permissions/roleDefinitions", nil) + r2 = r2.WithContext(ctx) + svc.ServeHTTP(rr2, r2) + Expect(rr2.Code).To(Equal(http.StatusOK)) + + var roles []libregraph.UnifiedRoleDefinition + Expect(json.Unmarshal(rr2.Body.Bytes(), &roles)).To(Succeed()) + viewer := findRoleByID(roles, unifiedrole.UnifiedRoleViewerID) + Expect(viewer).NotTo(BeNil()) + Expect(viewer.GetDisplayName()).To(Equal("Can view")) + }) + }) + + Describe("GetRoleDefinition", func() { + It("returns a single role in English when no Accept-Language is set", func() { + r := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/roleManagement/permissions/roleDefinitions/"+unifiedrole.UnifiedRoleViewerID, nil) + r = r.WithContext(ctx) + svc.ServeHTTP(rr, r) + + Expect(rr.Code).To(Equal(http.StatusOK)) + var role libregraph.UnifiedRoleDefinition + Expect(json.Unmarshal(rr.Body.Bytes(), &role)).To(Succeed()) + Expect(role.GetDisplayName()).To(Equal("Can view")) + }) + + It("returns a single role translated when Accept-Language is German", func() { + r := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/roleManagement/permissions/roleDefinitions/"+unifiedrole.UnifiedRoleViewerID, nil) + r.Header.Set("Accept-Language", "de") + r = r.WithContext(ctx) + svc.ServeHTTP(rr, r) + + Expect(rr.Code).To(Equal(http.StatusOK)) + Expect(rr.Header().Get("Content-Language")).To(Equal("de")) + var role libregraph.UnifiedRoleDefinition + Expect(json.Unmarshal(rr.Body.Bytes(), &role)).To(Succeed()) + Expect(role.GetDisplayName()).To(Equal("Kann anzeigen")) + Expect(role.GetDescription()).To(Equal("Ansehen und herunterladen.")) + }) + + It("returns 404 for an unknown roleID", func() { + r := httptest.NewRequest(http.MethodGet, "/graph/v1beta1/roleManagement/permissions/roleDefinitions/unknown-role-id", nil) + r = r.WithContext(ctx) + svc.ServeHTTP(rr, r) + + Expect(rr.Code).To(Equal(http.StatusNotFound)) + }) + }) +}) + +// findRoleByID returns the first role with the given ID from the slice, or nil. +func findRoleByID(roles []libregraph.UnifiedRoleDefinition, id string) *libregraph.UnifiedRoleDefinition { + for i := range roles { + if roles[i].GetId() == id { + return &roles[i] + } + } + return nil +} diff --git a/services/graph/pkg/unifiedrole/roles.go b/services/graph/pkg/unifiedrole/roles.go index 5b866e70ef..b076aef28e 100644 --- a/services/graph/pkg/unifiedrole/roles.go +++ b/services/graph/pkg/unifiedrole/roles.go @@ -6,11 +6,11 @@ import ( "strings" libregraph "github.com/opencloud-eu/libre-graph-api-go" + "github.com/opencloud-eu/reva/v2/pkg/conversions" "google.golang.org/protobuf/proto" - "github.com/opencloud-eu/reva/v2/pkg/conversions" - "github.com/opencloud-eu/opencloud/pkg/l10n" + graphl10n "github.com/opencloud-eu/opencloud/services/graph/pkg/l10n" ) const ( @@ -546,6 +546,46 @@ func weightRoles(roleSet []*libregraph.UnifiedRoleDefinition, constraints string return roleSet } +// cloneRole returns a shallow struct copy of r with independent allocations for +// the Description and DisplayName pointer fields — the only fields mutated by +// TranslateEntity. All other fields (Id, LibreGraphWeight, RolePermissions) are +// either read-only or stripped before rendering, so sharing their values is safe. +func cloneRole(r *libregraph.UnifiedRoleDefinition) libregraph.UnifiedRoleDefinition { + c := *r + if r.Description != nil { + s := *r.Description + c.Description = &s + } + if r.DisplayName != nil { + s := *r.DisplayName + c.DisplayName = &s + } + return c +} + +// LocalizeRole returns a translated, independent copy of a single role definition. +// The global buildInRoles singleton is never modified. +func LocalizeRole(r *libregraph.UnifiedRoleDefinition, locale string) libregraph.UnifiedRoleDefinition { + c := cloneRole(r) + if locale != "" && locale != "en" { + _ = graphl10n.TranslateEntity(locale, "en", &c, + l10n.TranslateField("Description"), + l10n.TranslateField("DisplayName"), + ) + } + return c +} + +// LocalizeRoles returns a translated, independent copy of each role definition. +// The global buildInRoles singleton is never modified. +func LocalizeRoles(roles []*libregraph.UnifiedRoleDefinition, locale string) []libregraph.UnifiedRoleDefinition { + out := make([]libregraph.UnifiedRoleDefinition, len(roles)) + for i, r := range roles { + out[i] = LocalizeRole(r, locale) + } + return out +} + // GetAllowedResourceActions returns the allowed resource actions for the provided role by condition func GetAllowedResourceActions(role *libregraph.UnifiedRoleDefinition, condition string) []string { if role == nil { diff --git a/services/graph/pkg/unifiedrole/roles_test.go b/services/graph/pkg/unifiedrole/roles_test.go index aae4733a4a..104e5660e6 100644 --- a/services/graph/pkg/unifiedrole/roles_test.go +++ b/services/graph/pkg/unifiedrole/roles_test.go @@ -2,6 +2,7 @@ package unifiedrole_test import ( "slices" + "sync" "testing" . "github.com/onsi/gomega" @@ -267,3 +268,84 @@ func TestGetAllowedResourceActions(t *testing.T) { }) } } + +func TestLocalizeRole_English(t *testing.T) { + g := NewWithT(t) + original := unifiedrole.RoleViewer + + result := unifiedrole.LocalizeRole(original, "en") + + // Strings are unchanged for English + g.Expect(result.GetDisplayName()).To(Equal(original.GetDisplayName())) + g.Expect(result.GetDescription()).To(Equal(original.GetDescription())) + + // Result is an independent copy — mutating it must not touch the global + translated := "mutated" + result.DisplayName = &translated + g.Expect(original.GetDisplayName()).NotTo(Equal("mutated")) +} + +func TestLocalizeRole_German(t *testing.T) { + g := NewWithT(t) + original := unifiedrole.RoleViewer + + result := unifiedrole.LocalizeRole(original, "de") + + g.Expect(result.GetDisplayName()).To(Equal("Kann anzeigen")) + g.Expect(result.GetDescription()).To(Equal("Ansehen und herunterladen.")) + + // Global singleton must be untouched + g.Expect(original.GetDisplayName()).NotTo(Equal("Kann anzeigen")) + g.Expect(original.GetDescription()).NotTo(Equal("Ansehen und herunterladen.")) +} + +func TestLocalizeRole_EmptyLocale(t *testing.T) { + g := NewWithT(t) + original := unifiedrole.RoleViewer + + result := unifiedrole.LocalizeRole(original, "") + + // Empty locale falls back to source strings + g.Expect(result.GetDisplayName()).To(Equal(original.GetDisplayName())) + g.Expect(result.GetDescription()).To(Equal(original.GetDescription())) +} + +func TestLocalizeRoles_German(t *testing.T) { + g := NewWithT(t) + roles := unifiedrole.BuildInRoles + + results := unifiedrole.LocalizeRoles(roles, "de") + + g.Expect(results).To(HaveLen(len(roles))) + + // Every result is a value (not a pointer) + for i, r := range results { + // Id is preserved + g.Expect(r.GetId()).To(Equal(roles[i].GetId())) + // Global singleton is not mutated + g.Expect(roles[i].GetDisplayName()).NotTo(Equal(r.GetDisplayName()), + "global displayName for role %s was mutated", r.GetId()) + } +} + +func TestLocalizeRole_ConcurrentCallsDoNotRace(t *testing.T) { + // Run with -race to detect data races on the global buildInRoles strings. + const goroutines = 20 + var wg sync.WaitGroup + wg.Add(goroutines) + for i := 0; i < goroutines; i++ { + locale := "de" + if i%2 == 0 { + locale = "fr" + } + go func(loc string) { + defer wg.Done() + _ = unifiedrole.LocalizeRoles(unifiedrole.BuildInRoles, loc) + }(locale) + } + wg.Wait() + + // After all concurrent translations the globals must still hold English strings + g := NewWithT(t) + g.Expect(unifiedrole.RoleViewer.GetDisplayName()).To(Equal("Can view")) +}