feat(user): allow renaming security keys (webauthn/passkey) (#39413)

Closes https://github.com/go-gitea/gitea/issues/39287

Security key nicknames could only be set at registration, so a skipped
nickname left an auto-generated hex name until the key was
re-registered. Each key now has a Rename button opening a dialog with
the current nickname. A nickname used by another of the user's keys
(case-insensitive) or a blank nickname is rejected. Renames are recorded
as `user:webauth:rename` audit events.

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/audit/action.go

@@ -84,6 +84,7 @@ var (
 	UserTwoFactorRegenerate         = define("user:twofactor:regenerate", "Regenerated two-factor authentication secret for user {scope}.")
 	UserTwoFactorDisable            = define("user:twofactor:disable", "Disabled two-factor authentication for user {scope}.")
 	UserWebAuthAdd                  = define("user:webauth:add", "Added WebAuthn key {credential} for user {scope}.")
+	UserWebAuthRename               = define("user:webauth:rename", "Renamed WebAuthn key {previous_credential} of user {scope} to {credential}.")
 	UserWebAuthRemove               = define("user:webauth:remove", "Removed WebAuthn key {credential} from user {scope}.")
 	UserExternalLoginAdd            = define("user:externallogin:add", "Added external login {external_id} for user {scope} using provider {provider}.")
 	UserExternalLoginRemove         = define("user:externallogin:remove", "Removed external login from authentication source {auth_source_id} for user {scope}.")

models/auth/webauthn.go

@@ -150,9 +150,9 @@ func GetWebAuthnCredentialByName(ctx context.Context, uid int64, name string) (*
 }
 
 // GetWebAuthnCredentialByID returns WebAuthn credential by id
-func GetWebAuthnCredentialByID(ctx context.Context, id int64) (*WebAuthnCredential, error) {
+func GetWebAuthnCredentialByID(ctx context.Context, uid, id int64) (*WebAuthnCredential, error) {
 	cred := new(WebAuthnCredential)
-	if found, err := db.GetEngine(ctx).ID(id).Get(cred); err != nil {
+	if found, err := db.GetEngine(ctx).Where("user_id = ?", uid).ID(id).Get(cred); err != nil {
 		return nil, err
 	} else if !found {
 		return nil, ErrWebAuthnCredentialNotExist{ID: id}
@@ -195,8 +195,26 @@ func CreateCredential(ctx context.Context, userID int64, name string, cred *weba
 	return c, nil
 }
 
+// RenameCredential renames the user's WebAuthnCredential, names are unique per user regardless of letter case
+func RenameCredential(ctx context.Context, uid, id int64, name string) (bool, error) {
+	used, err := db.GetEngine(ctx).Where("user_id = ? AND lower_name = ? AND id != ?", uid, strings.ToLower(name), id).Exist(&WebAuthnCredential{})
+	if err != nil {
+		return false, err
+	} else if used {
+		return false, util.ErrorWrapTranslatable(
+			util.NewAlreadyExistErrorf("WebAuthn credential name already exists [uid: %d, name: %s]", uid, name),
+			"settings.webauthn_nickname_been_used",
+		)
+	}
+	updated, err := db.GetEngine(ctx).ID(id).Where("user_id=? AND `name`<>?", uid, name).Cols("name", "lower_name").Update(&WebAuthnCredential{
+		Name:      name,
+		LowerName: strings.ToLower(name),
+	})
+	return updated > 0, err
+}
+
 // DeleteCredential will delete WebAuthnCredential
-func DeleteCredential(ctx context.Context, id, userID int64) (bool, error) {
-	had, err := db.GetEngine(ctx).ID(id).Where("user_id = ?", userID).Delete(&WebAuthnCredential{})
+func DeleteCredential(ctx context.Context, uid, id int64) (bool, error) {
+	had, err := db.GetEngine(ctx).ID(id).Where("user_id = ?", uid).Delete(&WebAuthnCredential{})
 	return had > 0, err
 }

models/auth/webauthn_test.go

@@ -16,11 +16,15 @@ import (
 func TestGetWebAuthnCredentialByID(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 
-	res, err := auth_model.GetWebAuthnCredentialByID(t.Context(), 1)
+	res, err := auth_model.GetWebAuthnCredentialByID(t.Context(), 32, 1)
 	assert.NoError(t, err)
 	assert.Equal(t, "WebAuthn credential", res.Name)
 
-	_, err = auth_model.GetWebAuthnCredentialByID(t.Context(), 342432)
+	_, err = auth_model.GetWebAuthnCredentialByID(t.Context(), 99999, 1)
+	assert.Error(t, err)
+	assert.True(t, auth_model.IsErrWebAuthnCredentialNotExist(err))
+
+	_, err = auth_model.GetWebAuthnCredentialByID(t.Context(), 32, 99999)
 	assert.Error(t, err)
 	assert.True(t, auth_model.IsErrWebAuthnCredentialNotExist(err))
 }

options/locale/locale_en-US.json

@@ -87,6 +87,7 @@
   "add_all": "Add All",
   "dismiss": "Dismiss",
   "remove": "Remove",
+  "rename": "Rename",
   "remove_all": "Remove All",
   "remove_label_str": "Remove item \"%s\"",
   "edit": "Edit",
@@ -798,7 +799,6 @@
   "settings.add_key_success": "The SSH key \"%s\" has been added.",
   "settings.add_gpg_key_success": "The GPG key \"%s\" has been added.",
   "settings.add_principal_success": "The SSH certificate principal \"%s\" has been added.",
-  "settings.delete_key": "Remove",
   "settings.ssh_key_deletion": "Remove SSH Key",
   "settings.gpg_key_deletion": "Remove GPG Key",
   "settings.ssh_principal_deletion": "Remove SSH Certificate Principal",
@@ -907,6 +907,7 @@
   "settings.webauthn_desc": "Security keys are hardware devices containing cryptographic keys. They can be used for two-factor authentication. Security keys must support the <a rel=\"noreferrer\" target=\"_blank\" href=\"%s\">WebAuthn Authenticator</a> standard.",
   "settings.webauthn_register_key": "Add Security Key",
   "settings.webauthn_nickname": "Nickname",
+  "settings.webauthn_nickname_been_used": "A security key with the same nickname already exists.",
   "settings.webauthn_delete_key": "Remove Security Key",
   "settings.webauthn_delete_key_desc": "If you remove a security key, you can no longer sign in with it. Continue?",
   "settings.webauthn_key_loss_warning": "If you lose your security keys, you will lose access to your account.",

routers/web/user/setting/security/webauthn.go

@@ -139,20 +139,49 @@ func WebauthnRegisterPost(ctx *context.Context) {
 	ctx.JSON(http.StatusCreated, cred)
 }
 
-// WebauthnDelete deletes an security key by id
+// WebauthnRename changes the nickname of a security key
+func WebauthnRename(ctx *context.Context) {
+	if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageMFA) {
+		ctx.HTTPError(http.StatusNotFound)
+		return
+	}
+
+	form := context.GetFetchActionForm[*forms.WebauthnRenameForm](ctx)
+	if form == nil {
+		return
+	}
+
+	cred, err := auth.GetWebAuthnCredentialByID(ctx, ctx.Doer.ID, ctx.FormInt64("id"))
+	if err != nil {
+		ctx.JSONErrorAuto(err)
+		return
+	}
+
+	renamed, err := auth.RenameCredential(ctx, ctx.Doer.ID, form.ID, form.Name)
+	if err != nil {
+		ctx.JSONErrorAuto(err)
+		return
+	}
+	if renamed {
+		audit.Record(ctx, audit_model.UserWebAuthRename, ctx.Doer, "previous_credential", cred.Name, "credential", form.Name)
+	}
+	ctx.JSONRedirect(setting.AppSubURL + "/user/settings/security")
+}
+
+// WebauthnDelete deletes a security key by id
 func WebauthnDelete(ctx *context.Context) {
 	if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageMFA) {
 		ctx.HTTPError(http.StatusNotFound)
 		return
 	}
 
-	cred, err := auth.GetWebAuthnCredentialByID(ctx, ctx.FormInt64("id"))
+	cred, err := auth.GetWebAuthnCredentialByID(ctx, ctx.Doer.ID, ctx.FormInt64("id"))
 	if err != nil {
-		ctx.NotFoundOrServerError("GetWebAuthnCredentialByID", auth.IsErrWebAuthnCredentialNotExist, err)
+		ctx.JSONErrorAuto(err)
 		return
 	}
 
-	if ok, err := auth.DeleteCredential(ctx, cred.ID, ctx.Doer.ID); err != nil {
+	if ok, err := auth.DeleteCredential(ctx, ctx.Doer.ID, cred.ID); err != nil {
 		ctx.ServerError("DeleteCredential", err)
 		return
 	} else if ok {

routers/web/web.go

@@ -660,6 +660,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 			m.Group("/webauthn", func() {
 				m.Post("/request_register", web.Bind[*forms.WebauthnRegistrationForm](), security.WebAuthnRegister)
 				m.Post("/register", security.WebauthnRegisterPost)
+				m.Post("/rename", security.WebauthnRename)
 				m.Post("/delete", security.WebauthnDelete)
 			})
 			m.Group("/openid", func() {

services/forms/user_form.go

@@ -287,7 +287,14 @@ type TwoFactorScratchAuthForm struct {
 // WebauthnRegistrationForm for reserving an WebAuthn name
 type WebauthnRegistrationForm struct {
 	middleware.FormDefaultValidator
-	Name string `binding:"Required"`
+	Name string `binding:"TrimSpace;MaxSize(255)"`
+}
+
+// WebauthnRenameForm for renaming a WebAuthn credential
+type WebauthnRenameForm struct {
+	middleware.FormDefaultValidator
+	ID   int64  `binding:"Required"`
+	Name string `binding:"TrimSpace;Required;MaxSize(255)"`
 }
 
 // PackageSettingForm form for package settings

templates/repo/settings/deploy_key_list.tmpl

@@ -33,7 +33,7 @@
 						</button>
 					{{end}}
 					<button type="button" class="ui red tiny button link-action" data-modal-confirm="#repo-deploy-key-delete-modal" data-url="{{ctx.RootData.Link}}/delete?id={{$key.ID}}">
-						{{ctx.Locale.Tr "settings.delete_key"}}
+						{{ctx.Locale.Tr "remove"}}
 					</button>
 				</div>
 			</div>

templates/user/settings/applications_oauth2_list.tmpl

@@ -27,7 +27,7 @@
 						<button type="button" class="ui red tiny button link-action" data-modal-confirm="#remove-gitea-oauth2-application"
 								data-url="{{$.Link}}/oauth2/{{.ID}}/delete">
 							{{svg "octicon-trash"}}
-							{{ctx.Locale.Tr "settings.delete_key"}}
+							{{ctx.Locale.Tr "remove"}}
 						</button>
 					{{end}}
 				</div>

templates/user/settings/keys_gpg.tmpl

@@ -77,7 +77,7 @@
 				</div>
 				<div class="item-trailing">
 					<button type="button" class="ui red tiny button link-action" data-modal-confirm="#delete-gpg" data-url="{{$.Link}}/delete?type=gpg&id={{.ID}}">
-						{{ctx.Locale.Tr "settings.delete_key"}}
+						{{ctx.Locale.Tr "remove"}}
 					</button>
 					{{if and (not .Verified) (ne $.VerifyingID .KeyID)}}
 						<a class="ui primary tiny button" href="?verify_gpg={{.KeyID}}">{{ctx.Locale.Tr "settings.gpg_key_verify"}}</a>

templates/user/settings/keys_principal.tmpl

@@ -27,7 +27,7 @@
 					</div>
 					<div class="item-trailing">
 						<button type="button" class="ui red tiny button link-action" data-modal-confirm="#delete-principal" data-url="{{$.Link}}/delete?type=principal&id={{.ID}}">
-							{{ctx.Locale.Tr "settings.delete_key"}}
+							{{ctx.Locale.Tr "remove"}}
 						</button>
 					</div>
 				</div>

templates/user/settings/keys_ssh.tmpl

@@ -57,7 +57,7 @@
 				</div>
 				<div class="item-trailing">
 					<button type="button" class="ui red tiny button link-action{{if index $.ExternalKeys $index}} disabled{{end}}" data-modal-confirm="#delete-ssh" data-url="{{$.Link}}/delete?type=ssh&id={{.ID}}"{{if index $.ExternalKeys $index}} title="{{ctx.Locale.Tr "settings.ssh_externally_managed"}}"{{end}}>
-						{{ctx.Locale.Tr "settings.delete_key"}}
+						{{ctx.Locale.Tr "remove"}}
 					</button>
 					{{if and (not .Verified) (ne $.VerifyingFingerprint .Fingerprint)}}
 						<a class="ui primary tiny button" href="?verify_ssh={{.Fingerprint}}">{{ctx.Locale.Tr "settings.ssh_key_verify"}}</a>

templates/user/settings/security/accountlinks.tmpl

@@ -41,7 +41,7 @@
 				</div>
 				<div class="item-trailing">
 					<button type="button" class="ui red tiny button link-action" data-modal-confirm="#delete-account-link" data-url="{{AppSubUrl}}/user/settings/security/account_link?id={{$loginSource.ID}}">
-						{{ctx.Locale.Tr "settings.delete_key"}}
+						{{ctx.Locale.Tr "remove"}}
 					</button>
 				</div>
 			</div>

templates/user/settings/security/openid.tmpl

@@ -30,7 +30,7 @@
 						{{end}}
 					</form>
 					<button type="button" class="ui red tiny button link-action" data-modal-confirm="#delete-openid" data-url="{{AppSubUrl}}/user/settings/security/openid/delete?id={{.ID}}">
-						{{ctx.Locale.Tr "settings.delete_key"}}
+						{{ctx.Locale.Tr "remove"}}
 					</button>
 				</div>
 			</div>

templates/user/settings/security/webauthn.tmpl

@@ -16,13 +16,18 @@
 					</div>
 				</div>
 				<div class="item-trailing">
+					<button type="button" class="ui tiny button show-modal" data-modal="#rename-webauthn-nickname" data-modal-id="{{.ID}}" data-modal-name="{{.Name}}">
+					{{ctx.Locale.Tr "rename"}}
+					</button>
 					<button type="button" class="ui red tiny button link-action" data-modal-confirm="#delete-registration" data-url="{{$.Link}}/webauthn/delete?id={{.ID}}">
-					{{ctx.Locale.Tr "settings.delete_key"}}
+					{{ctx.Locale.Tr "remove"}}
 					</button>
 				</div>
 			</div>
 		{{end}}
 	</div>
+</div>
+<div class="ui attached segment">
 	<div class="ui form">
 		<div class="required field">
 			<label for="nickname">{{ctx.Locale.Tr "settings.webauthn_nickname"}}</label>
@@ -30,6 +35,20 @@
 		</div>
 		<button type="button" id="register-webauthn" class="ui primary button">{{svg "octicon-key"}} {{ctx.Locale.Tr "settings.webauthn_register_key"}}</button>
 	</div>
+	<div class="ui small modal" id="rename-webauthn-nickname">
+		<div class="header">
+			{{svg "octicon-pencil"}}
+			{{ctx.Locale.Tr "rename"}}
+		</div>
+		<form class="content ui form form-fetch-action" action="{{$.Link}}/webauthn/rename" method="post">
+			<input type="hidden" name="id">
+			<div class="required field">
+				<label>{{ctx.Locale.Tr "settings.webauthn_nickname"}}</label>
+				<input name="name" type="text" maxlength="255" required>
+			</div>
+			{{template "base/modal_actions_confirm" (dict "ModalButtonTypes" "confirm")}}
+		</form>
+	</div>
 	<div class="ui small modal" id="delete-registration">
 		<div class="header">
 			{{svg "octicon-trash"}}

tests/e2e/webauthn.test.ts

@@ -5,7 +5,7 @@ const signedIn = /^(?!.*\/user\/(login|webauthn))/; // the target of a finished
 
 async function registerKey(page: Page, nickname: string) {
   await page.goto('/user/settings/security');
-  await page.getByLabel('Nickname').fill(nickname);
+  await page.getByRole('textbox', {name: 'Nickname'}).fill(nickname);
   await page.getByRole('button', {name: 'Add Security Key'}).click();
 }
 

tests/integration/webauthn_test.go

@@ -4,13 +4,21 @@
 package integration
 
 import (
+	"fmt"
 	"net/http"
+	"net/http/httptest"
+	"strconv"
 	"testing"
 
+	auth_model "gitea.dev/models/auth"
+	"gitea.dev/models/unittest"
+	"gitea.dev/modules/test"
 	"gitea.dev/tests"
 
 	"github.com/go-webauthn/webauthn/protocol"
+	"github.com/go-webauthn/webauthn/webauthn"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 // one credential serves both logins, so their user verification is coupled
@@ -36,3 +44,36 @@ func TestWebAuthnUserVerification(t *testing.T) {
 	req = NewRequestWithJSON(t, "POST", "/user/webauthn/passkey/login", map[string]string{"bogus": "1"})
 	session.MakeRequest(t, req, http.StatusForbidden)
 }
+
+func TestWebAuthnRename(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	session := loginUser(t, "user2")
+	cred, err := auth_model.CreateCredential(t.Context(), 2, "My credential", &webauthn.Credential{ID: []byte("mine")})
+	require.NoError(t, err)
+	_, err = auth_model.CreateCredential(t.Context(), 2, "Other credential", &webauthn.Credential{ID: []byte("other")})
+	require.NoError(t, err)
+
+	htmlDoc := NewHTMLParser(t, session.MakeRequest(t, NewRequest(t, "GET", "/user/settings/security"), http.StatusOK).Body)
+	AssertHTMLElement(t, htmlDoc, `#rename-webauthn-nickname form[action$="/webauthn/rename"]`, true)
+	assert.Equal(t, "My credential", htmlDoc.Find(fmt.Sprintf(`[data-modal="#rename-webauthn-nickname"][data-modal-id="%d"]`, cred.ID)).AttrOr("data-modal-name", ""))
+
+	rename := func(id int64, name string, expectedStatus int) *httptest.ResponseRecorder {
+		req := NewRequestWithValues(t, "POST", "/user/settings/security/webauthn/rename", map[string]string{"id": strconv.FormatInt(id, 10), "name": name})
+		return session.MakeRequest(t, req, expectedStatus)
+	}
+
+	rename(cred.ID, "Renamed credential", http.StatusOK)
+	unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{ID: cred.ID, Name: "Renamed credential", LowerName: "renamed credential"})
+
+	rename(cred.ID, "RENAMED credential", http.StatusOK)
+	unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{ID: cred.ID, Name: "RENAMED credential"})
+
+	resp := rename(cred.ID, "other CREDENTIAL", http.StatusBadRequest)
+	assert.Equal(t, "A security key with the same nickname already exists.", test.ParseJSONError(resp.Body.Bytes()).ErrorMessage)
+	unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{ID: cred.ID, Name: "RENAMED credential"})
+
+	rename(cred.ID, "  ", http.StatusBadRequest)
+	rename(1, "Stolen credential", http.StatusNotFound)
+	unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{ID: 1, UserID: 32, Name: "WebAuthn credential"})
+}