From dd781ffc51a06c0bf9b8a46c9a37a23e16119f29 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Sat, 31 Dec 2022 18:27:43 +1100 Subject: [PATCH] refactor: adjust settings components --- internal/handlers/handler_webauthn_devices.go | 75 +++++++++++++------ internal/handlers/types.go | 4 + internal/mocks/storage.go | 29 +++---- internal/server/handlers.go | 6 +- internal/storage/provider.go | 2 +- internal/storage/sql_provider.go | 34 +++++---- .../storage/sql_provider_backend_postgres.go | 2 +- internal/storage/sql_provider_queries.go | 9 ++- web/src/services/Api.ts | 1 + web/src/services/Webauthn.ts | 6 +- .../WebauthnDeviceItem.tsx | 4 + 11 files changed, 107 insertions(+), 65 deletions(-) diff --git a/internal/handlers/handler_webauthn_devices.go b/internal/handlers/handler_webauthn_devices.go index ccc331ebd..077cbf32d 100644 --- a/internal/handlers/handler_webauthn_devices.go +++ b/internal/handlers/handler_webauthn_devices.go @@ -3,11 +3,13 @@ package handlers import ( "encoding/json" "errors" + "fmt" "strconv" "github.com/valyala/fasthttp" "github.com/authelia/authelia/v4/internal/middlewares" + "github.com/authelia/authelia/v4/internal/model" "github.com/authelia/authelia/v4/internal/regulation" "github.com/authelia/authelia/v4/internal/storage" ) @@ -30,11 +32,11 @@ func getWebauthnDeviceIDFromContext(ctx *middlewares.AutheliaCtx) (int, error) { return deviceID, nil } -// WebauthnDevicesGet returns all devices registered for the current user. -func WebauthnDevicesGet(ctx *middlewares.AutheliaCtx) { - userSession := ctx.GetSession() +// WebauthnDevicesGET returns all devices registered for the current user. +func WebauthnDevicesGET(ctx *middlewares.AutheliaCtx) { + s := ctx.GetSession() - devices, err := ctx.Providers.StorageProvider.LoadWebauthnDevicesByUsername(ctx, userSession.Username) + devices, err := ctx.Providers.StorageProvider.LoadWebauthnDevicesByUsername(ctx, s.Username) if err != nil && err != storage.ErrNoWebauthnDevice { ctx.Error(err, messageOperationFailed) @@ -47,19 +49,20 @@ func WebauthnDevicesGet(ctx *middlewares.AutheliaCtx) { } } -// WebauthnDeviceUpdate updates the description for a specific device for the current user. -func WebauthnDeviceUpdate(ctx *middlewares.AutheliaCtx) { - type requestPostData struct { - Description string `json:"description"` - } +// WebauthnDevicePUT updates the description for a specific device for the current user. +func WebauthnDevicePUT(ctx *middlewares.AutheliaCtx) { + var ( + bodyJSON bodyEditWebauthnDeviceRequest - var postData *requestPostData + id int + device *model.WebauthnDevice + err error + ) - userSession := ctx.GetSession() + s := ctx.GetSession() - err := json.Unmarshal(ctx.PostBody(), &postData) - if err != nil { - ctx.Logger.Errorf("Unable to parse %s update request data for user '%s': %+v", regulation.AuthTypeWebauthn, userSession.Username, err) + if err = json.Unmarshal(ctx.PostBody(), &bodyJSON); err != nil { + ctx.Logger.Errorf("Unable to parse %s update request data for user '%s': %+v", regulation.AuthTypeWebauthn, s.Username, err) ctx.SetStatusCode(fasthttp.StatusBadRequest) ctx.Error(err, messageOperationFailed) @@ -67,28 +70,54 @@ func WebauthnDeviceUpdate(ctx *middlewares.AutheliaCtx) { return } - deviceID, err := getWebauthnDeviceIDFromContext(ctx) - if err != nil { + if id, err = getWebauthnDeviceIDFromContext(ctx); err != nil { return } - if err := ctx.Providers.StorageProvider.UpdateWebauthnDeviceDescription(ctx, userSession.Username, deviceID, postData.Description); err != nil { + if device, err = ctx.Providers.StorageProvider.LoadWebauthnDeviceByID(ctx, id); err != nil { + ctx.Error(err, messageOperationFailed) + return + } + + if device.Username != s.Username { + ctx.Error(fmt.Errorf("user '%s' tried to delete device with id '%d' which belongs to '%s", s.Username, device.ID, device.Username), messageOperationFailed) + return + } + + if err = ctx.Providers.StorageProvider.UpdateWebauthnDeviceDescription(ctx, s.Username, id, bodyJSON.Description); err != nil { ctx.Error(err, messageOperationFailed) return } } -// WebauthnDeviceDelete deletes a specific device for the current user. -func WebauthnDeviceDelete(ctx *middlewares.AutheliaCtx) { - userSession := ctx.GetSession() +// WebauthnDeviceDELETE deletes a specific device for the current user. +func WebauthnDeviceDELETE(ctx *middlewares.AutheliaCtx) { + var ( + id int + device *model.WebauthnDevice + err error + ) - deviceID, err := getWebauthnDeviceIDFromContext(ctx) - if err != nil { + if id, err = getWebauthnDeviceIDFromContext(ctx); err != nil { return } - if err := ctx.Providers.StorageProvider.DeleteWebauthnDeviceByUsernameAndID(ctx, userSession.Username, deviceID); err != nil { + if device, err = ctx.Providers.StorageProvider.LoadWebauthnDeviceByID(ctx, id); err != nil { ctx.Error(err, messageOperationFailed) return } + + s := ctx.GetSession() + + if device.Username != s.Username { + ctx.Error(fmt.Errorf("user '%s' tried to delete device with id '%d' which belongs to '%s", s.Username, device.ID, device.Username), messageOperationFailed) + return + } + + if err = ctx.Providers.StorageProvider.DeleteWebauthnDevice(ctx, device.KID.String()); err != nil { + ctx.Error(err, messageOperationFailed) + return + } + + ctx.ReplyOK() } diff --git a/internal/handlers/types.go b/internal/handlers/types.go index 517d297a6..65856dfb3 100644 --- a/internal/handlers/types.go +++ b/internal/handlers/types.go @@ -39,6 +39,10 @@ type bodySignWebauthnRequest struct { WorkflowID string `json:"workflowID"` } +type bodyEditWebauthnDeviceRequest struct { + Description string `json:"description"` +} + // bodySignDuoRequest is the model of the request body of Duo 2FA authentication endpoint. type bodySignDuoRequest struct { TargetURL string `json:"targetURL"` diff --git a/internal/mocks/storage.go b/internal/mocks/storage.go index 728670914..01ac5203b 100644 --- a/internal/mocks/storage.go +++ b/internal/mocks/storage.go @@ -194,20 +194,6 @@ func (mr *MockStorageMockRecorder) DeleteWebauthnDeviceByUsername(arg0, arg1, ar return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteWebauthnDeviceByUsername", reflect.TypeOf((*MockStorage)(nil).DeleteWebauthnDeviceByUsername), arg0, arg1, arg2) } -// DeleteWebauthnDeviceByUsernameAndID mocks base method. -func (m *MockStorage) DeleteWebauthnDeviceByUsernameAndID(arg0 context.Context, arg1 string, arg2 int) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteWebauthnDeviceByUsernameAndID", arg0, arg1, arg2) - ret0, _ := ret[0].(error) - return ret0 -} - -// DeleteWebauthnDeviceByUsernameAndID indicates an expected call of DeleteWebauthnDeviceByUsernameAndID. -func (mr *MockStorageMockRecorder) DeleteWebauthnDeviceByUsernameAndID(arg0, arg1, arg2 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteWebauthnDeviceByUsernameAndID", reflect.TypeOf((*MockStorage)(nil).DeleteWebauthnDeviceByUsernameAndID), arg0, arg1, arg2) -} - // FindIdentityVerification mocks base method. func (m *MockStorage) FindIdentityVerification(arg0 context.Context, arg1 string) (bool, error) { m.ctrl.T.Helper() @@ -418,6 +404,21 @@ func (mr *MockStorageMockRecorder) LoadUserOpaqueIdentifiers(arg0 interface{}) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LoadUserOpaqueIdentifiers", reflect.TypeOf((*MockStorage)(nil).LoadUserOpaqueIdentifiers), arg0) } +// LoadWebauthnDeviceByID mocks base method. +func (m *MockStorage) LoadWebauthnDeviceByID(arg0 context.Context, arg1 int) (*model.WebauthnDevice, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LoadWebauthnDeviceByID", arg0, arg1) + ret0, _ := ret[0].(*model.WebauthnDevice) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// LoadWebauthnDeviceByID indicates an expected call of LoadWebauthnDeviceByID. +func (mr *MockStorageMockRecorder) LoadWebauthnDeviceByID(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LoadWebauthnDeviceByID", reflect.TypeOf((*MockStorage)(nil).LoadWebauthnDeviceByID), arg0, arg1) +} + // LoadWebauthnDevices mocks base method. func (m *MockStorage) LoadWebauthnDevices(arg0 context.Context, arg1, arg2 int) ([]model.WebauthnDevice, error) { m.ctrl.T.Helper() diff --git a/internal/server/handlers.go b/internal/server/handlers.go index 6e59ad200..c6fa06a59 100644 --- a/internal/server/handlers.go +++ b/internal/server/handlers.go @@ -200,9 +200,9 @@ func handleRouter(config schema.Configuration, providers middlewares.Providers) r.POST("/api/secondfactor/webauthn/assertion", middleware1FA(handlers.WebauthnAssertionPOST)) // Management of the webauthn devices. - r.GET("/api/secondfactor/webauthn/devices", middleware1FA(handlers.WebauthnDevicesGet)) - r.PUT("/api/secondfactor/webauthn/devices/{deviceID}", middleware2FA(handlers.WebauthnDeviceUpdate)) - r.DELETE("/api/secondfactor/webauthn/devices/{deviceID}", middleware2FA(handlers.WebauthnDeviceDelete)) + r.GET("/api/secondfactor/webauthn/devices", middleware1FA(handlers.WebauthnDevicesGET)) + r.PUT("/api/secondfactor/webauthn/device/{deviceID}", middleware2FA(handlers.WebauthnDevicePUT)) + r.DELETE("/api/secondfactor/webauthn/device/{deviceID}", middleware2FA(handlers.WebauthnDeviceDELETE)) } // Configure DUO api endpoint only if configuration exists. diff --git a/internal/storage/provider.go b/internal/storage/provider.go index 8e5f5f270..d9ba032b0 100644 --- a/internal/storage/provider.go +++ b/internal/storage/provider.go @@ -43,9 +43,9 @@ type Provider interface { UpdateWebauthnDeviceSignIn(ctx context.Context, id int, rpid string, lastUsedAt sql.NullTime, signCount uint32, cloneWarning bool) (err error) DeleteWebauthnDevice(ctx context.Context, kid string) (err error) DeleteWebauthnDeviceByUsername(ctx context.Context, username, description string) (err error) - DeleteWebauthnDeviceByUsernameAndID(ctx context.Context, username string, deviceID int) (err error) LoadWebauthnDevices(ctx context.Context, limit, page int) (devices []model.WebauthnDevice, err error) LoadWebauthnDevicesByUsername(ctx context.Context, username string) (devices []model.WebauthnDevice, err error) + LoadWebauthnDeviceByID(ctx context.Context, id int) (device *model.WebauthnDevice, err error) SavePreferredDuoDevice(ctx context.Context, device model.DuoDevice) (err error) DeletePreferredDuoDevice(ctx context.Context, username string) (err error) diff --git a/internal/storage/sql_provider.go b/internal/storage/sql_provider.go index b166398e1..4d896519e 100644 --- a/internal/storage/sql_provider.go +++ b/internal/storage/sql_provider.go @@ -49,6 +49,7 @@ func NewSQLProvider(config *schema.Configuration, name, driverName, dataSourceNa sqlUpsertWebauthnDevice: fmt.Sprintf(queryFmtUpsertWebauthnDevice, tableWebauthnDevices), sqlSelectWebauthnDevices: fmt.Sprintf(queryFmtSelectWebauthnDevices, tableWebauthnDevices), sqlSelectWebauthnDevicesByUsername: fmt.Sprintf(queryFmtSelectWebauthnDevicesByUsername, tableWebauthnDevices), + sqlSelectWebauthnDeviceByID: fmt.Sprintf(queryFmtSelectWebauthnDeviceByID, tableWebauthnDevices), sqlUpdateWebauthnDeviceDescriptionByUsernameAndID: fmt.Sprintf(queryFmtUpdateUpdateWebauthnDeviceDescriptionByUsernameAndID, tableWebauthnDevices), sqlUpdateWebauthnDevicePublicKey: fmt.Sprintf(queryFmtUpdateWebauthnDevicePublicKey, tableWebauthnDevices), sqlUpdateWebauthnDevicePublicKeyByUsername: fmt.Sprintf(queryFmtUpdateUpdateWebauthnDevicePublicKeyByUsername, tableWebauthnDevices), @@ -56,7 +57,6 @@ func NewSQLProvider(config *schema.Configuration, name, driverName, dataSourceNa sqlUpdateWebauthnDeviceRecordSignInByUsername: fmt.Sprintf(queryFmtUpdateWebauthnDeviceRecordSignInByUsername, tableWebauthnDevices), sqlDeleteWebauthnDevice: fmt.Sprintf(queryFmtDeleteWebauthnDevice, tableWebauthnDevices), sqlDeleteWebauthnDeviceByUsername: fmt.Sprintf(queryFmtDeleteWebauthnDeviceByUsername, tableWebauthnDevices), - sqlDeleteWebauthnDeviceByUsernameAndID: fmt.Sprintf(queryFmtDeleteWebauthnDeviceByUsernameAndID, tableWebauthnDevices), sqlDeleteWebauthnDeviceByUsernameAndDescription: fmt.Sprintf(queryFmtDeleteWebauthnDeviceByUsernameAndDescription, tableWebauthnDevices), sqlUpsertDuoDevice: fmt.Sprintf(queryFmtUpsertDuoDevice, tableDuoDevices), @@ -166,6 +166,7 @@ type SQLProvider struct { sqlUpsertWebauthnDevice string sqlSelectWebauthnDevices string sqlSelectWebauthnDevicesByUsername string + sqlSelectWebauthnDeviceByID string sqlUpdateWebauthnDeviceDescriptionByUsernameAndID string sqlUpdateWebauthnDevicePublicKey string @@ -175,7 +176,6 @@ type SQLProvider struct { sqlDeleteWebauthnDevice string sqlDeleteWebauthnDeviceByUsername string - sqlDeleteWebauthnDeviceByUsernameAndID string sqlDeleteWebauthnDeviceByUsernameAndDescription string // Table: duo_devices. @@ -803,7 +803,7 @@ func (p *SQLProvider) DeleteTOTPConfiguration(ctx context.Context, username stri func (p *SQLProvider) LoadTOTPConfiguration(ctx context.Context, username string) (config *model.TOTPConfiguration, err error) { config = &model.TOTPConfiguration{} - if err = p.db.QueryRowxContext(ctx, p.sqlSelectTOTPConfig, username).StructScan(config); err != nil { + if err = p.db.GetContext(ctx, config, p.sqlSelectTOTPConfig, username); err != nil { if errors.Is(err, sql.ErrNoRows) { return nil, ErrNoTOTPConfiguration } @@ -903,19 +903,6 @@ func (p *SQLProvider) DeleteWebauthnDeviceByUsername(ctx context.Context, userna return nil } -// DeleteWebauthnDeviceByUsernameAndID deletes a registered Webauthn device by username and ID. -func (p *SQLProvider) DeleteWebauthnDeviceByUsernameAndID(ctx context.Context, username string, deviceID int) (err error) { - if len(username) == 0 { - return fmt.Errorf("error deleting webauthn device with username '%s' and id '%d': username must not be empty", username, deviceID) - } - - if _, err = p.db.ExecContext(ctx, p.sqlDeleteWebauthnDeviceByUsernameAndID, username, deviceID); err != nil { - return fmt.Errorf("error deleting webauthn device with username '%s' and id '%d': %w", username, deviceID, err) - } - - return nil -} - // LoadWebauthnDevices loads Webauthn device registrations. func (p *SQLProvider) LoadWebauthnDevices(ctx context.Context, limit, page int) (devices []model.WebauthnDevice, err error) { devices = make([]model.WebauthnDevice, 0, limit) @@ -937,6 +924,21 @@ func (p *SQLProvider) LoadWebauthnDevices(ctx context.Context, limit, page int) return devices, nil } +// LoadWebauthnDeviceByID loads a webauthn device registration for a given id. +func (p *SQLProvider) LoadWebauthnDeviceByID(ctx context.Context, id int) (device *model.WebauthnDevice, err error) { + device = &model.WebauthnDevice{} + + if err = p.db.GetContext(ctx, device, p.sqlSelectWebauthnDeviceByID, id); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, sql.ErrNoRows + } + + return nil, fmt.Errorf("error selecting Webauthn device with id '%d': %w", id, err) + } + + return device, nil +} + // LoadWebauthnDevicesByUsername loads all webauthn devices registration for a given username. func (p *SQLProvider) LoadWebauthnDevicesByUsername(ctx context.Context, username string) (devices []model.WebauthnDevice, err error) { if err = p.db.SelectContext(ctx, &devices, p.sqlSelectWebauthnDevicesByUsername, username); err != nil { diff --git a/internal/storage/sql_provider_backend_postgres.go b/internal/storage/sql_provider_backend_postgres.go index 6bc0e3afd..44062fc09 100644 --- a/internal/storage/sql_provider_backend_postgres.go +++ b/internal/storage/sql_provider_backend_postgres.go @@ -61,6 +61,7 @@ func NewPostgreSQLProvider(config *schema.Configuration, caCertPool *x509.CertPo provider.sqlSelectWebauthnDevices = provider.db.Rebind(provider.sqlSelectWebauthnDevices) provider.sqlSelectWebauthnDevicesByUsername = provider.db.Rebind(provider.sqlSelectWebauthnDevicesByUsername) + provider.sqlSelectWebauthnDeviceByID = provider.db.Rebind(provider.sqlSelectWebauthnDeviceByID) provider.sqlUpdateWebauthnDeviceDescriptionByUsernameAndID = provider.db.Rebind(provider.sqlUpdateWebauthnDeviceDescriptionByUsernameAndID) provider.sqlUpdateWebauthnDevicePublicKey = provider.db.Rebind(provider.sqlUpdateWebauthnDevicePublicKey) provider.sqlUpdateWebauthnDevicePublicKeyByUsername = provider.db.Rebind(provider.sqlUpdateWebauthnDevicePublicKeyByUsername) @@ -68,7 +69,6 @@ func NewPostgreSQLProvider(config *schema.Configuration, caCertPool *x509.CertPo provider.sqlUpdateWebauthnDeviceRecordSignInByUsername = provider.db.Rebind(provider.sqlUpdateWebauthnDeviceRecordSignInByUsername) provider.sqlDeleteWebauthnDevice = provider.db.Rebind(provider.sqlDeleteWebauthnDevice) provider.sqlDeleteWebauthnDeviceByUsername = provider.db.Rebind(provider.sqlDeleteWebauthnDeviceByUsername) - provider.sqlDeleteWebauthnDeviceByUsernameAndID = provider.db.Rebind(provider.sqlDeleteWebauthnDeviceByUsernameAndID) provider.sqlDeleteWebauthnDeviceByUsernameAndDescription = provider.db.Rebind(provider.sqlDeleteWebauthnDeviceByUsernameAndDescription) provider.sqlSelectDuoDevice = provider.db.Rebind(provider.sqlSelectDuoDevice) diff --git a/internal/storage/sql_provider_queries.go b/internal/storage/sql_provider_queries.go index 9c073adf5..e65298018 100644 --- a/internal/storage/sql_provider_queries.go +++ b/internal/storage/sql_provider_queries.go @@ -134,6 +134,11 @@ const ( FROM %s WHERE username = ?;` + queryFmtSelectWebauthnDeviceByID = ` + SELECT id, created_at, last_used_at, rpid, username, description, kid, public_key, attestation_type, transport, aaguid, sign_count, clone_warning + FROM %s + WHERE id = ?;` + queryFmtUpdateWebauthnDevicePublicKey = ` UPDATE %s SET public_key = ? @@ -181,10 +186,6 @@ const ( DELETE FROM %s WHERE username = ?;` - queryFmtDeleteWebauthnDeviceByUsernameAndID = ` - DELETE FROM %s - WHERE username = ? AND id = ?;` - queryFmtDeleteWebauthnDeviceByUsernameAndDescription = ` DELETE FROM %s WHERE username = ? AND description = ?;` diff --git a/web/src/services/Api.ts b/web/src/services/Api.ts index d183b05ba..56c853c5d 100644 --- a/web/src/services/Api.ts +++ b/web/src/services/Api.ts @@ -18,6 +18,7 @@ export const WebauthnAttestationPath = basePath + "/api/secondfactor/webauthn/at export const WebauthnAssertionPath = basePath + "/api/secondfactor/webauthn/assertion"; export const WebauthnDevicesPath = basePath + "/api/secondfactor/webauthn/devices"; +export const WebauthnDevicePath = basePath + "/api/secondfactor/webauthn/device"; export const InitiateDuoDeviceSelectionPath = basePath + "/api/secondfactor/duo_devices"; export const CompleteDuoDeviceSelectionPath = basePath + "/api/secondfactor/duo_device"; diff --git a/web/src/services/Webauthn.ts b/web/src/services/Webauthn.ts index 1d60a7f38..b224e8cf6 100644 --- a/web/src/services/Webauthn.ts +++ b/web/src/services/Webauthn.ts @@ -24,7 +24,7 @@ import { ServiceResponse, WebauthnAssertionPath, WebauthnAttestationPath, - WebauthnDevicesPath, + WebauthnDevicePath, WebauthnIdentityFinishPath, } from "@services/Api"; import { SignInResponse } from "@services/SignIn"; @@ -399,12 +399,12 @@ export async function performAssertionCeremony( } export async function deleteDevice(deviceID: string): Promise { - let response = await axios.delete(`${WebauthnDevicesPath}/${deviceID}`); + let response = await axios.delete(`${WebauthnDevicePath}/${deviceID}`); return response.status; } export async function updateDevice(deviceID: string, description: string): Promise { - let response = await axios.put>(`${WebauthnDevicesPath}/${deviceID}`, { + let response = await axios.put>(`${WebauthnDevicePath}/${deviceID}`, { description: description, }); return response.status; diff --git a/web/src/views/Settings/TwoFactorAuthentication/WebauthnDeviceItem.tsx b/web/src/views/Settings/TwoFactorAuthentication/WebauthnDeviceItem.tsx index 6173b39b0..3028dfa69 100644 --- a/web/src/views/Settings/TwoFactorAuthentication/WebauthnDeviceItem.tsx +++ b/web/src/views/Settings/TwoFactorAuthentication/WebauthnDeviceItem.tsx @@ -44,6 +44,8 @@ export default function WebauthnDeviceItem(props: Props) { const status = await updateDevice(props.device.id, name); + console.log("Status was: ", status); + setLoadingEdit(false); if (status !== 200) { @@ -65,6 +67,8 @@ export default function WebauthnDeviceItem(props: Props) { const status = await deleteDevice(props.device.id); + console.log("Status was: ", status); + setLoadingDelete(false); if (status !== 200) {