Skip to content

Commit

Permalink
Add admin/superuser error handling and cleanup (#8559)
Browse files Browse the repository at this point in the history
* Add admin/superuser error handling and cleanup

- Added a defer function to clean up the admin user if an error occurs during creation.
- Added tests for creating an admin user, verifying successful creation, attempting to create another admin user, deleting the admin user, creating an admin user without a secret access key, and re-adding the admin user.
- Included a new test `TestAddAdminUser` to verify the behavior of the `AddAdminUser` function in various scenarios.

* Added a logger to log the error message and warn about deleting the user
  • Loading branch information
nopcoder authored Jan 29, 2025
1 parent f2b9264 commit 340dc93
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 0 deletions.
10 changes: 10 additions & 0 deletions pkg/auth/setup/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,16 @@ func AddAdminUser(ctx context.Context, authService auth.Service, user *model.Sup
if err != nil {
return nil, fmt.Errorf("create user - %w", err)
}
defer func() {
// delete admin on any error to avoid partial setup
if err != nil {
logger := logging.ContextUnavailable()
logger.WithError(err).Warn("Failed to create admin user, deleting user")
if delUserErr := authService.DeleteUser(ctx, user.Username); delUserErr != nil {
logger.WithError(delUserErr).Error("Failed to delete user")
}
}
}()

if addToAdmins {
// verify the admin group exists
Expand Down
80 changes: 80 additions & 0 deletions pkg/auth/setup/setup_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package setup_test

import (
"context"
"errors"
"testing"
"time"

"github.com/treeverse/lakefs/pkg/auth"
"github.com/treeverse/lakefs/pkg/auth/crypt"
"github.com/treeverse/lakefs/pkg/auth/model"
authparams "github.com/treeverse/lakefs/pkg/auth/params"
"github.com/treeverse/lakefs/pkg/auth/setup"
"github.com/treeverse/lakefs/pkg/kv"
"github.com/treeverse/lakefs/pkg/kv/kvtest"
"github.com/treeverse/lakefs/pkg/logging"
)

func SetupService(t *testing.T, secret string) (*auth.BasicAuthService, kv.Store) {
t.Helper()
kvStore := kvtest.GetStore(context.Background(), t)
return auth.NewBasicAuthService(kvStore, crypt.NewSecretStore([]byte(secret)), authparams.ServiceCache{
Enabled: false,
}, logging.ContextUnavailable()), kvStore
}

func TestAddAdminUser(t *testing.T) {
authService, _ := SetupService(t, "secret")
ctx := context.Background()

// setup admin user
superuser := &model.SuperuserConfiguration{
User: model.User{
CreatedAt: time.Now(),
Username: "testuser",
},
AccessKeyID: "key",
SecretAccessKey: "secret",
}
_, err := setup.AddAdminUser(ctx, authService, superuser, false)
if err != nil {
t.Fatal("failed to add admin user:", err)
}

// try to add another admin user - should fail
superuser2 := &model.SuperuserConfiguration{
User: model.User{
CreatedAt: time.Now(),
Username: "testuser2",
},
AccessKeyID: "key2",
SecretAccessKey: "secret2",
}
_, err = setup.AddAdminUser(ctx, authService, superuser2, false)
expectedErr := auth.ErrAlreadyExists
if !errors.Is(err, expectedErr) {
t.Fatalf("adding another admin. err:%v, expected:%v", err, expectedErr)
}

// delete admin user should work
err = authService.DeleteUser(ctx, superuser.User.Username)
if err != nil {
t.Fatal("failed to delete admin user:", err)
}

// setup admin user again without secret access key should fail
superuser.SecretAccessKey = ""
_, err = setup.AddAdminUser(ctx, authService, superuser, false)
expectedErr = auth.ErrNotFound
if !errors.Is(err, expectedErr) {
t.Fatalf("adding admin without secret access. key err:%v, expected:%v", err, expectedErr)
}

// we should be able to re-add the admin user
superuser.SecretAccessKey = "new-secret"
_, err = setup.AddAdminUser(ctx, authService, superuser, false)
if err != nil {
t.Fatal("failed to re-add admin user:", err)
}
}

0 comments on commit 340dc93

Please sign in to comment.