From 6dab90472b7683898a5f0e0685577dc7d37caf32 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 9 Mar 2026 14:03:41 -0700 Subject: [PATCH] admin: fix access key creation UX (#8579) * admin: remove misleading "secret key only shown once" warning The access key details modal already allows viewing both the access key and secret key at any time, so the warning about the secret key only being displayed once is incorrect and misleading. * admin: allow specifying custom access key and secret key Add optional access_key and secret_key fields to the create access key API. When provided, the specified keys are used instead of generating random ones. The UI now shows a form with optional fields when creating a new key, with a note that leaving them blank auto-generates keys. * admin: check access key uniqueness before creating Access keys must be globally unique across all users since S3 auth looks them up in a single global map. Add an explicit check using GetUserByAccessKey before creating, so the user gets a clear error ("access key is already in use") rather than a generic store error. * Update object_store_users_templ.go * admin: address review feedback for access key creation Handler: - Use decodeJSONBody/newJSONMaxReader instead of raw json.Decode to enforce request size limits and handle malformed JSON properly - Return 409 Conflict for duplicate access keys, 400 Bad Request for validation errors, instead of generic 500 Backend: - Validate access key length (4-128 chars) and secret key length (8-128 chars) when user-provided Frontend: - Extract resetCreateKeyForm() helper to avoid duplicated cleanup logic - Wire resetCreateKeyForm to accessKeysModal hidden.bs.modal event so form state is always cleared when modal is dismissed - Change secret key input to type="password" with a visibility toggle * admin: guard against nil request and handle GetUserByAccessKey errors - Add nil check for the CreateAccessKeyRequest pointer before dereferencing, defaulting to an empty request (auto-generate both keys). - Handle non-"not found" errors from GetUserByAccessKey explicitly instead of silently proceeding, so store errors (e.g. db connection failures) surface rather than being swallowed. * Update object_store_users_templ.go * admin: fix access key uniqueness check with gRPC store GetUserByAccessKey returns a gRPC NotFound status error (not the sentinel credential.ErrAccessKeyNotFound) when using the gRPC store, causing the uniqueness check to fail with a spurious error. Treat the lookup as best-effort: only reject when a user is found (err == nil). Any error (not-found via any store, connectivity issues) falls through to the store's own CreateAccessKey which enforces uniqueness definitively. * admin: fix error handling and input validation for access key creation Backend: - Remove access key value from the duplicate-key error message to avoid logging the caller-supplied identifier. Handler: - Handle empty POST body (io.EOF) as a valid request that auto-generates both keys, instead of rejecting it as malformed JSON. - Return 404 for "not found" errors (e.g. non-existent user) instead of collapsing them into a 500. Frontend: - Add minlength/maxlength attributes matching backend constraints (access key 4-128, secret key 8-128). - Call reportValidity() before submitting so invalid lengths are caught client-side without a round trip. * admin: use sentinel errors and fix GetUserByAccessKey error handling Backend (user_management.go): - Define sentinel errors (ErrAccessKeyInUse, ErrUserNotFound, ErrInvalidInput) and wrap them in returned errors so callers can use errors.Is. - Handle GetUserByAccessKey errors properly: check the sentinel credential.ErrAccessKeyNotFound first, then fall back to string matching for stores (gRPC) that return non-sentinel not-found errors. Surface unexpected errors instead of silently proceeding. Handler (user_handlers.go): - Replace fragile strings.Contains error matching with errors.Is against the new dash sentinels. Frontend (object_store_users.templ): - Add double-submit guard (isCreatingKey flag + button disabling) to prevent duplicate access key creation requests. --- weed/admin/dash/admin_data.go | 5 + weed/admin/dash/user_management.go | 51 +++++++++- weed/admin/handlers/user_handlers.go | 26 ++++- weed/admin/static/js/admin.js | 4 - weed/admin/view/app/object_store_users.templ | 99 +++++++++++++++++-- .../view/app/object_store_users_templ.go | 2 +- 6 files changed, 167 insertions(+), 20 deletions(-) diff --git a/weed/admin/dash/admin_data.go b/weed/admin/dash/admin_data.go index 46a7ddb14..3cf6abf16 100644 --- a/weed/admin/dash/admin_data.go +++ b/weed/admin/dash/admin_data.go @@ -80,6 +80,11 @@ type AccessKeyInfo struct { CreatedAt time.Time `json:"created_at"` } +type CreateAccessKeyRequest struct { + AccessKey string `json:"access_key"` + SecretKey string `json:"secret_key"` +} + type UpdateAccessKeyStatusRequest struct { Status string `json:"status" binding:"required"` } diff --git a/weed/admin/dash/user_management.go b/weed/admin/dash/user_management.go index ecae2169b..7832e501f 100644 --- a/weed/admin/dash/user_management.go +++ b/weed/admin/dash/user_management.go @@ -4,13 +4,21 @@ import ( "context" "crypto/rand" "encoding/base64" + "errors" "fmt" + "strings" "time" "github.com/seaweedfs/seaweedfs/weed/credential" "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" ) +var ( + ErrAccessKeyInUse = errors.New("access key already in use") + ErrUserNotFound = errors.New("user not found") + ErrInvalidInput = errors.New("invalid input") +) + // CreateObjectStoreUser creates a new user using the credential manager func (s *AdminServer) CreateObjectStoreUser(req CreateUserRequest) (*ObjectStoreUser, error) { if s.credentialManager == nil { @@ -219,7 +227,7 @@ func (s *AdminServer) GetObjectStoreUserDetails(username string) (*UserDetails, } // CreateAccessKey creates a new access key for a user -func (s *AdminServer) CreateAccessKey(username string) (*AccessKeyInfo, error) { +func (s *AdminServer) CreateAccessKey(username string, req *CreateAccessKeyRequest) (*AccessKeyInfo, error) { if s.credentialManager == nil { return nil, fmt.Errorf("credential manager not available") } @@ -230,14 +238,41 @@ func (s *AdminServer) CreateAccessKey(username string) (*AccessKeyInfo, error) { _, err := s.credentialManager.GetUser(ctx, username) if err != nil { if err == credential.ErrUserNotFound { - return nil, fmt.Errorf("user %s not found", username) + return nil, fmt.Errorf("user %s: %w", username, ErrUserNotFound) } return nil, fmt.Errorf("failed to get user: %w", err) } - // Generate new access key - accessKey := generateAccessKey() - secretKey := generateSecretKey() + if req == nil { + req = &CreateAccessKeyRequest{} + } + + // Validate provided keys + if req.AccessKey != "" && (len(req.AccessKey) < 4 || len(req.AccessKey) > 128) { + return nil, fmt.Errorf("access key must be between 4 and 128 characters: %w", ErrInvalidInput) + } + if req.SecretKey != "" && (len(req.SecretKey) < 8 || len(req.SecretKey) > 128) { + return nil, fmt.Errorf("secret key must be between 8 and 128 characters: %w", ErrInvalidInput) + } + + // Use provided keys or generate new ones + accessKey := req.AccessKey + if accessKey == "" { + accessKey = generateAccessKey() + } + secretKey := req.SecretKey + if secretKey == "" { + secretKey = generateSecretKey() + } + + // Verify access key is globally unique + existingUser, err := s.credentialManager.GetUserByAccessKey(ctx, accessKey) + if existingUser != nil { + return nil, ErrAccessKeyInUse + } + if err != nil && !errors.Is(err, credential.ErrAccessKeyNotFound) && !isNotFoundError(err) { + return nil, fmt.Errorf("failed to check access key uniqueness: %w", err) + } credential := &iam_pb.Credential{ AccessKey: accessKey, @@ -382,6 +417,12 @@ func (s *AdminServer) UpdateUserPolicies(username string, actions []string) erro return nil } +// isNotFoundError checks for "not found" in the error message as a fallback +// for stores (e.g. gRPC) that don't return the credential.ErrAccessKeyNotFound sentinel. +func isNotFoundError(err error) bool { + return err != nil && strings.Contains(strings.ToLower(err.Error()), "not found") +} + // Helper functions for generating keys and IDs func generateAccessKey() string { // Generate 20-character access key (AWS standard) diff --git a/weed/admin/handlers/user_handlers.go b/weed/admin/handlers/user_handlers.go index fa08a71fc..5b8b0443d 100644 --- a/weed/admin/handlers/user_handlers.go +++ b/weed/admin/handlers/user_handlers.go @@ -1,7 +1,9 @@ package handlers import ( + "errors" "fmt" + "io" "net/http" "time" @@ -155,10 +157,30 @@ func (h *UserHandlers) CreateAccessKey(w http.ResponseWriter, r *http.Request) { return } - accessKey, err := h.adminServer.CreateAccessKey(username) + var req *dash.CreateAccessKeyRequest + var body dash.CreateAccessKeyRequest + if err := decodeJSONBody(newJSONMaxReader(w, r), &body); err != nil { + if !errors.Is(err, io.EOF) { + writeJSONError(w, http.StatusBadRequest, "Invalid request: "+err.Error()) + return + } + // Empty body: auto-generate both keys + } else { + req = &body + } + + accessKey, err := h.adminServer.CreateAccessKey(username, req) if err != nil { glog.Errorf("Failed to create access key for user %s: %v", username, err) - writeJSONError(w, http.StatusInternalServerError, "Failed to create access key: "+err.Error()) + if errors.Is(err, dash.ErrAccessKeyInUse) { + writeJSONError(w, http.StatusConflict, err.Error()) + } else if errors.Is(err, dash.ErrUserNotFound) { + writeJSONError(w, http.StatusNotFound, err.Error()) + } else if errors.Is(err, dash.ErrInvalidInput) { + writeJSONError(w, http.StatusBadRequest, err.Error()) + } else { + writeJSONError(w, http.StatusInternalServerError, "Failed to create access key: "+err.Error()) + } return } diff --git a/weed/admin/static/js/admin.js b/weed/admin/static/js/admin.js index 316f9d2a0..ec809f9d5 100644 --- a/weed/admin/static/js/admin.js +++ b/weed/admin/static/js/admin.js @@ -2196,10 +2196,6 @@ function showNewAccessKeyModal(accessKeyData) { Success! Your new access key has been created. -
- - Important: This is the only time the secret key will be displayed. Please save it securely. -
diff --git a/weed/admin/view/app/object_store_users.templ b/weed/admin/view/app/object_store_users.templ index 86715dc54..3773c8797 100644 --- a/weed/admin/view/app/object_store_users.templ +++ b/weed/admin/view/app/object_store_users.templ @@ -442,10 +442,32 @@ templ ObjectStoreUsers(data dash.ObjectStoreUsersData) {
Access Keys for
-
+
@@ -1347,10 +1369,66 @@ templ ObjectStoreUsers(data dash.ObjectStoreUsersData) { } } + // Reset and hide the create key form + function resetCreateKeyForm() { + document.getElementById('createKeyForm').style.display = 'none'; + document.getElementById('newAccessKeyInput').value = ''; + document.getElementById('newSecretKeyInput').value = ''; + document.getElementById('newSecretKeyInput').type = 'password'; + document.getElementById('secretKeyToggleIcon').className = 'fas fa-eye'; + } + + // Toggle create key form visibility + function toggleCreateKeyForm() { + const form = document.getElementById('createKeyForm'); + if (form.style.display === 'none') { + form.style.display = 'block'; + } else { + resetCreateKeyForm(); + } + } + + // Toggle secret key input visibility + function toggleSecretKeyVisibility() { + const input = document.getElementById('newSecretKeyInput'); + const icon = document.getElementById('secretKeyToggleIcon'); + if (input.type === 'password') { + input.type = 'text'; + icon.className = 'fas fa-eye-slash'; + } else { + input.type = 'password'; + icon.className = 'fas fa-eye'; + } + } + + // Reset form when modal is dismissed + document.getElementById('accessKeysModal').addEventListener('hidden.bs.modal', resetCreateKeyForm); + // Create new access key + var isCreatingKey = false; async function createAccessKey() { + if (isCreatingKey) return; + const username = document.getElementById('accessKeysUsername').textContent; - + const accessKeyInput = document.getElementById('newAccessKeyInput'); + const secretKeyInput = document.getElementById('newSecretKeyInput'); + if ((accessKeyInput.value.trim() && !accessKeyInput.reportValidity()) || + (secretKeyInput.value.trim() && !secretKeyInput.reportValidity())) { + return; + } + const accessKey = accessKeyInput.value.trim(); + const secretKey = secretKeyInput.value.trim(); + + const body = {}; + if (accessKey) body.access_key = accessKey; + if (secretKey) body.secret_key = secretKey; + + isCreatingKey = true; + const createBtn = document.querySelector('#createKeyForm .btn-primary'); + const cancelBtn = document.querySelector('#createKeyForm .btn-secondary'); + if (createBtn) createBtn.disabled = true; + if (cancelBtn) cancelBtn.disabled = true; + try { const encodedUsername = encodeURIComponent(username); const response = await fetch(`/api/users/${encodedUsername}/access-keys`, { @@ -1358,19 +1436,20 @@ templ ObjectStoreUsers(data dash.ObjectStoreUsersData) { headers: { 'Content-Type': 'application/json', }, - body: JSON.stringify({}) + body: JSON.stringify(body) }); - + if (response.ok) { const result = await response.json(); - - // Show the new access key details (IMPORTANT: secret key is only shown once!) + + // Show the new access key details if (result.access_key) { showNewAccessKeyModal(result.access_key); } - + showSuccessMessage('Access key created successfully'); - + resetCreateKeyForm(); + // Refresh access keys display refreshAccessKeysList(username); } else { @@ -1380,6 +1459,10 @@ templ ObjectStoreUsers(data dash.ObjectStoreUsersData) { } catch (error) { console.error('Error creating access key:', error); showAlert('Failed to create access key: ' + error.message, 'error'); + } finally { + isCreatingKey = false; + if (createBtn) createBtn.disabled = false; + if (cancelBtn) cancelBtn.disabled = false; } } diff --git a/weed/admin/view/app/object_store_users_templ.go b/weed/admin/view/app/object_store_users_templ.go index 7f2a73c34..28cee4884 100644 --- a/weed/admin/view/app/object_store_users_templ.go +++ b/weed/admin/view/app/object_store_users_templ.go @@ -193,7 +193,7 @@ func ObjectStoreUsers(data dash.ObjectStoreUsersData) templ.Component { if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err } - templ_7745c5c3_Err = templruntime.WriteString(templ_7745c5c3_Buffer, 15, "
Create New User
Hold Ctrl/Cmd to select multiple permissions
Apply selected permissions to specific buckets or all buckets
Hold Ctrl/Cmd to select multiple buckets
Hold Ctrl/Cmd to select multiple policies
Edit User
Apply selected permissions to specific buckets or all buckets
Hold Ctrl/Cmd to select multiple buckets
User Details
Manage Access Keys
Access Keys for
") + templ_7745c5c3_Err = templruntime.WriteString(templ_7745c5c3_Buffer, 15, "
Create New User
Hold Ctrl/Cmd to select multiple permissions
Apply selected permissions to specific buckets or all buckets
Hold Ctrl/Cmd to select multiple buckets
Hold Ctrl/Cmd to select multiple policies
Edit User
Apply selected permissions to specific buckets or all buckets
Hold Ctrl/Cmd to select multiple buckets
User Details
Manage Access Keys
Access Keys for

Leave blank to auto-generate.

") if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err }