Skip to content

Commit 71147f8

Browse files
erkaerka
committed
improve test coverage and fix bugs
Signed-off-by: Roman Dmytrenko <[email protected]>
1 parent aaa1e39 commit 71147f8

File tree

11 files changed

+270
-37
lines changed

11 files changed

+270
-37
lines changed

‎go.work.sum

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -542,6 +542,7 @@ github.com/aws/aws-sdk-go-v2 v1.27.2/go.mod h1:ffIFB97e2yNsv4aTSGkqtHnppsIJzw7G7
542542
github.com/aws/aws-sdk-go-v2 v1.30.3/go.mod h1:nIQjQVp5sfpQcTc9mPSr1B0PaWK5ByX9MOoDadSN4lc=
543543
github.com/aws/aws-sdk-go-v2 v1.30.4/go.mod h1:CT+ZPWXbYrci8chcARI3OmI/qgd+f6WtuLOoaIA8PR0=
544544
github.com/aws/aws-sdk-go-v2 v1.30.5/go.mod h1:CT+ZPWXbYrci8chcARI3OmI/qgd+f6WtuLOoaIA8PR0=
545+
github.com/aws/aws-sdk-go-v2 v1.32.5/go.mod h1:P5WJBrYqqbWVaOxgH0X/FYYD47/nooaPOZPlQdmiN2U=
545546
github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.2/go.mod h1:lPprDr1e6cJdyYeGXnRaJoP4Md+cDBvi2eOj00BlGmg=
546547
github.com/aws/aws-sdk-go-v2/config v1.27.17/go.mod h1:MzM3balLZeaafYcPz8IihAmam/aCz6niPQI0FdprxW0=
547548
github.com/aws/aws-sdk-go-v2/credentials v1.17.17/go.mod h1:e4khg9iY08LnFK/HXQDWMf9GDaiMari7jWPnXvKAuBU=
@@ -550,10 +551,12 @@ github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.9/go.mod h1:CZBXGLaJnEZ
550551
github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15/go.mod h1:U9ke74k1n2bf+RIgoX1SXFed1HLs51OgUSs+Ph0KJP8=
551552
github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.16/go.mod h1:2DwJF39FlNAUiX5pAc0UNeiz16lK2t7IaFcm0LFHEgc=
552553
github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.17/go.mod h1:Dh5zzJYMtxfIjYW+/evjQ8uj2OyR/ve2KROHGHlSFqE=
554+
github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.24/go.mod h1:5CI1JemjVwde8m2WG3cz23qHKPOxbpkq0HaoreEgLIY=
553555
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.9/go.mod h1:5jJcHuwDagxN+ErjQ3PU3ocf6Ylc/p9x+BLO/+X4iXw=
554556
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15/go.mod h1:ZQLZqhcu+JhSrA9/NXRm8SkDvsycE+JkV3WGY41e+IM=
555557
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.16/go.mod h1:7ZfEPZxkW42Afq4uQB8H2E2e6ebh6mXTueEpYzjCzcs=
556558
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.17/go.mod h1:aLJpZlCmjE+V+KtN1q1uyZkfnUWpQGpbsn89XPKyzfU=
559+
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.24/go.mod h1:dCn9HbJ8+K31i8IQ8EWmWj0EiIk0+vKiHNMxTTYveAg=
557560
github.com/aws/aws-sdk-go-v2/internal/v4a v1.3.8/go.mod h1:hD5YwHLOy6k7d6kqcn3me1bFWHOtzhaXstMd6BpdB68=
558561
github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.2/go.mod h1:5CsjAbs3NlGQyZNFACh+zztPDI7fU6eW9QsxjfnuBKg=
559562
github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.4/go.mod h1:Vz1JQXliGcQktFTN/LN6uGppAIRoLBR2bMvIMP0gOjc=
@@ -562,6 +565,7 @@ github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.10/go.mod h1:g
562565
github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17/go.mod h1:RkZEx4l0EHYDJpWppMJ3nD9wZJAa8/0lq9aVC+r2UII=
563566
github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.18/go.mod h1:++NHzT+nAF7ZPrHPsA+ENvsXkOO8wEu+C6RXltAG4/c=
564567
github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.19/go.mod h1:SCWkEdRq8/7EK60NcvvQ6NXKuTcchAD4ROAsC37VEZE=
568+
github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.12.5/go.mod h1:qu/W9HXQbbQ4+1+JcZp0ZNPV31ym537ZJN+fiS7Ti8E=
565569
github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.17.8/go.mod h1:yUQPRlWqGG0lfNsmjbRWKVwgilfBtZTOFSLEYALlAig=
566570
github.com/aws/aws-sdk-go-v2/service/kms v1.29.2/go.mod h1:elLDaj+1RNl9Ovn3dB6dWLVo5WQ+VLSUMKegl7N96fY=
567571
github.com/aws/aws-sdk-go-v2/service/kms v1.31.0/go.mod h1:2snWQJQUKsbN66vAawJuOGX7dr37pfOq9hb0tZDGIqQ=

