Skip to content

Commit 7bb7e0f

Browse files
heiytorheiytor
committed
refactor(api): change PublicKey update/delete to use full entity
- Refactor UpdatePublicKey service to first fetch existing key, mutate its fields, and persist using PublicKeyUpdate(*models.PublicKey) - Refactor DeletePublicKey service to first fetch existing key and delete using PublicKeyDelete(*models.PublicKey) - Update store interface to change PublicKeyUpdate and PublicKeyDelete signatures to accept *models.PublicKey instead of (fingerprint, tenantID, ...) - Update Mongo implementation to handle PublicKeyUpdate and PublicKeyDelete using full entity, removing unused aggregation logic - Update mocks to reflect new method signatures and return types - Refactor and extend service tests to cover new behavior and error scenarios - Simplify mongo store tests to validate only the returned error, not entity payloads BREAKING CHANGE: PublicKeyUpdate and PublicKeyDelete now accept *models.PublicKey and return only error
1 parent cf343f1 commit 7bb7e0f

File tree

6 files changed

+179
-238
lines changed

6 files changed

+179
-238
lines changed

‎api/services/sshkeys.go‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,11 @@ func (s *service) ListPublicKeys(ctx context.Context, req *requests.ListPublicKe
165165
}
166166

167167
func (s *service) UpdatePublicKey(ctx context.Context, fingerprint, tenant string, key requests.PublicKeyUpdate) (*models.PublicKey, error) {
168+
publicKey, err := s.store.PublicKeyGet(ctx, fingerprint, tenant)
169+
if err != nil {
170+
return nil, NewErrPublicKeyNotFound(fingerprint, err)
171+
}
172+
168173
// Checks if public key filter type is Tags. If it is, checks if there are, at least, one tag on the public key
169174
// filter and if the all tags exist on database.
170175
tagIDs := []string{}
@@ -191,30 +196,31 @@ func (s *service) UpdatePublicKey(ctx context.Context, fingerprint, tenant strin
191196
}
192197
}
193198

194-
model := models.PublicKeyUpdate{
195-
PublicKeyFields: models.PublicKeyFields{
196-
Name: key.Name,
197-
Username: key.Username,
198-
Filter: models.PublicKeyFilter{
199-
Hostname: key.Filter.Hostname,
200-
Taggable: models.Taggable{TagIDs: tagIDs, Tags: nil},
201-
},
202-
},
199+
// Update the public key fields
200+
publicKey.Name = key.Name
201+
publicKey.Username = key.Username
202+
publicKey.Filter.Hostname = key.Filter.Hostname
203+
publicKey.Filter.TagIDs = tagIDs
204+
publicKey.Filter.Tags = nil
205+
206+
if err := s.store.PublicKeyUpdate(ctx, publicKey); err != nil {
207+
return nil, err
203208
}
204209

205-
return s.store.PublicKeyUpdate(ctx, fingerprint, tenant, &model)
210+
return s.store.PublicKeyGet(ctx, fingerprint, tenant)
206211
}
207212

