CSS-4603 Refactor toward using `canonical/ofga` #990

babakks · 2023-07-07T12:20:41Z

Description

Refactored OpenFGA related packages to use canonical/ofga module.

Fixes CSS-4603

Engineering checklist

Check only items that apply

Documentation updated
Covered by unit tests
Covered by integration tests

Test instructions

Notes for code reviewers

Signed-off-by: Babak K. Shandiz <[email protected]>

kian99

I think this looks like a good improvement with nice shifting of some logic to the library.
I would say we need more reviews on this though.

.gitignore

cmd/jimmctl/cmd/jimmsuite_test.go

internal/openfga/names/names.go

internal/openfga/openfga.go

internal/openfga/openfga_test.go

ale8k · 2023-07-12T12:22:44Z

internal/jimm/access_test.go

@@ -27,7 +27,7 @@ import (
 // func TestJwtGenerator(t *testing.T) {
 // 	c := qt.New(t)

-// 	_, client, _, err := jimmtest.SetupTestOFGAClient(c.Name())
+// 	client, _, _, err := jimmtest.SetupTestOFGAClient(c.Name())


This is commented out, do we still need it?

Thanks to @kian99, we made a card for this TODO and removed the code part.

ale8k · 2023-07-12T12:26:31Z

internal/jimmjwx/utils_test.go

@@ -89,7 +89,7 @@ func setupService(ctx context.Context, c *qt.C) (*jimm.Service, *httptest.Server
 	err := store.CleanupJWKS(ctx)
 	c.Assert(err, qt.IsNil)

-	_, ofgaClient, cfg, err := jimmtest.SetupTestOFGAClient(c.Name())
+	_, _, cofgaParams, err := jimmtest.SetupTestOFGAClient(c.Name())


cofga is kind of ambiguous

cofga is an alias for canonical/ofga module; I couldn't find any better, yet concise, replacement for it. Can you suggest me one? @ale8k @kian99

ale8k

So it looks good to me, but won't approve until tests are all good and I'll go over it once more because it touches a lot of places. Also appears the new client is basically ours? Or at least inspired from ours?

Signed-off-by: Babak K. Shandiz <[email protected]>

kian99 · 2023-08-21T06:33:49Z

internal/jimm/access_test.go

@@ -27,7 +27,7 @@ import (
 // func TestJwtGenerator(t *testing.T) {
 // 	c := qt.New(t)

-// 	_, client, _, err := jimmtest.SetupTestOFGAClient(c.Name())
+// 	client, _, _, err := jimmtest.SetupTestOFGAClient(c.Name())


ale8k · 2023-08-21T09:51:51Z

service_test.go

@@ -539,3 +492,14 @@ func key(candid *candidtest.Server, user string) *bakery.KeyPair {
 		Private: bakery.PrivateKey{Key: bakery.Key(key.Private.Key)},
 	}
 }
+
+func cofgaParamsToJIMMOpenFGAParams(cofgaParams cofga.OpenFGAParams) jimm.OpenFGAParams {


I'd godoc comment this to explain why

ale8k

lgtm bar some comments

alesstimec · 2023-08-21T10:14:47Z

internal/jimmtest/suite.go

 }

 func (s *JIMMSuite) SetUpTest(c *gc.C) {
-	ofgaAPI, ofgaClient, ofgaConfig, err := SetupTestOFGAClient(c.TestName())
+	ofgaClient, cofgaClient, cofgaConfig, err := SetupTestOFGAClient(c.TestName())


var err error c.OFGAClient, c.OFGAClient, c.OFGAConfig, err = SetupTestOFGAClient(c.TestName())

alesstimec · 2023-08-21T10:15:57Z

internal/openfga/names/export_test.go

@@ -2,10 +2,12 @@

 package names

+import cofga "github.com/canonical/ofga"


there is no naming conflict here.. so i'd leave it as

import "github.com/canonical/ofga"

alesstimec · 2023-08-21T10:16:53Z

internal/openfga/user_test.go

@@ -6,9 +6,9 @@ import (
 	"context"
 	"sort"

+	cofga "github.com/canonical/ofga"


again, no naming conflict so just import it as ofga...

alesstimec · 2023-08-21T10:17:07Z

service.go

 	"net/http"
 	"os"
 	"strconv"
 	"strings"
 	"time"

 	"github.com/canonical/candid/candidclient"
+	cofga "github.com/canonical/ofga"


Signed-off-by: Babak K. Shandiz <[email protected]>

babakks · 2023-08-21T10:49:06Z

At the end I removed any unnecessary aliases to reference our internal openfga package (now accessed through openfga everywhere), and kept references to canonical/ofga under cofga alias.

Signed-off-by: Babak K. Shandiz <[email protected]>

babakks added 13 commits July 7, 2023 13:16

Fix exclusion of jimmctl binary

e7012d4

Signed-off-by: Babak K. Shandiz <[email protected]>

Apply initial refactoring toward using canonical/ofga

87e153c

Signed-off-by: Babak K. Shandiz <[email protected]>

Add go-env target to setup Go env vars

bc1876c

Signed-off-by: Babak K. Shandiz <[email protected]>

Update docs to mention running make go-env

7f29b1e

Signed-off-by: Babak K. Shandiz <[email protected]>

Run make go-env

fe4563e

Signed-off-by: Babak K. Shandiz <[email protected]>

Fix some errors

6b8dfa5

Signed-off-by: Babak K. Shandiz <[email protected]>

Remove commented/unused code

69ad7e3

Signed-off-by: Babak K. Shandiz <[email protected]>

Organize OpenFGA-related exported types

f9655dc

Signed-off-by: Babak K. Shandiz <[email protected]>

Rename AddRelations to AddRelation to match ofga symbol names

26f0680

Signed-off-by: Babak K. Shandiz <[email protected]>

Replace references to key with tuple

9f6b78d

Signed-off-by: Babak K. Shandiz <[email protected]>

Fix some failing tests

b525753

Signed-off-by: Babak K. Shandiz <[email protected]>

Merge branch 'feature-rebac' into css-4603/use-ofga

ccbb88b

Signed-off-by: Babak K. Shandiz <[email protected]>

Remove commented code

e5094a0

Signed-off-by: Babak K. Shandiz <[email protected]>

babakks marked this pull request as ready for review July 12, 2023 08:44

kian99 approved these changes Jul 12, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

cmd/jimmctl/cmd/jimmsuite_test.go Show resolved Hide resolved

internal/openfga/names/names.go Show resolved Hide resolved

internal/openfga/openfga.go Show resolved Hide resolved

internal/openfga/openfga_test.go Outdated Show resolved Hide resolved

ale8k reviewed Jul 12, 2023

View reviewed changes

babakks added 12 commits August 15, 2023 13:30

Merge branch 'feature-rebac' into css-4603/use-ofga

c70bdca

Signed-off-by: Babak K. Shandiz <[email protected]>

Merge branch 'feature-rebac' into css-4603/use-ofga

afb463b

Signed-off-by: Babak K. Shandiz <[email protected]>

Fix some references

c8ed897

Signed-off-by: Babak K. Shandiz <[email protected]>

Fix some tests

0d2aeba

Signed-off-by: Babak K. Shandiz <[email protected]>

Delete associated authorization model

40f092b

Signed-off-by: Babak K. Shandiz <[email protected]>

Fix setup

1a99aa9

Signed-off-by: Babak K. Shandiz <[email protected]>

Fix authTypeDefinition empty check

9fac17b

Signed-off-by: Babak K. Shandiz <[email protected]>

Update go.sum

dd97072

Signed-off-by: Babak K. Shandiz <[email protected]>

Fix dependency issues

244508d

Signed-off-by: Babak K. Shandiz <[email protected]>

Replace go.sum/go.mod from feature-rebac

51df1df

Signed-off-by: Babak K. Shandiz <[email protected]>

Re-add canonical/ofga module as dependency

43af474

Signed-off-by: Babak K. Shandiz <[email protected]>

Upgrade frankban/quicktest

d74ed6e

Signed-off-by: Babak K. Shandiz <[email protected]>

babakks added 2 commits August 18, 2023 14:08

Update to latest canonical/ofga changes

6a7ef44

Signed-off-by: Babak K. Shandiz <[email protected]>

Tidy dependencies

c86391c

Signed-off-by: Babak K. Shandiz <[email protected]>

kian99 approved these changes Aug 21, 2023

View reviewed changes

ale8k reviewed Aug 21, 2023

View reviewed changes

ale8k approved these changes Aug 21, 2023

View reviewed changes

alesstimec approved these changes Aug 21, 2023

View reviewed changes

babakks added 3 commits August 21, 2023 11:28

Update TODO with Jira card reference

580b779

Signed-off-by: Babak K. Shandiz <[email protected]>

Simplify struct field assignment

89979d9

Signed-off-by: Babak K. Shandiz <[email protected]>

Make internal openfga package imports consistent

dc3df67

Signed-off-by: Babak K. Shandiz <[email protected]>

Add godoc for config type adapter function

a48c9d3

Signed-off-by: Babak K. Shandiz <[email protected]>

babakks merged commit dc7f417 into canonical:feature-rebac Aug 21, 2023
1 check passed

babakks deleted the css-4603/use-ofga branch August 21, 2023 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSS-4603 Refactor toward using `canonical/ofga` #990

CSS-4603 Refactor toward using `canonical/ofga` #990

babakks commented Jul 7, 2023

kian99 left a comment

ale8k Jul 12, 2023

kian99 Aug 21, 2023

babakks Aug 21, 2023

ale8k Jul 12, 2023

babakks Aug 21, 2023 •

edited

Loading

ale8k left a comment

kian99 Aug 21, 2023

ale8k Aug 21, 2023

ale8k left a comment

alesstimec Aug 21, 2023

alesstimec Aug 21, 2023

alesstimec Aug 21, 2023

alesstimec Aug 21, 2023

babakks commented Aug 21, 2023

		@@ -2,10 +2,12 @@

		package names

		import cofga "github.com/canonical/ofga"

CSS-4603 Refactor toward using canonical/ofga #990

CSS-4603 Refactor toward using canonical/ofga #990

Conversation

babakks commented Jul 7, 2023

Description

Engineering checklist

Test instructions

Notes for code reviewers

kian99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babakks Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

ale8k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ale8k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babakks commented Aug 21, 2023

CSS-4603 Refactor toward using `canonical/ofga` #990

CSS-4603 Refactor toward using `canonical/ofga` #990

babakks Aug 21, 2023 •

edited

Loading