‎internal/server/authz/engine/bundle/engine.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,9 @@ func (e *Engine) Namespaces(ctx context.Context, input map[string]interface{}) (
9696
if !ok {
9797
return nil, fmt.Errorf("unexpected result type: %T", values)
9898
}
99-
var namespaces []string
100-
for _, ns := range values {
101-
namespaces = append(namespaces, fmt.Sprintf("%s", ns))
99+
namespaces := make([]string, len(values))
100+
for i, ns := range values {
101+
namespaces[i] = fmt.Sprintf("%s", ns)
102102
}
103103
return namespaces, nil
104104
}

‎internal/server/authz/engine/bundle/engine_test.go

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,5 +246,78 @@ func TestEngine_IsAllowed(t *testing.T) {
246246
})
247247
}
248248

249+
t.Run("viewable namespaces without definition", func(t *testing.T) {
250+
namespaces, err := engine.Namespaces(ctx, map[string]any{})
251+
require.Error(t, err)
252+
require.Nil(t, namespaces)
253+
})
254+
249255
assert.NoError(t, engine.Shutdown(ctx))
250256
}
257+
258+
func TestViewableNamespaces(t *testing.T) {
259+
ctx := context.Background()
260+
261+
policy, err := os.ReadFile("../testdata/viewable_namespaces.rego")
262+
require.NoError(t, err)
263+
264+
data, err := os.ReadFile("../testdata/viewable_namespaces.json")
265+
require.NoError(t, err)
266+
267+
var (
268+
server = sdktest.MustNewServer(
269+
sdktest.MockBundle("/bundles/bundle.tar.gz", map[string]string{
270+
"main.rego": string(policy),
271+
"data.json": string(data),
272+
}),
273+
)
274+
config = fmt.Sprintf(`{
275+
"services": {
276+
"test": {
277+
"url": %q
278+
}
279+
},
280+
"bundles": {
281+
"test": {
282+
"resource": "/bundles/bundle.tar.gz"
283+
}
284+
},
285+
}`, server.URL())
286+
)
287+
288+
t.Cleanup(server.Stop)
289+
290+
opa, err := sdk.New(ctx, sdk.Options{
291+
Config: strings.NewReader(config),
292+
Store: inmem.New(),
293+
Logger: ozap.Wrap(zaptest.NewLogger(t), &zap.AtomicLevel{}),
294+
})
295+
296+
require.NoError(t, err)
297+
assert.NotNil(t, opa)
298+
299+
engine := &Engine{
300+
opa: opa,
301+
logger: zaptest.NewLogger(t),
302+
}
303+
t.Cleanup(func() {
304+
assert.NoError(t, engine.Shutdown(ctx))
305+
})
306+
307+
tt := []struct {
308+
name string
309+
roles []string
310+
namespaces []string
311+
}{
312+
{"empty", []string{}, []string{}},
313+
{"devs", []string{"devs"}, []string{"local", "staging"}},
314+
{"devsops", []string{"devs", "ops"}, []string{"local", "production", "staging"}},
315+
}
316+
for _, tt := range tt {
317+
t.Run(tt.name, func(t *testing.T) {
318+
namespaces, err := engine.Namespaces(ctx, map[string]any{"roles": tt.roles})
319+
require.NoError(t, err)
320+
require.Equal(t, tt.namespaces, namespaces)
321+
})
322+
}
323+
}

‎internal/server/authz/engine/rego/engine.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -167,15 +167,15 @@ func (e *Engine) Namespaces(ctx context.Context, input map[string]any) ([]string
167167
return nil, err
168168
}
169169
if len(results) == 0 {
170-
return []string{}, nil
170+
return nil, errors.New("no results found")
171171
}
172172
values, ok := results[0].Expressions[0].Value.([]any)
173173
if !ok {
174174
return nil, fmt.Errorf("unexpected result type: %T", results[0].Expressions[0].Value)
175175
}
176-
var namespaces []string
177-
for _, ns := range values {
178-
namespaces = append(namespaces, fmt.Sprintf("%s", ns))
176+
namespaces := make([]string, len(values))
177+
for i, ns := range values {
178+
namespaces[i] = fmt.Sprintf("%s", ns)
179179
}
180180
return namespaces, nil
181181
}