208213
func (s *service) DeletePublicKey(ctx context.Context, fingerprint, tenant string) error {
209214
if _, err := s.store.NamespaceResolve(ctx, store.NamespaceTenantIDResolver, tenant); err != nil {
210215
return NewErrNamespaceNotFound(tenant, err)
211216
}
212217

213-
if _, err := s.store.PublicKeyGet(ctx, fingerprint, tenant); err != nil {
218+
publicKey, err := s.store.PublicKeyGet(ctx, fingerprint, tenant)
219+
if err != nil {
214220
return NewErrPublicKeyNotFound(fingerprint, err)
215221
}
216222

217-
return s.store.PublicKeyDelete(ctx, fingerprint, tenant)
223+
return s.store.PublicKeyDelete(ctx, publicKey)
218224
}
219225

220226
func (s *service) CreatePrivateKey(ctx context.Context) (*models.PrivateKey, error) {

‎api/services/sshkeys_test.go‎

Lines changed: 95 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,20 @@ func TestUpdatePublicKeys(t *testing.T) {
340340
requiredMocks func()
341341
expected Expected
342342
}{
343+
{
344+
description: "fail when public key not found",
345+
fingerprint: "fingerprint",
346+
tenantID: "tenant",
347+
keyUpdate: requests.PublicKeyUpdate{
348+
Filter: requests.PublicKeyFilter{
349+
Tags: []string{"tag1", "tag2"},
350+
},
351+
},
352+
requiredMocks: func() {
353+
storeMock.On("PublicKeyGet", ctx, "fingerprint", "tenant").Return(nil, store.ErrNoDocuments).Once()
354+
},
355+
expected: Expected{nil, NewErrPublicKeyNotFound("fingerprint", store.ErrNoDocuments)},
356+
},
343357
{
344358
description: "fail update the key when tag list retrieval fails",
345359
fingerprint: "fingerprint",
@@ -350,6 +364,11 @@ func TestUpdatePublicKeys(t *testing.T) {
350364
},
351365
},
352366
requiredMocks: func() {
367+
existingKey := &models.PublicKey{
368+
Fingerprint: "fingerprint",
369+
TenantID: "tenant",
370+
}
371+
storeMock.On("PublicKeyGet", ctx, "fingerprint", "tenant").Return(existingKey, nil).Once()
353372
queryOptionsMock.
354373
On("InNamespace", "tenant").
355374
Return(nil).
@@ -368,10 +387,15 @@ func TestUpdatePublicKeys(t *testing.T) {
368387
},
369388
},
370389
requiredMocks: func() {
390+
existingKey := &models.PublicKey{
391+
Fingerprint: "fingerprint",
392+
TenantID: "tenant",
393+
}
371394
tags := []models.Tag{
372395
{ID: "tag1_id", Name: "tag1", TenantID: "tenant"},
373396
{ID: "tag4_id", Name: "tag4", TenantID: "tenant"},
374397
}
398+
storeMock.On("PublicKeyGet", ctx, "fingerprint", "tenant").Return(existingKey, nil).Once()
375399
queryOptionsMock.
376400
On("InNamespace", "tenant").
377401
Return(nil).
@@ -381,7 +405,7 @@ func TestUpdatePublicKeys(t *testing.T) {
381405
expected: Expected{nil, NewErrTagNotFound("tag2", nil)},
382406
},
383407
{
384-
description: "Fail update the key when filter is tags",
408+
description: "fail update the key when filter is tags",
385409
fingerprint: "fingerprint",
386410
tenantID: "tenant",
387411
keyUpdate: requests.PublicKeyUpdate{
@@ -390,29 +414,34 @@ func TestUpdatePublicKeys(t *testing.T) {
390414
},
391415
},
392416
requiredMocks: func() {
417+
existingKey := &models.PublicKey{
418+
Fingerprint: "fingerprint",
419+
TenantID: "tenant",
420+
PublicKeyFields: models.PublicKeyFields{
421+
Filter: models.PublicKeyFilter{},
422+
},
423+
}
393424
tags := []models.Tag{
394425
{ID: "tag1_id", Name: "tag1", TenantID: "tenant"},
395426
{ID: "tag2_id", Name: "tag2", TenantID: "tenant"},
396427
}
397-
model := models.PublicKeyUpdate{
398-
PublicKeyFields: models.PublicKeyFields{
399-
Filter: models.PublicKeyFilter{
400-
Taggable: models.Taggable{TagIDs: []string{"tag1_id", "tag2_id"}, Tags: nil},
401-
},
402-
},
403-
}
404428

429+
expectedKey := *existingKey
430+
expectedKey.Filter.TagIDs = []string{"tag1_id", "tag2_id"}
431+
expectedKey.Filter.Tags = nil
432+
433+
storeMock.On("PublicKeyGet", ctx, "fingerprint", "tenant").Return(existingKey, nil).Once()
405434
queryOptionsMock.
406435
On("InNamespace", "tenant").
407436
Return(nil).
408437
Once()
409438
storeMock.On("TagList", ctx, mock.AnythingOfType("store.QueryOption")).Return(tags, len(tags), nil).Once()
410-
storeMock.On("PublicKeyUpdate", ctx, "fingerprint", "tenant", &model).Return(nil, errors.New("error", "", 0)).Once()
439+
storeMock.On("PublicKeyUpdate", ctx, &expectedKey).Return(errors.New("error", "", 0)).Once()
411440
},
412441
expected: Expected{nil, errors.New("error", "", 0)},
413442
},
414443
{
415-
description: "Successful update the key when filter is tags",
444+
description: "successful update the key when filter is tags",
416445
fingerprint: "fingerprint",
417446
tenantID: "tenant",
418447
keyUpdate: requests.PublicKeyUpdate{
@@ -421,34 +450,44 @@ func TestUpdatePublicKeys(t *testing.T) {
421450
},
422451
},
423452
requiredMocks: func() {
453+
existingKey := &models.PublicKey{
454+
Fingerprint: "fingerprint",
455+
TenantID: "tenant",
456+
PublicKeyFields: models.PublicKeyFields{
457+
Filter: models.PublicKeyFilter{},
458+
},
459+
}
424460
tags := []models.Tag{
425461
{ID: "tag1_id", Name: "tag1", TenantID: "tenant"},
426462
{ID: "tag2_id", Name: "tag2", TenantID: "tenant"},
427463
}
428-
model := models.PublicKeyUpdate{
429-
PublicKeyFields: models.PublicKeyFields{
430-
Filter: models.PublicKeyFilter{
431-
Taggable: models.Taggable{TagIDs: []string{"tag1_id", "tag2_id"}, Tags: nil},
432-
},
433-
},
434-
}
435464

436-
keyUpdateWithTagsModel := &models.PublicKey{
465+
expectedKey := *existingKey
466+
expectedKey.Filter.TagIDs = []string{"tag1_id", "tag2_id"}
467+
expectedKey.Filter.Tags = nil
468+
469+
updatedKey := &models.PublicKey{
470+
Fingerprint: "fingerprint",
471+
TenantID: "tenant",
437472
PublicKeyFields: models.PublicKeyFields{
438473
Filter: models.PublicKeyFilter{
439474
Taggable: models.Taggable{TagIDs: []string{"tag1_id", "tag2_id"}},
440475
},
441476
},
442477
}
443478

479+
storeMock.On("PublicKeyGet", ctx, "fingerprint", "tenant").Return(existingKey, nil).Once()
444480
queryOptionsMock.
445481
On("InNamespace", "tenant").
446482
Return(nil).
447483
Once()
448484
storeMock.On("TagList", ctx, mock.AnythingOfType("store.QueryOption")).Return(tags, len(tags), nil).Once()
449-
storeMock.On("PublicKeyUpdate", ctx, "fingerprint", "tenant", &model).Return(keyUpdateWithTagsModel, nil).Once()
485+
storeMock.On("PublicKeyUpdate", ctx, &expectedKey).Return(nil).Once()
486+
storeMock.On("PublicKeyGet", ctx, "fingerprint", "tenant").Return(updatedKey, nil).Once()
450487
},
451488
expected: Expected{&models.PublicKey{
489+
Fingerprint: "fingerprint",
490+
TenantID: "tenant",
452491
PublicKeyFields: models.PublicKeyFields{
453492
Filter: models.PublicKeyFilter{
454493
Taggable: models.Taggable{TagIDs: []string{"tag1_id", "tag2_id"}},
@@ -457,7 +496,7 @@ func TestUpdatePublicKeys(t *testing.T) {
457496
}, nil},
458497
},
459498
{
460-
description: "Fail update the key when filter is hostname",
499+
description: "successful update the key when filter is hostname",
461500
fingerprint: "fingerprint",
462501
tenantID: "tenant",
463502
keyUpdate: requests.PublicKeyUpdate{
@@ -466,48 +505,36 @@ func TestUpdatePublicKeys(t *testing.T) {
466505
},
467506
},
468507
requiredMocks: func() {
469-
model := models.PublicKeyUpdate{
508+
existingKey := &models.PublicKey{
509+
Fingerprint: "fingerprint",
510+
TenantID: "tenant",
470511
PublicKeyFields: models.PublicKeyFields{
471-
Filter: models.PublicKeyFilter{
472-
Hostname: ".*",
473-
Taggable: models.Taggable{TagIDs: []string{}, Tags: nil},
474-
},
512+
Filter: models.PublicKeyFilter{},
475513
},
476514
}
477515

478-
storeMock.On("PublicKeyUpdate", ctx, "fingerprint", "tenant", &model).Return(nil, errors.New("error", "", 0)).Once()
479-
},
480-
expected: Expected{nil, errors.New("error", "", 0)},
481-
},
482-
{
483-
description: "Successful update the key when filter is hostname",
484-
fingerprint: "fingerprint",
485-
tenantID: "tenant",
486-
keyUpdate: requests.PublicKeyUpdate{
487-
Filter: requests.PublicKeyFilter{
488-
Hostname: ".*",
489-
},
490-
},
491-
requiredMocks: func() {
492-
model := models.PublicKeyUpdate{
493-
PublicKeyFields: models.PublicKeyFields{
494-
Filter: models.PublicKeyFilter{
495-
Hostname: ".*",
496-
Taggable: models.Taggable{TagIDs: []string{}, Tags: nil},
497-
},
498-
},
499-
}
516+
expectedKey := *existingKey
517+
expectedKey.Filter.Hostname = ".*"
518+
expectedKey.Filter.TagIDs = []string{}
519+
expectedKey.Filter.Tags = nil
500520

501-
keyUpdateWithHostnameModel := &models.PublicKey{
521+
updatedKey := &models.PublicKey{
522+
Fingerprint: "fingerprint",
523+
TenantID: "tenant",
502524
PublicKeyFields: models.PublicKeyFields{
503525
Filter: models.PublicKeyFilter{
504526
Hostname: ".*",
505527
},
506528
},
507529
}
508-
storeMock.On("PublicKeyUpdate", ctx, "fingerprint", "tenant", &model).Return(keyUpdateWithHostnameModel, nil).Once()
530+
531+
storeMock.On("PublicKeyGet", ctx, "fingerprint", "tenant").Return(existingKey, nil).Once()
532+
storeMock.On("PublicKeyUpdate", ctx, &expectedKey).Return(nil).Once()
533+
storeMock.On("PublicKeyGet", ctx, "fingerprint", "tenant").Return(updatedKey, nil).Once()
509534
},
510535
expected: Expected{&models.PublicKey{
536+
Fingerprint: "fingerprint",
537+
TenantID: "tenant",
511538
PublicKeyFields: models.PublicKeyFields{
512539
Filter: models.PublicKeyFilter{
513540
Hostname: ".*",
@@ -581,39 +608,41 @@ func TestDeletePublicKeys(t *testing.T) {
581608
tenantID: "tenant1",
582609
requiredMocks: func() {
583610
namespace := &models.Namespace{TenantID: "tenant1"}
611+
publicKey := &models.PublicKey{
612+
Data: []byte("teste"),
613+
Fingerprint: "fingerprint",
614+
CreatedAt: clock.Now(),
615+
TenantID: "tenant1",
616+
PublicKeyFields: models.PublicKeyFields{Name: "teste"},
617+
}
584618

585619
storeMock.On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, namespace.TenantID).Return(namespace, nil).Once()
586620
storeMock.On("PublicKeyGet", ctx, "fingerprint", namespace.TenantID).
587-
Return(&models.PublicKey{
588-
Data: []byte("teste"),
589-
Fingerprint: "fingerprint",
590-
CreatedAt: clock.Now(),
591-
TenantID: "tenant1",
592-
PublicKeyFields: models.PublicKeyFields{Name: "teste"},
593-
}, nil).Once()
594-
storeMock.On("PublicKeyDelete", ctx, "fingerprint", "tenant1").
621+
Return(publicKey, nil).Once()
622+
storeMock.On("PublicKeyDelete", ctx, publicKey).
595623
Return(errors.New("error", "", 0)).Once()
596624
},
597625
expected: Expected{errors.New("error", "", 0)},
598626
},
599627
{
600-
description: "Successful to delete the key",
628+
description: "successful to delete the key",
601629
ctx: ctx,
602630
fingerprint: "fingerprint",
603631
tenantID: "tenant1",
604632
requiredMocks: func() {
605633
namespace := &models.Namespace{TenantID: "tenant1"}
634+
publicKey := &models.PublicKey{
635+
Data: []byte("teste"),
636+
Fingerprint: "fingerprint",
637+
CreatedAt: clock.Now(),
638+
TenantID: "tenant1",
639+
PublicKeyFields: models.PublicKeyFields{Name: "teste"},
640+
}
606641

607642
storeMock.On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, namespace.TenantID).Return(namespace, nil).Once()
608643
storeMock.On("PublicKeyGet", ctx, "fingerprint", namespace.TenantID).
609-
Return(&models.PublicKey{
610-
Data: []byte("teste"),
611-
Fingerprint: "fingerprint",
612-
CreatedAt: clock.Now(),
613-
TenantID: "tenant1",
614-
PublicKeyFields: models.PublicKeyFields{Name: "teste"},
615-
}, nil).Once()
616-
storeMock.On("PublicKeyDelete", ctx, "fingerprint", "tenant1").Return(nil).Once()
644+
Return(publicKey, nil).Once()
645+
storeMock.On("PublicKeyDelete", ctx, publicKey).Return(nil).Once()
617646
},
618647
expected: Expected{nil},
619648
},

0 commit comments

Comments
 (0)