Feature/password by role by tnramalho · Pull Request #347 · btwld/nestjs-modules

tnramalho · 2025-01-30T19:17:20Z

Defining password strength based on role

* main: feat: move event manager options to contructor feat: fix tsdoc type feat: implement event manager on dependent modules feat: add event manager

packages/nestjs-common/src/domain/role/interfaces/role-ownable.interface.ts

packages/nestjs-common/src/domain/user/interfaces/user-roles.interface.ts

packages/nestjs-password/src/interfaces/password-strength-service.interface.ts

packages/nestjs-user/src/config/user-default.config.ts

packages/nestjs-user/src/dto/user-roles.dto.ts

packages/nestjs-user/src/services/user-mutate.service.ts

packages/nestjs-user/src/services/user-password.service.ts

packages/nestjs-user/src/user.module-definition.ts

packages/nestjs-user/src/user.types.ts

packages/nestjs-user/src/user.utils.ts

MrMaz · 2025-02-06T14:11:33Z

Just a couple of high level things before deeper review of the latest commits.

You have UserRoleService hard coded on the providers.
The normalizeRoleNames is something the implementations need to do. The service should return the entire role object by default.

MrMaz · 2025-02-08T21:55:32Z

packages/nestjs-common/src/domain/role/interfaces/role-ownable.interface.ts

