Skip to content

Commit

Permalink
Merge pull request #1116 from alesstimec/can-add-model-check-when-add…
Browse files Browse the repository at this point in the history
…ing-model-01

CSS-6081 Add-model check when creating a model
alesstimec authored Jan 5, 2024
2 parents 31f3423 + 9142662 commit a9dfb57
Showing 4 changed files with 108 additions and 9 deletions.
4 changes: 4 additions & 0 deletions internal/jimm/cloudcredential_test.go
Original file line number Diff line number Diff line change
@@ -361,6 +361,10 @@ func TestUpdateCloudCredential(t *testing.T) {
err = alice.SetCloudAccess(context.Background(), cloud.ResourceTag(), ofganames.AdministratorRelation)
c.Assert(err, qt.IsNil)

e := openfga.NewUser(&eve, client)
err = e.SetCloudAccess(context.Background(), cloud.ResourceTag(), ofganames.CanAddModelRelation)
c.Assert(err, qt.IsNil)

controller1 := dbmodel.Controller{
Name: "test-controller-1",
UUID: "00000000-0000-0000-0000-0000-0000000000001",
21 changes: 15 additions & 6 deletions internal/jimm/model.go
Original file line number Diff line number Diff line change
@@ -256,6 +256,7 @@ func (b *modelBuilder) WithCloudCredential(credentialTag names.CloudCredentialTa
b.err = errors.E(err, fmt.Sprintf("failed to fetch cloud credentials %s", credential.Path()))
}
b.credential = &credential

return b
}

@@ -518,10 +519,10 @@ func (b *modelBuilder) JujuModelInfo() *jujuparams.ModelInfo {
func (j *JIMM) AddModel(ctx context.Context, user *openfga.User, args *ModelCreateArgs) (_ *jujuparams.ModelInfo, err error) {
const op = errors.Op("jimm.AddModel")

owner := dbmodel.User{
owner := &dbmodel.User{
Username: args.Owner.Id(),
}
err = j.Database.GetUser(ctx, &owner)
err = j.Database.GetUser(ctx, owner)
if err != nil {
return nil, errors.E(op, err)
}
@@ -531,10 +532,8 @@ func (j *JIMM) AddModel(ctx context.Context, user *openfga.User, args *ModelCrea
return nil, errors.E(op, errors.CodeUnauthorized, "unauthorized")
}

// TODO(Kian) CSS-6081 Missing check for add-model access on the desired cloud.

builder := newModelBuilder(ctx, j)
builder = builder.WithOwner(&owner)
builder = builder.WithOwner(owner)
builder = builder.WithName(args.Name)
if err := builder.Error(); err != nil {
return nil, errors.E(op, err)
@@ -573,6 +572,16 @@ func (j *JIMM) AddModel(ctx context.Context, user *openfga.User, args *ModelCrea
return nil, errors.E(op, err)
}

// at this point we know which cloud will host the model and
// we must check the user has add-model permission on the cloud
canAddModel, err := openfga.NewUser(owner, j.OpenFGAClient).IsAllowedAddModel(ctx, builder.cloud.ResourceTag())
if err != nil {
return nil, errors.E(op, "permission check failed")
}
if !canAddModel {
return nil, errors.E(op, errors.CodeUnauthorized, "unauthorized")
}

// fetch cloud region defaults
if args.Cloud != (names.CloudTag{}) && builder.cloudRegion != "" {
cloudRegionDefaults := dbmodel.CloudDefaults{
@@ -629,7 +638,7 @@ func (j *JIMM) AddModel(ctx context.Context, user *openfga.User, args *ModelCrea
zap.String("model", builder.model.UUID.String),
)
}
err = openfga.NewUser(&owner, j.OpenFGAClient).SetModelAccess(ctx, names.NewModelTag(mi.UUID), ofganames.AdministratorRelation)
err = openfga.NewUser(owner, j.OpenFGAClient).SetModelAccess(ctx, names.NewModelTag(mi.UUID), ofganames.AdministratorRelation)
if err != nil {
zapctx.Error(
ctx,
86 changes: 86 additions & 0 deletions internal/jimm/model_test.go
Original file line number Diff line number Diff line change
@@ -158,6 +158,9 @@ clouds:
type: test-provider
regions:
- name: test-region-1
users:
- user: alice@external
access: add-model
user-defaults:
- user: alice@external
defaults:
@@ -268,6 +271,9 @@ clouds:
type: test-provider
regions:
- name: test-region-1
users:
- user: alice@external
access: add-model
user-defaults:
- user: alice@external
defaults:
@@ -379,6 +385,9 @@ clouds:
type: test-provider
regions:
- name: test-region-1
users:
- user: alice@external
access: add-model
cloud-credentials:
- name: test-credential-1
owner: alice@external
@@ -469,6 +478,11 @@ clouds:
type: test-provider
regions:
- name: test-region-1
users:
- user: alice@external
access: admin
- user: bob@external
access: add-model
cloud-credentials:
- name: test-credential-1
owner: alice@external
@@ -564,6 +578,9 @@ clouds:
type: test-provider
regions:
- name: test-region-1
users:
- user: alice@external
access: add-model
cloud-credentials:
- name: test-credential-1
owner: alice@external
@@ -627,6 +644,9 @@ clouds:
type: test-provider
regions:
- name: test-region-1
users:
- user: alice@external
access: add-model
cloud-credentials:
- name: test-credential-1
owner: alice@external
@@ -677,6 +697,9 @@ clouds:
type: test-provider
regions:
- name: test-region-1
users:
- user: alice@external
access: add-model
- name: test-cloud-2
type: test-provider
regions:
@@ -752,6 +775,9 @@ clouds:
type: test-provider
regions:
- name: test-region-1
users:
- user: alice@external
access: add-model
cloud-credentials:
- name: test-credential-1
owner: alice@external
@@ -803,6 +829,63 @@ users:
CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@external/test-credential-1").String(),
},
expectError: "failed to update cloud credential: a silly error",
}, {
name: "UserWithoutAddModelPermission",
env: `
clouds:
- name: test-cloud
type: test-provider
regions:
- name: test-region-1
cloud-credentials:
- name: test-credential-1
owner: alice@external
cloud: test-cloud
auth-type: empty
controllers:
- name: controller-1
uuid: 00000000-0000-0000-0000-0000-0000000000001
cloud: test-cloud
region: test-region-1
cloud-regions:
- cloud: test-cloud
region: test-region-1
priority: 0
- name: controller-2
uuid: 00000000-0000-0000-0000-0000-0000000000002
cloud: test-cloud
region: test-region-1
cloud-regions:
- cloud: test-cloud
region: test-region-1
priority: 1
`[1:],
updateCredential: func(_ context.Context, _ jujuparams.TaggedCredential) ([]jujuparams.UpdateCredentialModelResult, error) {
return nil, nil
},
grantJIMMModelAdmin: func(_ context.Context, _ names.ModelTag) error {
return nil
},
createModel: createModel(`
uuid: 00000001-0000-0000-0000-0000-000000000001
status:
status: started
info: running a test
life: alive
users:
- user: alice@external
access: admin
`[1:]),
username: "alice@external",
jimmAdmin: true,
args: jujuparams.ModelCreateArgs{
Name: "test-model",
OwnerTag: names.NewUserTag("alice@external").String(),
CloudTag: names.NewCloudTag("test-cloud").String(),
CloudRegion: "test-region-1",
CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@external/test-credential-1").String(),
},
expectError: "unauthorized",
}}

func TestAddModel(t *testing.T) {
@@ -3482,6 +3565,9 @@ clouds:
regions:
- name: test-region-1
- name: test-region-2
users:
- user: alice@external
access: add-model
user-defaults:
- user: alice@external
controller-access: superuser
6 changes: 3 additions & 3 deletions internal/jujuclient/storage_test.go
Original file line number Diff line number Diff line change
@@ -21,9 +21,9 @@ var _ = gc.Suite(&storageSuite{})
func (s *storageSuite) TestListFilesystems(c *gc.C) {
ctx := context.Background()

cct := names.NewCloudCredentialTag(jimmtest.TestCloudName + "/bob@external/pw1").String()
cct := names.NewCloudCredentialTag(jimmtest.TestCloudName + "/bob@external/pw1")
cred := jujuparams.TaggedCredential{
Tag: cct,
Tag: cct.String(),
Credential: jujuparams.CloudCredential{
AuthType: "userpass",
Attributes: map[string]string{
@@ -51,7 +51,7 @@ func (s *storageSuite) TestListFilesystems(c *gc.C) {
err = s.API.CreateModel(ctx, &jujuparams.ModelCreateArgs{
Name: "model-1",
OwnerTag: names.NewUserTag("bob@external").String(),
CloudCredentialTag: cct,
CloudCredentialTag: cct.String(),
}, &modelInfo)
c.Assert(err, gc.Equals, nil)
uuid := modelInfo.UUID

0 comments on commit a9dfb57

Please sign in to comment.