‎internal/server/authz/engine/rego/engine_test.go

Lines changed: 56 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"testing"
99
"time"
1010

11+
"github.com/stretchr/testify/assert"
1112
"github.com/stretchr/testify/require"
1213
"go.flipt.io/flipt/internal/server/authz/engine/rego/source"
1314
authrpc "go.flipt.io/flipt/rpc/flipt/auth"
@@ -29,6 +30,17 @@ func TestEngine_NewEngine(t *testing.T) {
2930
}
3031

3132
func TestEngine_IsAllowed(t *testing.T) {
33+
policy, err := os.ReadFile("../testdata/rbac.rego")
34+
require.NoError(t, err)
35+
36+
data, err := os.ReadFile("../testdata/rbac.json")
37+
require.NoError(t, err)
38+
39+
ctx, cancel := context.WithCancel(context.Background())
40+
t.Cleanup(cancel)
41+
engine, err := newEngine(ctx, zaptest.NewLogger(t), withPolicySource(policySource(string(policy))), withDataSource(dataSource(string(data)), 5*time.Second))
42+
require.NoError(t, err)
43+
3244
tests := []struct {
3345
name string
3446
input string
@@ -200,17 +212,6 @@ func TestEngine_IsAllowed(t *testing.T) {
200212

201213
for _, tt := range tests {
202214
t.Run(tt.name, func(t *testing.T) {
203-
policy, err := os.ReadFile("../testdata/rbac.rego")
204-
require.NoError(t, err)
205-
206-
data, err := os.ReadFile("../testdata/rbac.json")
207-
require.NoError(t, err)
208-
209-
ctx, cancel := context.WithCancel(context.Background())
210-
t.Cleanup(cancel)
211-
engine, err := newEngine(ctx, zaptest.NewLogger(t), withPolicySource(policySource(string(policy))), withDataSource(dataSource(string(data)), 5*time.Second))
212-
require.NoError(t, err)
213-
214215
var input map[string]interface{}
215216

216217
err = json.Unmarshal([]byte(tt.input), &input)
@@ -221,6 +222,14 @@ func TestEngine_IsAllowed(t *testing.T) {
221222
require.Equal(t, tt.expected, allowed)
222223
})
223224
}
225+
226+
t.Run("viewable namespaces without definition", func(t *testing.T) {
227+
ctx, cancel := context.WithCancel(context.Background())
228+
t.Cleanup(cancel)
229+
namespaces, err := engine.Namespaces(ctx, map[string]any{})
230+
require.Error(t, err)
231+
require.Nil(t, namespaces)
232+
})
224233
}
225234

226235
func TestEngine_IsAuthMethod(t *testing.T) {
@@ -269,6 +278,42 @@ func TestEngine_IsAuthMethod(t *testing.T) {
269278
}
270279
}
271280

281+
func TestViewableNamespaces(t *testing.T) {
282+
ctx := context.Background()
283+
284+
policy, err := os.ReadFile("../testdata/viewable_namespaces.rego")
285+
require.NoError(t, err)
286+
287+
data, err := os.ReadFile("../testdata/viewable_namespaces.json")
288+
require.NoError(t, err)
289+
290+
ctx, cancel := context.WithCancel(context.Background())
291+
t.Cleanup(cancel)
292+
engine, err := newEngine(ctx, zaptest.NewLogger(t), withPolicySource(policySource(string(policy))), withDataSource(dataSource(string(data)), 5*time.Second))
293+
require.NoError(t, err)
294+
295+
t.Cleanup(func() {
296+
assert.NoError(t, engine.Shutdown(ctx))
297+
})
298+
299+
tt := []struct {
300+
name string
301+
roles []string
302+
namespaces []string
303+
}{
304+
{"empty", []string{}, []string{}},
305+
{"devs", []string{"devs"}, []string{"local", "staging"}},
306+
{"devsops", []string{"devs", "ops"}, []string{"local", "production", "staging"}},
307+
}
308+
for _, tt := range tt {
309+
t.Run(tt.name, func(t *testing.T) {
310+
namespaces, err := engine.Namespaces(ctx, map[string]any{"roles": tt.roles})
311+
require.NoError(t, err)
312+
require.Equal(t, tt.namespaces, namespaces)
313+
})
314+
}
315+
}
316+
272317
type policySource string
273318

274319
func (p policySource) Get(context.Context, source.Hash) ([]byte, source.Hash, error) {

‎internal/server/authz/engine/testdata/viewable_namespaces.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"roles_to_namespaces": {
3+
"devs": ["local", "staging"],
4+
"ops": ["staging", "production"]
5+
}
6+
}

‎internal/server/authz/engine/testdata/viewable_namespaces.rego

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
2+
package flipt.authz.v1
3+
4+
import rego.v1
5+
import data
6+
7+
viewable_namespaces contains namespace if {
8+
some role in input.roles
9+
some namespace in data.roles_to_namespaces[role]
10+
}
11+
12+
default allow := false
13+
14+
allow if {
15+
input.request.namespace in viewable_namespaces
16+
}

‎internal/server/authz/middleware/grpc/middleware.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,12 @@ func AuthorizationRequiredInterceptor(logger *zap.Logger, policyVerifier authz.V
9898
return ctx, errUnauthorized
9999
}
100100

101-
// FIXME: This is proof of concept
102101
if info.FullMethod == flipt.Flipt_ListNamespaces_FullMethodName {
103102
namespaces, err := policyVerifier.Namespaces(ctx, map[string]any{
104103
"request": request,
105104
"authentication": auth,
106105
})
106+
107107
logger.Debug("policy namespaces evaluation", zap.Any("namespaces", namespaces), zap.Error(err))
108108
if err == nil && len(namespaces) > 0 {
109109
// if user has no access to `default` namespace the api call to list namespaces

‎internal/server/authz/middleware/grpc/middleware_test.go

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,10 @@ import (
1515
)
1616

1717
type mockPolicyVerifier struct {
18-
isAllowed bool
19-
wantErr error
20-
input map[string]any
18+
isAllowed bool
19+
wantErr error
20+
input map[string]any
21+
viewableNamespaces []string
2122
}
2223

2324
func (v *mockPolicyVerifier) IsAllowed(ctx context.Context, input map[string]any) (bool, error) {
@@ -26,6 +27,9 @@ func (v *mockPolicyVerifier) IsAllowed(ctx context.Context, input map[string]any
2627
}
2728

2829
func (v *mockPolicyVerifier) Namespaces(_ context.Context, _ map[string]any) ([]string, error) {
30+
if v.viewableNamespaces != nil {
31+
return v.viewableNamespaces, nil
32+
}
2933
return nil, errors.ErrUnsupported
3034
}
3135

@@ -50,14 +54,16 @@ var adminAuth = &authrpc.Authentication{
5054

5155
func TestAuthorizationRequiredInterceptor(t *testing.T) {
5256
tests := []struct {
53-
name string
54-
server any
55-
req any
56-
authn *authrpc.Authentication
57-
validatorAllowed bool
58-
validatorErr error
59-
wantAllowed bool
60-
authzInput map[string]any
57+
name string
58+
server any
59+
req any
60+
authn *authrpc.Authentication
61+
validatorAllowed bool
62+
validatorErr error
63+
wantAllowed bool
64+
authzInput map[string]any
65+
viewableNamespaces []string
66+
serverFullMethod string
6167
}{
6268
{
6369
name: "allowed",
@@ -124,6 +130,22 @@ func TestAuthorizationRequiredInterceptor(t *testing.T) {
124130
validatorErr: errors.New("error"),
125131
wantAllowed: false,
126132
},
133+
{
134+
name: "list namespaces",
135+
authn: adminAuth,
136+
req: &flipt.ListNamespaceRequest{},
137+
wantAllowed: true,
138+
viewableNamespaces: []string{"special"},
139+
serverFullMethod: "/flipt.Flipt/ListNamespaces",
140+
authzInput: map[string]any{
141+
"request": flipt.Request{
142+
Resource: flipt.ResourceNamespace,
143+
Action: flipt.ActionRead,
144+
Status: flipt.StatusSuccess,
145+
},
146+
"authentication": adminAuth,
147+
},
148+
},
127149
}
128150

129151
for _, tt := range tests {
@@ -140,14 +162,16 @@ func TestAuthorizationRequiredInterceptor(t *testing.T) {
140162

141163
srv = &grpc.UnaryServerInfo{Server: &mockServer{}}
142164
policyVerfier = &mockPolicyVerifier{
143-
isAllowed: tt.validatorAllowed,
144-
wantErr: tt.validatorErr,
165+
isAllowed: tt.validatorAllowed,
166+
wantErr: tt.validatorErr,
167+
viewableNamespaces: tt.viewableNamespaces,
145168
}
146169
)
147170

148171
if tt.server != nil {
149172
srv.Server = tt.server
150173
}
174+
srv.FullMethod = tt.serverFullMethod
151175

152176
_, err := AuthorizationRequiredInterceptor(logger, policyVerfier)(ctx, tt.req, srv, handler)
153177

0 commit comments

Comments
 (0)