From a6d2fd45df64dea2a78284c112392952fcc1b1d4 Mon Sep 17 00:00:00 2001 From: Hayden <64056131+hay-kot@users.noreply.github.com> Date: Sun, 9 Oct 2022 09:23:21 -0800 Subject: [PATCH] feat: change password (#35) * refactor: implement factories for testing * add additional factories * change protection for dropFields * prevent timed attacks on login * use switch instead of else-if * API implementation for changing password * add change-password dialog --- Taskfile.yml | 2 +- backend/app/api/docs/docs.go | 40 ++++ backend/app/api/docs/swagger.json | 40 ++++ backend/app/api/docs/swagger.yaml | 24 ++ backend/app/api/routes.go | 1 + backend/app/api/v1/v1_ctrl_auth.go | 9 +- backend/app/api/v1/v1_ctrl_user.go | 34 +++ backend/internal/repo/repo_users.go | 4 + backend/internal/services/service_user.go | 34 ++- frontend/lib/api/__test__/factories/index.ts | 80 +++++++ frontend/lib/api/__test__/public.test.ts | 26 +-- frontend/lib/api/__test__/test-utils.ts | 25 +-- frontend/lib/api/__test__/user/labels.test.ts | 16 +- .../lib/api/__test__/user/locations.test.ts | 14 +- frontend/lib/api/__test__/user/user.test.ts | 27 +++ frontend/lib/api/base/base-api.ts | 2 +- frontend/lib/api/classes/users.ts | 12 +- frontend/lib/api/types/data-contracts.ts | 5 + frontend/pages/profile.vue | 212 +++++++++++------- 19 files changed, 458 insertions(+), 149 deletions(-) create mode 100644 frontend/lib/api/__test__/factories/index.ts create mode 100644 frontend/lib/api/__test__/user/user.test.ts diff --git a/Taskfile.yml b/Taskfile.yml index 0b8049e..a4b1972 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -20,7 +20,7 @@ tasks: --path ./backend/app/api/docs/swagger.json \ --output ./frontend/lib/api/types - python3 ./scripts/process-types.py ./frontend/lib/api/types/data-contracts.ts + - python3 ./scripts/process-types.py ./frontend/lib/api/types/data-contracts.ts sources: - "./backend/app/api/**/*" - "./backend/internal/repo/**/*" diff --git a/backend/app/api/docs/docs.go b/backend/app/api/docs/docs.go index 784c591..32af2e3 100644 --- a/backend/app/api/docs/docs.go +++ b/backend/app/api/docs/docs.go @@ -822,6 +822,35 @@ const docTemplate = `{ } } }, + "/v1/users/change-password": { + "put": { + "security": [ + { + "Bearer": [] + } + ], + "tags": [ + "User" + ], + "summary": "Updates the users password", + "parameters": [ + { + "description": "Password Payload", + "name": "payload", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/v1.ChangePassword" + } + } + ], + "responses": { + "204": { + "description": "" + } + } + } + }, "/v1/users/login": { "post": { "consumes": [ @@ -1579,6 +1608,17 @@ const docTemplate = `{ } } }, + "v1.ChangePassword": { + "type": "object", + "properties": { + "current": { + "type": "string" + }, + "new": { + "type": "string" + } + } + }, "v1.GroupInvitation": { "type": "object", "properties": { diff --git a/backend/app/api/docs/swagger.json b/backend/app/api/docs/swagger.json index 2e6dea6..831066f 100644 --- a/backend/app/api/docs/swagger.json +++ b/backend/app/api/docs/swagger.json @@ -814,6 +814,35 @@ } } }, + "/v1/users/change-password": { + "put": { + "security": [ + { + "Bearer": [] + } + ], + "tags": [ + "User" + ], + "summary": "Updates the users password", + "parameters": [ + { + "description": "Password Payload", + "name": "payload", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/v1.ChangePassword" + } + } + ], + "responses": { + "204": { + "description": "" + } + } + } + }, "/v1/users/login": { "post": { "consumes": [ @@ -1571,6 +1600,17 @@ } } }, + "v1.ChangePassword": { + "type": "object", + "properties": { + "current": { + "type": "string" + }, + "new": { + "type": "string" + } + } + }, "v1.GroupInvitation": { "type": "object", "properties": { diff --git a/backend/app/api/docs/swagger.yaml b/backend/app/api/docs/swagger.yaml index ae460a6..e52d258 100644 --- a/backend/app/api/docs/swagger.yaml +++ b/backend/app/api/docs/swagger.yaml @@ -353,6 +353,13 @@ definitions: version: type: string type: object + v1.ChangePassword: + properties: + current: + type: string + new: + type: string + type: object v1.GroupInvitation: properties: expiresAt: @@ -877,6 +884,23 @@ paths: summary: Retrieves the basic information about the API tags: - Base + /v1/users/change-password: + put: + parameters: + - description: Password Payload + in: body + name: payload + required: true + schema: + $ref: '#/definitions/v1.ChangePassword' + responses: + "204": + description: "" + security: + - Bearer: [] + summary: Updates the users password + tags: + - User /v1/users/login: post: consumes: diff --git a/backend/app/api/routes.go b/backend/app/api/routes.go index b9e93be..3b131b2 100644 --- a/backend/app/api/routes.go +++ b/backend/app/api/routes.go @@ -65,6 +65,7 @@ func (a *app) newRouter(repos *repo.AllRepos) *chi.Mux { r.Put(v1Base("/users/self/password"), v1Ctrl.HandleUserUpdatePassword()) r.Post(v1Base("/users/logout"), v1Ctrl.HandleAuthLogout()) r.Get(v1Base("/users/refresh"), v1Ctrl.HandleAuthRefresh()) + r.Put(v1Base("/users/self/change-password"), v1Ctrl.HandleUserSelfChangePassword()) r.Post(v1Base("/groups/invitations"), v1Ctrl.HandleGroupInvitationsCreate()) diff --git a/backend/app/api/v1/v1_ctrl_auth.go b/backend/app/api/v1/v1_ctrl_auth.go index 253dee2..0a4f990 100644 --- a/backend/app/api/v1/v1_ctrl_auth.go +++ b/backend/app/api/v1/v1_ctrl_auth.go @@ -36,7 +36,8 @@ func (ctrl *V1Controller) HandleAuthLogin() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { loginForm := &LoginForm{} - if r.Header.Get("Content-Type") == server.ContentFormUrlEncoded { + switch r.Header.Get("Content-Type") { + case server.ContentFormUrlEncoded: err := r.ParseForm() if err != nil { server.Respond(w, http.StatusBadRequest, server.Wrap(err)) @@ -46,7 +47,7 @@ func (ctrl *V1Controller) HandleAuthLogin() http.HandlerFunc { loginForm.Username = r.PostFormValue("username") loginForm.Password = r.PostFormValue("password") - } else if r.Header.Get("Content-Type") == server.ContentJSON { + case server.ContentJSON: err := server.Decode(r, loginForm) if err != nil { @@ -54,7 +55,7 @@ func (ctrl *V1Controller) HandleAuthLogin() http.HandlerFunc { server.Respond(w, http.StatusBadRequest, server.Wrap(err)) return } - } else { + default: server.Respond(w, http.StatusBadRequest, errors.New("invalid content type")) return } @@ -67,7 +68,7 @@ func (ctrl *V1Controller) HandleAuthLogin() http.HandlerFunc { newToken, err := ctrl.svc.User.Login(r.Context(), loginForm.Username, loginForm.Password) if err != nil { - server.RespondError(w, http.StatusUnauthorized, err) + server.RespondError(w, http.StatusInternalServerError, err) return } diff --git a/backend/app/api/v1/v1_ctrl_user.go b/backend/app/api/v1/v1_ctrl_user.go index eba2219..4c6b71c 100644 --- a/backend/app/api/v1/v1_ctrl_user.go +++ b/backend/app/api/v1/v1_ctrl_user.go @@ -119,3 +119,37 @@ func (ctrl *V1Controller) HandleUserSelfDelete() http.HandlerFunc { server.Respond(w, http.StatusNoContent, nil) } } + +type ( + ChangePassword struct { + Current string `json:"current,omitempty"` + New string `json:"new,omitempty"` + } +) + +// HandleUserSelfChangePassword godoc +// @Summary Updates the users password +// @Tags User +// @Success 204 +// @Param payload body ChangePassword true "Password Payload" +// @Router /v1/users/change-password [PUT] +// @Security Bearer +func (ctrl *V1Controller) HandleUserSelfChangePassword() http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + var cp ChangePassword + err := server.Decode(r, &cp) + if err != nil { + log.Err(err).Msg("user failed to change password") + } + + ctx := services.NewContext(r.Context()) + + ok := ctrl.svc.User.ChangePassword(ctx, cp.Current, cp.New) + if !ok { + server.RespondError(w, http.StatusInternalServerError, err) + return + } + + server.Respond(w, http.StatusNoContent, nil) + } +} diff --git a/backend/internal/repo/repo_users.go b/backend/internal/repo/repo_users.go index f05dbb6..9763e38 100644 --- a/backend/internal/repo/repo_users.go +++ b/backend/internal/repo/repo_users.go @@ -121,3 +121,7 @@ func (e *UserRepository) GetSuperusers(ctx context.Context) ([]*ent.User, error) return users, nil } + +func (r *UserRepository) ChangePassword(ctx context.Context, UID uuid.UUID, pw string) error { + return r.db.User.UpdateOneID(UID).SetPassword(pw).Exec(ctx) +} diff --git a/backend/internal/services/service_user.go b/backend/internal/services/service_user.go index 688821a..e8bc00c 100644 --- a/backend/internal/services/service_user.go +++ b/backend/internal/services/service_user.go @@ -142,7 +142,13 @@ func (svc *UserService) createToken(ctx context.Context, userId uuid.UUID) (User func (svc *UserService) Login(ctx context.Context, username, password string) (UserAuthTokenDetail, error) { usr, err := svc.repos.Users.GetOneEmail(ctx, username) - if err != nil || !hasher.CheckPasswordHash(password, usr.PasswordHash) { + if err != nil { + // SECURITY: Perform hash to ensure response times are the same + hasher.CheckPasswordHash("not-a-real-password", "not-a-real-password") + return UserAuthTokenDetail{}, ErrorInvalidLogin + } + + if !hasher.CheckPasswordHash(password, usr.PasswordHash) { return UserAuthTokenDetail{}, ErrorInvalidLogin } @@ -190,3 +196,29 @@ func (svc *UserService) NewInvitation(ctx Context, uses int, expiresAt time.Time return token.Raw, nil } + +func (svc *UserService) ChangePassword(ctx Context, current string, new string) (ok bool) { + usr, err := svc.repos.Users.GetOneId(ctx, ctx.UID) + if err != nil { + return false + } + + if !hasher.CheckPasswordHash(current, usr.PasswordHash) { + log.Err(errors.New("current password is incorrect")).Msg("Failed to change password") + return false + } + + hashed, err := hasher.HashPassword(new) + if err != nil { + log.Err(err).Msg("Failed to hash password") + return false + } + + err = svc.repos.Users.ChangePassword(ctx.Context, ctx.UID, hashed) + if err != nil { + log.Err(err).Msg("Failed to change password") + return false + } + + return true +} diff --git a/frontend/lib/api/__test__/factories/index.ts b/frontend/lib/api/__test__/factories/index.ts new file mode 100644 index 0000000..51a0fb0 --- /dev/null +++ b/frontend/lib/api/__test__/factories/index.ts @@ -0,0 +1,80 @@ +import { faker } from "@faker-js/faker"; +import { expect } from "vitest"; +import { overrideParts } from "../../base/urls"; +import { PublicApi } from "../../public"; +import { LabelCreate, LocationCreate, UserRegistration } from "../../types/data-contracts"; +import * as config from "../../../../test/config"; +import { UserClient } from "../../user"; +import { Requests } from "../../../requests"; + +/** + * Returns a random user registration object that can be + * used to signup a new user. + */ +function user(): UserRegistration { + return { + email: faker.internet.email(), + password: faker.internet.password(), + name: faker.name.firstName(), + token: "", + }; +} + +function location(): LocationCreate { + return { + name: faker.address.city(), + description: faker.lorem.sentence(), + }; +} + +function label(): LabelCreate { + return { + name: faker.lorem.word(), + description: faker.lorem.sentence(), + color: faker.internet.color(), + }; +} + +function publicClient(): PublicApi { + overrideParts(config.BASE_URL, "/api/v1"); + const requests = new Requests(""); + return new PublicApi(requests); +} + +function userClient(token: string): UserClient { + overrideParts(config.BASE_URL, "/api/v1"); + const requests = new Requests("", token); + return new UserClient(requests); +} + +type TestUser = { + client: UserClient; + user: UserRegistration; +}; + +async function userSingleUse(): Promise { + const usr = user(); + + const pub = publicClient(); + await pub.register(usr); + const result = await pub.login(usr.email, usr.password); + + expect(result.error).toBeFalsy(); + expect(result.status).toBe(200); + + return { + client: new UserClient(new Requests("", result.data.token)), + user: usr, + }; +} + +export const factories = { + user, + location, + label, + client: { + public: publicClient, + user: userClient, + singleUse: userSingleUse, + }, +}; diff --git a/frontend/lib/api/__test__/public.test.ts b/frontend/lib/api/__test__/public.test.ts index 4b07165..044558e 100644 --- a/frontend/lib/api/__test__/public.test.ts +++ b/frontend/lib/api/__test__/public.test.ts @@ -1,20 +1,10 @@ import { describe, test, expect } from "vitest"; -import { faker } from "@faker-js/faker"; -import { UserRegistration } from "../types/data-contracts"; -import { client, sharedUserClient, userClient } from "./test-utils"; - -function userFactory(): UserRegistration { - return { - email: faker.internet.email(), - password: faker.internet.password(), - name: faker.name.firstName(), - token: "", - }; -} +import { factories } from "./factories"; +import { sharedUserClient } from "./test-utils"; describe("[GET] /api/v1/status", () => { test("server should respond", async () => { - const api = client(); + const api = factories.client.public(); const { response, data } = await api.status(); expect(response.status).toBe(200); expect(data.health).toBe(true); @@ -22,8 +12,8 @@ describe("[GET] /api/v1/status", () => { }); describe("first time user workflow (register, login, join group)", () => { - const api = client(); - const userData = userFactory(); + const api = factories.client.public(); + const userData = factories.user(); test("user should be able to register", async () => { const { response } = await api.register(userData); @@ -36,7 +26,7 @@ describe("first time user workflow (register, login, join group)", () => { expect(data.token).toBeTruthy(); // Cleanup - const userApi = userClient(data.token); + const userApi = factories.client.user(data.token); { const { response } = await userApi.user.delete(); expect(response.status).toBe(204); @@ -59,7 +49,7 @@ describe("first time user workflow (register, login, join group)", () => { // Create User 2 with token - const duplicateUser = userFactory(); + const duplicateUser = factories.user(); duplicateUser.token = data.token; const { response: registerResp } = await api.register(duplicateUser); @@ -70,7 +60,7 @@ describe("first time user workflow (register, login, join group)", () => { // Get Self and Assert - const client2 = userClient(loginData.token); + const client2 = factories.client.user(loginData.token); const { data: user2 } = await client2.user.self(); diff --git a/frontend/lib/api/__test__/test-utils.ts b/frontend/lib/api/__test__/test-utils.ts index 2cef4a3..41ef871 100644 --- a/frontend/lib/api/__test__/test-utils.ts +++ b/frontend/lib/api/__test__/test-utils.ts @@ -1,21 +1,6 @@ import { beforeAll, expect } from "vitest"; -import { Requests } from "../../requests"; -import { overrideParts } from "../base/urls"; -import { PublicApi } from "../public"; -import * as config from "../../../test/config"; import { UserClient } from "../user"; - -export function client() { - overrideParts(config.BASE_URL, "/api/v1"); - const requests = new Requests(""); - return new PublicApi(requests); -} - -export function userClient(token: string) { - overrideParts(config.BASE_URL, "/api/v1"); - const requests = new Requests("", token); - return new UserClient(requests); -} +import { factories } from "./factories"; const cache = { token: "", @@ -27,7 +12,7 @@ const cache = { */ export async function sharedUserClient(): Promise { if (cache.token) { - return userClient(cache.token); + return factories.client.user(cache.token); } const testUser = { email: "__test__@__test__.com", @@ -36,12 +21,12 @@ export async function sharedUserClient(): Promise { token: "", }; - const api = client(); + const api = factories.client.public(); const { response: tryLoginResp, data } = await api.login(testUser.email, testUser.password); if (tryLoginResp.status === 200) { cache.token = data.token; - return userClient(cache.token); + return factories.client.user(cache.token); } const { response: registerResp } = await api.register(testUser); @@ -51,7 +36,7 @@ export async function sharedUserClient(): Promise { expect(loginResp.status).toBe(200); cache.token = loginData.token; - return userClient(data.token); + return factories.client.user(data.token); } beforeAll(async () => { diff --git a/frontend/lib/api/__test__/user/labels.test.ts b/frontend/lib/api/__test__/user/labels.test.ts index 1fc414a..5564e6b 100644 --- a/frontend/lib/api/__test__/user/labels.test.ts +++ b/frontend/lib/api/__test__/user/labels.test.ts @@ -1,23 +1,17 @@ import { describe, expect, test } from "vitest"; import { LabelOut } from "../../types/data-contracts"; import { UserClient } from "../../user"; +import { factories } from "../factories"; import { sharedUserClient } from "../test-utils"; describe("locations lifecycle (create, update, delete)", () => { - let increment = 0; - /** * useLabel sets up a label resource for testing, and returns a function * that can be used to delete the label from the backend server. */ async function useLabel(api: UserClient): Promise<[LabelOut, () => Promise]> { - const { response, data } = await api.labels.create({ - name: `__test__.label.name_${increment}`, - description: `__test__.label.description_${increment}`, - color: "", - }); + const { response, data } = await api.labels.create(factories.label()); expect(response.status).toBe(201); - increment++; const cleanup = async () => { const { response } = await api.labels.delete(data.id); @@ -29,11 +23,7 @@ describe("locations lifecycle (create, update, delete)", () => { test("user should be able to create a label", async () => { const api = await sharedUserClient(); - const labelData = { - name: "test-label", - description: "test-description", - color: "", - }; + const labelData = factories.label(); const { response, data } = await api.labels.create(labelData); diff --git a/frontend/lib/api/__test__/user/locations.test.ts b/frontend/lib/api/__test__/user/locations.test.ts index 8583074..eb7765e 100644 --- a/frontend/lib/api/__test__/user/locations.test.ts +++ b/frontend/lib/api/__test__/user/locations.test.ts @@ -1,22 +1,17 @@ import { describe, expect, test } from "vitest"; import { LocationOut } from "../../types/data-contracts"; import { UserClient } from "../../user"; +import { factories } from "../factories"; import { sharedUserClient } from "../test-utils"; describe("locations lifecycle (create, update, delete)", () => { - let increment = 0; - /** * useLocatio sets up a location resource for testing, and returns a function * that can be used to delete the location from the backend server. */ async function useLocation(api: UserClient): Promise<[LocationOut, () => Promise]> { - const { response, data } = await api.locations.create({ - name: `__test__.location.name_${increment}`, - description: `__test__.location.description_${increment}`, - }); + const { response, data } = await api.locations.create(factories.location()); expect(response.status).toBe(201); - increment++; const cleanup = async () => { const { response } = await api.locations.delete(data.id); @@ -29,10 +24,7 @@ describe("locations lifecycle (create, update, delete)", () => { test("user should be able to create a location", async () => { const api = await sharedUserClient(); - const locationData = { - name: "test-location", - description: "test-description", - }; + const locationData = factories.location(); const { response, data } = await api.locations.create(locationData); diff --git a/frontend/lib/api/__test__/user/user.test.ts b/frontend/lib/api/__test__/user/user.test.ts new file mode 100644 index 0000000..ca4174d --- /dev/null +++ b/frontend/lib/api/__test__/user/user.test.ts @@ -0,0 +1,27 @@ +import { faker } from "@faker-js/faker"; +import { describe, expect, test } from "vitest"; +import { factories } from "../factories"; + +describe("basic user workflows", () => { + test("user should be able to change password", async () => { + const { client, user } = await factories.client.singleUse(); + const password = faker.internet.password(); + + // Change Password + { + const response = await client.user.changePassword(user.password, password); + expect(response.error).toBeFalsy(); + expect(response.status).toBe(204); + } + + // Ensure New Login is Valid + { + const pub = factories.client.public(); + const response = await pub.login(user.email, password); + expect(response.error).toBeFalsy(); + expect(response.status).toBe(200); + } + + await client.user.delete(); + }, 20000); +}); diff --git a/frontend/lib/api/base/base-api.ts b/frontend/lib/api/base/base-api.ts index 1c9614e..c8da034 100644 --- a/frontend/lib/api/base/base-api.ts +++ b/frontend/lib/api/base/base-api.ts @@ -43,7 +43,7 @@ export class BaseAPI { * are present. This is useful for when you want to send a subset of fields to * the server like when performing an update. */ - dropFields(obj: T, keys: Array = []): T { + protected dropFields(obj: T, keys: Array = []): T { const result = { ...obj }; [...keys, "createdAt", "updatedAt"].forEach(key => { // @ts-ignore - we are checking for the key above diff --git a/frontend/lib/api/classes/users.ts b/frontend/lib/api/classes/users.ts index 39f03b4..21006d0 100644 --- a/frontend/lib/api/classes/users.ts +++ b/frontend/lib/api/classes/users.ts @@ -1,5 +1,5 @@ import { BaseAPI, route } from "../base"; -import { UserOut } from "../types/data-contracts"; +import { ChangePassword, UserOut } from "../types/data-contracts"; import { Result } from "../types/non-generated"; export class UserApi extends BaseAPI { @@ -14,4 +14,14 @@ export class UserApi extends BaseAPI { public delete() { return this.http.delete({ url: route("/users/self") }); } + + public changePassword(current: string, newPassword: string) { + return this.http.put({ + url: route("/users/self/change-password"), + body: { + current, + new: newPassword, + }, + }); + } } diff --git a/frontend/lib/api/types/data-contracts.ts b/frontend/lib/api/types/data-contracts.ts index bd15e83..3111a85 100644 --- a/frontend/lib/api/types/data-contracts.ts +++ b/frontend/lib/api/types/data-contracts.ts @@ -238,6 +238,11 @@ export interface Build { version: string; } +export interface ChangePassword { + current: string; + new: string; +} + export interface GroupInvitation { expiresAt: Date; token: string; diff --git a/frontend/pages/profile.vue b/frontend/pages/profile.vue index 165b489..a564844 100644 --- a/frontend/pages/profile.vue +++ b/frontend/pages/profile.vue @@ -192,78 +192,132 @@ token.value = data.token; } } + + const passwordChange = reactive({ + loading: false, + dialog: false, + current: "", + new: "", + isValid: false, + }); + + function openPassChange() { + passwordChange.dialog = true; + } + + async function changePassword() { + passwordChange.loading = true; + if (!passwordChange.isValid) { + return; + } + + const { error } = await api.user.changePassword(passwordChange.current, passwordChange.new); + + if (error) { + notify.error("Failed to change password."); + passwordChange.loading = false; + return; + } + + notify.success("Password changed successfully."); + passwordChange.dialog = false; + passwordChange.new = ""; + passwordChange.current = ""; + passwordChange.loading = false; + }