+import { RoleInterface } from './role.interface';
+
+export interface RoleOwnableInterface {
+  roleId?: ReferenceId;


roleId property should be required

Suggested change

roleId?: ReferenceId;

roleId: ReferenceId;

MrMaz · 2025-02-11T13:08:51Z

...estjs-common/src/domain/password/interfaces/password-strength-transform-options.interface.ts

+import { RoleOwnableInterface } from '../../role/interfaces/role-ownable.interface';
+
+export interface PasswordStrengthTransformOptionsInterface {
+  roles: RoleOwnableInterface[];


roles propert should be optional

Suggested change

roles: RoleOwnableInterface[];

roles?: RoleOwnableInterface[];

MrMaz · 2025-02-11T13:11:31Z

packages/nestjs-password/src/interfaces/password-create-object-options.interface.ts

+   * Optional password strength requirement. If provided, will validate
+   * that password meets minimum strength requirements.
+   */
+  passwordStrength?: PasswordStrengthEnum | null;


undefined is better

Suggested change

passwordStrength?: PasswordStrengthEnum | null;

passwordStrength?: PasswordStrengthEnum | undefined;

MrMaz · 2025-02-11T13:12:00Z

packages/nestjs-password/src/interfaces/password-strength-options.interface.ts

+ * Password Strength Options Interface
+ */
+export interface PasswordStrengthOptionsInterface {
+  passwordStrength?: PasswordStrengthEnum | null;


undefined is better

Suggested change

passwordStrength?: PasswordStrengthEnum | null;

passwordStrength?: PasswordStrengthEnum | undefined;

MrMaz · 2025-02-11T13:13:25Z

packages/nestjs-password/src/services/password-creation.service.ts

+        !this.passwordStrengthService.isStrong(password, {
+          passwordStrength: options?.passwordStrength,
+        })


should we have a try/catch here?

MrMaz · 2025-02-11T13:14:42Z

packages/nestjs-password/src/services/password-strength.service.ts

+  ): boolean {
+    const { passwordStrength } = options || {};
+
+    // TODO: Should we allow overriding the minimum password strength even if the provided strength is lower than the configured minimum?


did you align with Gabriel on this point?

Yes, what ever user define with the roles should overwrite

MrMaz · 2025-02-11T13:15:56Z

packages/nestjs-password/src/services/password-strength.service.ts

+      passwordStrength ||
+      this.settings?.minPasswordStrength ||
+      PasswordStrengthEnum.None;


coalesce is safer i think in case strength is explicitly zero? recommend test coverage to be sure.

Suggested change

passwordStrength ||

this.settings?.minPasswordStrength ||

PasswordStrengthEnum.None;

passwordStrength ??

this.settings?.minPasswordStrength ??

PasswordStrengthEnum.None;

MrMaz · 2025-02-11T13:20:29Z

packages/nestjs-user/src/interfaces/user-role-service.interface.ts

+   */
+  getUserRoles(
+    userDto: UserRolesInterface,
+    userToUpdateId?: ReferenceId,


i think this is redundant, it would always be the id in the UserDto

Not always, on create user for example we wont have a userId inside dto, however now that i review maybe we can remove the dto from this function, and keep function only to get roles based on user id, like

then i can verify before i call this function if i actually need it
this.userRoleService.resolvePasswordStrength

MrMaz · 2025-02-11T13:21:12Z

packages/nestjs-user/src/interfaces/user-role-service.interface.ts

+   * @returns Array of role names, or empty array if no roles found
+   */
+  getUserRoles(
+    userDto: UserRolesInterface,


should require the user id on the object?

Suggested change

userDto: UserRolesInterface,

userDto: ReferenceIdInterface & UserRolesInterface,

maybe we dont even need that object anymore, it was only being used to get the userRoles of it

MrMaz · 2025-02-11T13:22:10Z

packages/nestjs-user/src/interfaces/user-role-service.interface.ts

+   */
+  resolvePasswordStrength(
+    roles?: RoleOwnableInterface[],
+  ): PasswordStrengthEnum | null | undefined;


i think null doesn't make sense

Suggested change

): PasswordStrengthEnum | null | undefined;

): PasswordStrengthEnum | undefined;

MrMaz · 2025-02-11T13:25:49Z

packages/nestjs-user/src/services/user-mutate.service.ts


  protected async transform<T extends DeepPartial<UserEntityInterface>>(
-    user: T | (T & PasswordPlainInterface),
+    user: T | (T & PasswordPlainInterface & UserRolesInterface),


adding roles interface like this is slightly weird because the only property is optional, we need to review this.

we need this to pass to getPasswordStrength to be able to get strength based on roles

MrMaz · 2025-02-11T13:26:58Z

packages/nestjs-user/src/services/user-password.service.ts

    @Optional()
+    @Inject(UserRoleService)
+    private userRoleService?: UserRoleServiceInterface,


should be last parameter for better back compat

MrMaz · 2025-02-11T13:28:12Z

packages/nestjs-user/src/services/user-password.service.ts

+      this.userRoleService.getUserRoles &&
+      this.userRoleService.resolvePasswordStrength


if you need to check that these methods exist, is a weird smell

true, i had injected wrong service, thats why lol,

MrMaz · 2025-02-11T13:29:17Z

packages/nestjs-user/src/services/user-password.service.ts

+      const roles = await this.userRoleService.getUserRoles(
+        userDto,
+        userToUpdateId,
+      );
+      passwordStrength = await this.userRoleService.resolvePasswordStrength(
+        roles,
+      );


we should only be making one call to the service, resolving the roles here is not necessary

lets discuss this one

MrMaz · 2025-02-11T14:03:25Z

packages/nestjs-user/src/services/user-role.service.ts

+      const user: (ReferenceIdInterface<string> & UserRolesInterface) | null =
+        await this.userLookupService.byId(userToUpdateId);


needs a try/catch?

MrMaz · 2025-02-11T14:03:37Z

packages/nestjs-user/src/services/user-role.service.ts

+    if (userToUpdateId) {
+      const user: (ReferenceIdInterface<string> & UserRolesInterface) | null =
+        await this.userLookupService.byId(userToUpdateId);
+      if (user && user.userRoles) return user.userRoles;


please use brackets

MrMaz · 2025-02-11T14:03:53Z

packages/nestjs-user/src/services/user-role.service.ts

+    }
+
+    // get roles from payload
+    if (userDto.userRoles && userDto.userRoles.length > 0)


please use brackets

MrMaz · 2025-02-11T14:05:03Z

packages/nestjs-user/src/services/user-role.service.ts

+    return (
+      roles &&
+      this.userSettings.passwordStrength?.passwordStrengthTransform &&
+      this.userSettings.passwordStrength?.passwordStrengthTransform({ roles })


must have a try/catch on this

MrMaz · 2025-02-11T14:06:46Z

packages/nestjs-user/src/user.types.ts

 }
+
+export type PasswordStrengthTransform = (
+  options: PasswordStrengthTransformOptionsInterface,


options parameter should be optional

Suggested change

options: PasswordStrengthTransformOptionsInterface,

options?: PasswordStrengthTransformOptionsInterface,

MrMaz · 2025-02-11T14:07:12Z

packages/nestjs-user/src/user.types.ts

+
+export type PasswordStrengthTransform = (
+  options: PasswordStrengthTransformOptionsInterface,
+) => PasswordStrengthEnum | null;


undefined is better than null

Suggested change

) => PasswordStrengthEnum | null;

) => PasswordStrengthEnum | undefined;

MrMaz · 2025-02-11T14:08:25Z

packages/nestjs-user/src/user.utils.ts

+
+export const defaultPasswordStrengthTransform: PasswordStrengthTransform = (
+  options: PasswordStrengthTransformOptionsInterface,
+): PasswordStrengthEnum | null => {


undefined is better than null

Suggested change

): PasswordStrengthEnum | null => {

): PasswordStrengthEnum | undefined => {

MrMaz · 2025-02-11T14:08:52Z

packages/nestjs-user/src/user.utils.ts

+import { PasswordStrengthTransformOptionsInterface } from '@concepta/nestjs-common';
+
+export const defaultPasswordStrengthTransform: PasswordStrengthTransform = (
+  options: PasswordStrengthTransformOptionsInterface,


options parameter should be optional

Suggested change

options: PasswordStrengthTransformOptionsInterface,

options?: PasswordStrengthTransformOptionsInterface,

MrMaz · 2025-02-11T14:09:48Z

packages/nestjs-user/src/user.utils.ts

+): PasswordStrengthEnum | null => {
+  const { roles } = options;
+
+  if (roles.some((role) => role.role?.name === 'admin')) {


role param in lambda needs to be typed

MrMaz · 2025-02-11T14:10:00Z

packages/nestjs-user/src/user.utils.ts

+  if (roles.some((role) => role.role?.name === 'admin')) {
+    return PasswordStrengthEnum.VeryStrong;
+  }
+  if (roles.some((role) => role.role?.name === 'user')) {


role param in lambda needs to be typed

tnramalho added 5 commits January 29, 2025 16:18

chore: add password strength by role

6f98c28

Merge branch 'main' into feature/password-by-role

4036938

* main: feat: move event manager options to contructor feat: fix tsdoc type feat: implement event manager on dependent modules feat: add event manager

chore: add tests for password strength

2971a81

chore: update dependencies

e64152c

chore: add user roles to dto

aa1ed9d

MrMaz requested changes Feb 4, 2025

View reviewed changes

tnramalho added 2 commits February 5, 2025 16:49

chore: make user roles service and update params

1fba77e

chore: add comments

4563d41

tnramalho added 2 commits February 6, 2025 11:43

Merge branch 'main' into feature/password-by-role

2e6fd9b

chore: update user role service

cddb9e9

MrMaz reviewed Feb 8, 2025

View reviewed changes

MrMaz reviewed Feb 11, 2025

View reviewed changes

tnramalho added 3 commits February 11, 2025 18:41

chore: mr changes

b671c37

Merge branch 'main' into feature/password-by-role

93f3a13

Merge branch 'main' into feature/password-by-role

a448275

	roles: RoleOwnableInterface[];
	roles?: RoleOwnableInterface[];

	passwordStrength?: PasswordStrengthEnum \| null;
	passwordStrength?: PasswordStrengthEnum \| undefined;

	userDto: UserRolesInterface,
	userDto: ReferenceIdInterface & UserRolesInterface,

	): PasswordStrengthEnum \| null \| undefined;
	): PasswordStrengthEnum \| undefined;

		this.userRoleService.getUserRoles &&
		this.userRoleService.resolvePasswordStrength

		const user: (ReferenceIdInterface<string> & UserRolesInterface) \| null =
		await this.userLookupService.byId(userToUpdateId);

	options: PasswordStrengthTransformOptionsInterface,
	options?: PasswordStrengthTransformOptionsInterface,

	) => PasswordStrengthEnum \| null;
	) => PasswordStrengthEnum \| undefined;

	): PasswordStrengthEnum \| null => {
	): PasswordStrengthEnum \| undefined => {

Conversation

tnramalho commented Jan 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrMaz commented Feb 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnramalho Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

tnramalho Feb 11, 2025 •

edited

Loading