ronantakizawa/codereview-bench huggingface dataset:
Repository: logto-io/logto
File: packages/core/src/utils/user.ts (TypeScript)

Code before:
 *
 * @param user - Internal user data model
 * @param extraInfo - Additional user-related information, such as SSO identities
 * @returns Formatted user profile response object
 */
export const transpileUserProfileResponse = (
  user: User,
  extraInfo: ExtraUserInfo = {}
): UserProfileResponse => {
  const { ssoIdentities } = extraInfo;

  return {
    ...pick(user, ...userInfoSelectFields),
    hasPassword: Boolean(user.passwordEncrypted),
    ...(ssoIdentities && { ssoIdentities }),
  };
};

// Not used yet, may be used in the future, keep as a individual method.
const getValidPhoneNumber = (phone: string): string => {
  if (!phone) {
    return phone;
  }

  try {
    const phoneNumber = parseE164PhoneNumberWithError(phone);

    if (!phoneNumber.isValid()) {
      throw new RequestError({ code: 'user.invalid_phone', status: 422 });
    }

    const { number } = phoneNumber;
    if (typeof number !== 'string') {
      throw new RequestError({ code: 'user.invalid_phone', status: 422 });
    }
    return number.slice(1);
  } catch (error) {
    if (error instanceof ParseError) {
      throw new RequestError({ code: 'user.invalid_phone', status: 422 }, error);
    }
    throw error;
  }
};

export const validatePhoneNumber = (phone: string): void => {
  getValidPhoneNumber(phone);
};


Review comment (suggestion):
Better use the parsePhoneNumber function from @logto/shared as the SSOT, see my updates [here](https://github.com/logto-io/logto/pull/7374). So you may let the function throws an error if the number is invalid.

Code after:
 *
 * @param user - Internal user data model
 * @param extraInfo - Additional user-related information, such as SSO identities
 * @returns Formatted user profile response object
 */
export const transpileUserProfileResponse = (
  user: User,
  extraInfo: ExtraUserInfo = {}
): UserProfileResponse => {
  const { ssoIdentities } = extraInfo;

  return {
    ...pick(user, ...userInfoSelectFields),
    hasPassword: Boolean(user.passwordEncrypted),
    ...(ssoIdentities && { ssoIdentities }),
  };
};

// Not used yet, may be used in the future, keep as a individual method.
const getValidPhoneNumber = (phone: string): string => {
  if (!phone) {
    return phone;
  }

  try {
    return PhoneNumberParser.parse(phone).number;
  } catch (error) {
    throw new RequestError({ code: 'user.invalid_phone', status: 422 }, error);
  }
};

export const validatePhoneNumber = (phone: string): void => {
  getValidPhoneNumber(phone);
};
Repository: logto-io/logto
File: packages/core/src/utils/user.ts (TypeScript)

Code before:
 * @returns Formatted user profile response object
 */
export const transpileUserProfileResponse = (
  user: User,
  extraInfo: ExtraUserInfo = {}
): UserProfileResponse => {
  const { ssoIdentities } = extraInfo;

  return {
    ...pick(user, ...userInfoSelectFields),
    hasPassword: Boolean(user.passwordEncrypted),
    ...(ssoIdentities && { ssoIdentities }),
  };
};

// Not used yet, may be used in the future, keep as a individual method.
const getValidPhoneNumber = (phone: string): string => {
  if (!phone) {
    return phone;
  }

  try {
    const phoneNumber = PhoneNumberParser.parse(phone);

    if (!phoneNumber.isValid()) {
      throw new RequestError({ code: 'user.invalid_phone', status: 422 });
    }

    const { number } = phoneNumber;
    if (typeof number !== 'string') {
      throw new RequestError({ code: 'user.invalid_phone', status: 422 });
    }
    return number.slice(1);
  } catch (error) {
    if (error instanceof ParseError) {
      throw new RequestError({ code: 'user.invalid_phone', status: 422 }, error);
    }
    throw error;
  }
};

export const validatePhoneNumber = (phone: string): void => {
  getValidPhoneNumber(phone);
};


Review comment (suggestion):
How about simply

```ts
import { parsePhoneNumber } from '@logto/shared/universal';

const getValidPhoneNumber = (phone: string) => {
 try {
   return parsePhoneNumber(phone, true);
 } catch (error: unknown) {
   throw new RequestError({ code: 'user.invalid_phone', status: 422 }, error);
 }
}
```

Code after:
 */
export const transpileUserProfileResponse = (
  user: User,
  extraInfo: ExtraUserInfo = {}
): UserProfileResponse => {
  const { ssoIdentities } = extraInfo;

  return {
    ...pick(user, ...userInfoSelectFields),
    hasPassword: Boolean(user.passwordEncrypted),
    ...(ssoIdentities && { ssoIdentities }),
  };
};

// Not used yet, may be used in the future, keep as a individual method.
const getValidPhoneNumber = (phone: string): string => {
  if (!phone) {
    return phone;
  }

  try {
    return PhoneNumberParser.parse(phone).number;
  } catch (error) {
    throw new RequestError({ code: 'user.invalid_phone', status: 422 }, error);
  }
};

export const validatePhoneNumber = (phone: string): void => {
  getValidPhoneNumber(phone);
};
Repository: logto-io/logto
File: packages/core/src/libraries/sign-in-experience/email-blocklist-policy.ts (TypeScript)

Code before:
  for (const item of list) {
    if (!emailOrEmailDomainRegex.test(item)) {
      invalidItems.add(item);
    }
  }

  return invalidItems;
};

export const validateEmailBlocklistPolicy = (emailBlocklistPolicy: EmailBlocklistPolicy) => {
  const { customBlocklist = [] } = emailBlocklistPolicy;
  const invalidItems = validateCustomBlockList(customBlocklist);

  if (invalidItems.size > 0) {
    throw new RequestError({
      code: 'sign_in_experiences.invalid_custom_email_blocklist_format',
      items: Array.from(invalidItems),
      status: 400,
    });
  }

  const deduplicateList = deduplicate(customBlocklist);

  if (deduplicateList.length !== customBlocklist.length) {
    throw new RequestError('sign_in_experiences.duplicate_custom_email_blocklist_items');
  }
};


Review comment (refactor):
Is this error necessary? You actually have the items deduplicated, then why not just proceed with the result?

Code after:
  const invalidItems = new Set();

  for (const item of list) {
    if (!emailOrEmailDomainRegex.test(item)) {
      invalidItems.add(item);
    }
  }

  return invalidItems;
};

const parseCustomBlocklist = (customBlocklist: string[]) => {
  const deduplicated = deduplicate(customBlocklist);
  const invalidItems = validateCustomBlockListFormat(deduplicated);

  if (invalidItems.size > 0) {
    throw new RequestError({
      code: 'sign_in_experiences.invalid_custom_email_blocklist_format',
      items: Array.from(invalidItems),
      status: 400,
    });
  }

  return deduplicated;
};

/**
 * This function will deduplicate the custom blocklist (if not undefined) and validate the format of each item.
 * If any item is invalid, it throws a RequestError with the details of the invalid items.
 */
export const parseEmailBlocklistPolicy = (
  emailBlocklistPolicy: EmailBlocklistPolicy
): EmailBlocklistPolicy => {
  // TODO: @simeng remove this validation when the feature is ready
  assertThat(
    EnvSet.values.isDevFeaturesEnabled,
    new RequestError('request.invalid_input', {
      details: 'Email block list policy is not supported in this environment',
    })
  );

  const { customBlocklist, ...rest } = emailBlocklistPolicy;

  return {
    ...rest,
    ...conditional(customBlocklist && { customBlocklist: parseCustomBlocklist(customBlocklist) }),
  };
};
Repository: logto-io/logto
File: packages/core/src/routes/account/mfa-verifications.ts (TypeScript)

Code before:
  router.delete(
    `${accountApiPrefix}/mfa-verifications/:verificationId`,
    koaGuard({
      params: z.object({
        verificationId: z.string(),
      }),
      status: [204, 400, 401],
    }),
    async (ctx, next) => {
      const { id: userId, scopes, identityVerified } = ctx.auth;
      assertThat(
        identityVerified,
        new RequestError({ code: 'verification_record.permission_denied', status: 401 })
      );
      const { fields } = ctx.accountCenter;
      assertThat(
        fields.mfa === AccountCenterControlValue.Edit,
        'account_center.filed_not_editable'
      );
      assertThat(scopes.has(UserScope.Identities), 'auth.unauthorized');

      const user = await findUserById(userId);
      const mfaVerification = user.mfaVerifications.find(
        (mfaVerification) =>
          mfaVerification.id === ctx.guard.params.verificationId &&
          mfaVerification.type === MfaFactor.WebAuthn
      );
      assertThat(mfaVerification, 'verification_record.not_found');

      const updatedUser = await updateUserById(userId, {
        mfaVerifications: user.mfaVerifications.filter(
          (mfaVerification) => mfaVerification.id !== ctx.guard.params.verificationId
        ),
      });

      ctx.appendDataHookContext('User.Data.Updated', { user: updatedUser });

      ctx.status = 204;

      return next();
    }
  );
}


Review comment (security):
According to your API naming convention, should it not limit the verification type to WebAuthn?  Can any type of mfa-verificaiton should can be deleted by a given ID?

Code after:
  router.delete(
    `${accountApiPrefix}/mfa-verifications/:verificationId`,
    koaGuard({
      params: z.object({
        verificationId: z.string(),
      }),
      status: [204, 400, 401],
    }),
    async (ctx, next) => {
      const { id: userId, scopes, identityVerified } = ctx.auth;
      assertThat(
        identityVerified,
        new RequestError({ code: 'verification_record.permission_denied', status: 401 })
      );
      const { fields } = ctx.accountCenter;
      assertThat(
        fields.mfa === AccountCenterControlValue.Edit,
        'account_center.field_not_editable'
      );
      assertThat(scopes.has(UserScope.Identities), 'auth.unauthorized');

      const user = await findUserById(userId);
      const mfaVerification = user.mfaVerifications.find(
        (mfaVerification) => mfaVerification.id === ctx.guard.params.verificationId
      );
      assertThat(mfaVerification, 'verification_record.not_found');

      const updatedUser = await updateUserById(userId, {
        mfaVerifications: user.mfaVerifications.filter(
          (mfaVerification) => mfaVerification.id !== ctx.guard.params.verificationId
        ),
      });

      ctx.appendDataHookContext('User.Data.Updated', { user: updatedUser });

      ctx.status = 204;

      return next();
    }
  );
}
Repository: logto-io/logto
File: packages/schemas/tables/custom_profile_fields.sql (SQL)

Code before:
create type custom_profile_field_type
  as enum ('Text', 'Number', 'Date', 'Checkbox', 'Select', 'Url', 'Regex', 'Address', 'Fullname');

create table custom_profile_fields (
  tenant_id varchar(21) not null
    references tenants (id) on update cascade on delete cascade,
  id varchar(21) not null,
  name varchar(128) not null,
  type custom_profile_field_type not null,
  label varchar(128) not null default '',
  description varchar(256),
  required boolean not null default false,
  placeholder varchar(256),
  min_length int4,
  max_length int4,
  min_value int4,
  max_value int4,
  format varchar(128),
  options jsonb /* @use FieldOptions */,
  parts jsonb /* @use FieldParts */,
  created_at timestamptz not null default(now()),
  sie_order int2 not null default 0,
  primary key (id),
  constraint custom_profile_fields__name
    unique (tenant_id, name),
  constraint custom_profile_fields__sie_order

Review comment (suggestion):
Try to avoid directly defining DB enum types. Using varchar fields along with TypeScript enum declarations is more flexible and easier to extend — for example, the hooksEvent column in the hooks table.

Code after:
create table custom_profile_fields (
  tenant_id varchar(21) not null
    references tenants (id) on update cascade on delete cascade,
  id varchar(21) not null,
  name varchar(128) not null,
  type varchar(128) not null /* @use CustomProfileFieldType */,
  label varchar(128) not null default '',
  description varchar(256),
  required boolean not null default false,
  config jsonb /* @use CustomProfileFieldConfig */ not null default '{}'::jsonb,
  created_at timestamptz not null default(now()),
  sie_order int2 not null default 0,
  primary key (id),
  constraint custom_profile_fields__name
    unique (tenant_id, name),
  constraint custom_profile_fields__sie_order
    unique (tenant_id, sie_order)
);

create or replace function custom_profile_fields__increment_sie_order() returns trigger as
$$ begin
  new.sie_order = (
    select coalesce(max(sie_order), 0)
    from custom_profile_fields
    where tenant_id = (
      select id from tenants where db_user = current_user
Repository: logto-io/logto
File: packages/core/src/routes/account-center/index.ts (TypeScript)

Code before:
import { AccountCenters, accountCenterFieldControlGuard } from '@logto/schemas';
import { z } from 'zod';

import koaGuard from '#src/middleware/koa-guard.js';

import type { ManagementApiRouter, RouterInitArgs } from '../types.js';

export default function accountCentersRoutes<T extends ManagementApiRouter>(
  ...args: RouterInitArgs<T>
) {
  const [router, { queries }] = args;
  const { findDefaultAccountCenter, updateDefaultAccountCenter } = queries.accountCenters;
  router.get(
    '/account-center',
    koaGuard({
      response: AccountCenters.guard,
      status: [200],
    }),
    async (ctx, next) => {
      ctx.body = await findDefaultAccountCenter();

      return next();
    }
  );

  router.patch(
    '/account-center',
    koaGuard({
      body: z.object({
        enabled: z.boolean().optional(),
        fields: accountCenterFieldControlGuard.optional(),
      }),
      response: AccountCenters.guard,
      status: [200],
    }),

    async (ctx, next) => {
      const { enabled, fields } = ctx.guard.body;
      // Make sure the account center exists
      await findDefaultAccountCenter();
      const updatedAccountCenter = await updateDefaultAccountCenter({
        enabled,
        fields,
      });

      ctx.body = updatedAccountCenter;

      return next();
    }
  );
}


Review comment (security):
If webauthnRelatedOrigins is undefined here, will it remove all existing webauthnRelatedOrigins stored in the config? Is that the expected behavior?

Code after:
    }
  );

  router.patch(
    '/account-center',
    koaGuard({
      body: z.object({
        enabled: z.boolean().optional(),
        fields: accountCenterFieldControlGuard.optional(),
        webauthnRelatedOrigins: webauthnRelatedOriginsGuard.optional(),
      }),
      response: AccountCenters.guard,
      status: [200],
    }),

    async (ctx, next) => {
      const { enabled, fields, webauthnRelatedOrigins } = ctx.guard.body;

      // Make sure the account center exists
      await findDefaultAccountCenter();
      const updatedAccountCenter = await updateDefaultAccountCenter({
        enabled,
        fields,
        webauthnRelatedOrigins: webauthnRelatedOrigins
          ? deduplicate(webauthnRelatedOrigins)
          : undefined,
      });

      ctx.body = updatedAccountCenter;

      return next();
    }
  );
}
Repository: logto-io/logto
File: packages/console/src/pages/OneTimeTokenLanding/index.tsx (TypeScript)

Code before:

    const isNewUser = yes(searchParams.get(OneTimeTokenLandingSearchParams.IsNewUser));

    if (!oneTimeToken || !email) {
      navigate('/', { replace: true });
      return;
    }

    if (isLoading || isAuthenticated) {
      return;
    }

    const handleSignIn = async () => {
      try {
        await signIn({
          redirectUri,
          clearTokens: false,
          firstScreen: isNewUser ? FirstScreen.Register : FirstScreen.SignIn,
          extraParams: {
            one_time_token: oneTimeToken,
            login_hint: email,
          },
        });
      } catch {
        navigate('/', { replace: true });
      }
    };

    void handleSignIn();
  }, [searchParams, signIn, isLoading, isAuthenticated, navigate, redirectUri]);

  return <AppLoading />;
}

export default OneTimeTokenLanding;


Review comment (suggestion):
This `try-catch` is also unnecessary, if an error occurs, it is expected to be handled in the outer `ErrorBoundary` wrapper

Code after:
import { useLogto } from '@logto/react';
import { ExtraParamsKey } from '@logto/schemas';
import { useContext, useEffect } from 'react';
import { useNavigate, useSearchParams } from 'react-router-dom';

import AppLoading from '@/components/AppLoading';
import { TenantsContext } from '@/contexts/TenantsProvider';
import useRedirectUri from '@/hooks/use-redirect-uri';

enum OneTimeTokenLandingSearchParams {
  OneTimeToken = 'one_time_token',
  Email = 'email',
}

/** The one-time token landing page for sign-in with one-time tokens. */
function OneTimeTokenLanding() {
  const navigate = useNavigate();
  const { isAuthenticated, signIn } = useLogto();
  const [searchParams] = useSearchParams();
  const { navigateTenant } = useContext(TenantsContext);
  const redirectUri = useRedirectUri();

  const oneTimeToken = searchParams.get(OneTimeTokenLandingSearchParams.OneTimeToken);
  const email = searchParams.get(OneTimeTokenLandingSearchParams.Email);

  useEffect(() => {
    if (isAuthenticated || !oneTimeToken || !email) {
      // Navigate to root, which will handle tenant selection
      navigate('/', { replace: true });
      return;
    }

    void signIn({
      redirectUri,
      /**
       * Can not clear tokens here since user may already have tokens and let user select which account to keep.
       * We can hence clear tokens in the <Callback /> page.
       */
      clearTokens: false,
      extraParams: {
        [ExtraParamsKey.OneTimeToken]: oneTimeToken,
        [ExtraParamsKey.LoginHint]: email,
      },
    });
  }, [isAuthenticated, navigate, navigateTenant, signIn, redirectUri, oneTimeToken, email]);

  return <AppLoading />;
}

export default OneTimeTokenLanding;
Repository: logto-io/logto
File: packages/console/src/pages/OneTimeTokenLanding/index.tsx (TypeScript)

Code before:

/** The one-time token landing page for sign-in with one-time tokens. */
function OneTimeTokenLanding() {
  const navigate = useNavigate();
  const { isAuthenticated, signIn } = useLogto();
  const [searchParams] = useSearchParams();
  const { navigateTenant, currentTenantId } = useContext(TenantsContext);
  const redirectUri = useRedirectUri();

  useEffect(() => {
    if (isAuthenticated) {
      // If we have a current tenant, navigate to it
      if (currentTenantId) {
        navigateTenant(currentTenantId);
      } else {
        // Otherwise navigate to root, which will handle tenant selection
        navigate('/', { replace: true });
      }
      return;
    }

    const oneTimeToken = searchParams.get(OneTimeTokenLandingSearchParams.OneTimeToken);
    const email = searchParams.get(OneTimeTokenLandingSearchParams.Email);

    if (!oneTimeToken || !email) {
      navigate('/', { replace: true });
      return;
    }

    void signIn({
      redirectUri,
      clearTokens: false,
      extraParams: {
        [ExtraParamsKey.OneTimeToken]: oneTimeToken,
        [ExtraParamsKey.LoginHint]: email,
      },
    });
  }, [isAuthenticated, navigate, currentTenantId, navigateTenant, searchParams, signIn, redirectUri]);

  return <AppLoading />;
}

export default OneTimeTokenLanding;


Review comment (suggestion):
You need to clear the tokens in the `Callback` page.

![image](https://github.com/user-attachments/assets/dfd60bae-51ec-454a-952f-c89de6df2af9)


Code after:

/** The one-time token landing page for sign-in with one-time tokens. */
function OneTimeTokenLanding() {
  const navigate = useNavigate();
  const { isAuthenticated, signIn } = useLogto();
  const [searchParams] = useSearchParams();
  const { navigateTenant } = useContext(TenantsContext);
  const redirectUri = useRedirectUri();

  const oneTimeToken = searchParams.get(OneTimeTokenLandingSearchParams.OneTimeToken);
  const email = searchParams.get(OneTimeTokenLandingSearchParams.Email);

  useEffect(() => {
    if (isAuthenticated || !oneTimeToken || !email) {
      // Navigate to root, which will handle tenant selection
      navigate('/', { replace: true });
      return;
    }

    void signIn({
      redirectUri,
      /**
       * Can not clear tokens here since user may already have tokens and let user select which account to keep.
       * We can hence clear tokens in the <Callback /> page.
       */
      clearTokens: false,
      extraParams: {
        [ExtraParamsKey.OneTimeToken]: oneTimeToken,
        [ExtraParamsKey.LoginHint]: email,
      },
    });
  }, [isAuthenticated, navigate, navigateTenant, signIn, redirectUri, oneTimeToken, email]);

  return <AppLoading />;
}

export default OneTimeTokenLanding;
Repository: logto-io/logto
File: packages/core/src/routes/experience/classes/profile.ts (TypeScript)

Code before:
    const user = await this.safeGetIdentifiedUser();

    if (this.#data.enterpriseSsoIdentity) {
      return;
    }

    const mandatoryProfileFields =
      await this.signInExperienceValidator.getMandatoryUserProfileBySignUpMethods();

    const missingMandatoryProfile = this.profileValidator.getMissingUserProfile(
      this.#data,
      mandatoryProfileFields,
      user
    );

    assertThat(
      missingMandatoryProfile.size === 0,
      new RequestError(
        { code: 'user.missing_profile', status: 422 },
        { missingProfile: [...missingMandatoryProfile] }
      )
    );

    assertThat(
      this.interactionContext.getInteractionEvent() === InteractionEvent.Register &&
        !(await this.profileValidator.hasMissingExtraProfileFields(this.#data, user)),
      new RequestError(
        {
          code: 'user.missing_profile',
          status: 422,
        },
        { missingProfile: [MissingProfile.extra] }
      )
    );
  }

  /**
   * Set profile without validation.
   * - skip profile uniqueness check.
   * - skip profile existence check in the current user account.
   */
  unsafeSet(profile: InteractionProfile) {
    this.#data = {
      ...this.#data,
      ...profile,
    };
  }

  /**
   * Prepend the profile data to the existing profile data.
   * Avoid overwriting the existing profile data.

Review comment (suggestion):
```suggestion
    assertThat(
      this.interactionContext.getInteractionEvent() !== InteractionEvent.Register ||
        !(await this.profileValidator.hasMissingExtraProfileFields(this.#data, user)),
```

Code after:
    const user = await this.safeGetIdentifiedUser();

    if (this.#data.enterpriseSsoIdentity) {
      return;
    }

    const mandatoryProfileFields =
      await this.signInExperienceValidator.getMandatoryUserProfileBySignUpMethods();

    const missingMandatoryProfile = this.profileValidator.getMissingUserProfile(
      this.#data,
      mandatoryProfileFields,
      user
    );

    assertThat(
      missingMandatoryProfile.size === 0,
      new RequestError(
        { code: 'user.missing_profile', status: 422 },
        { missingProfile: [...missingMandatoryProfile] }
      )
    );

    assertThat(
      this.interactionContext.getInteractionEvent() !== InteractionEvent.Register ||
        !(await this.profileValidator.hasMissingExtraProfileFields(this.#data, user)),
      new RequestError(
        {
          code: 'user.missing_profile',
          status: 422,
        },
        { missingProfile: [MissingProfile.extraProfile] }
      )
    );
  }

  /**
   * Set profile without validation.
   * - skip profile uniqueness check.
   * - skip profile existence check in the current user account.
   */
  unsafeSet(profile: InteractionProfile) {
    this.#data = {
      ...this.#data,
      ...profile,
    };
  }

  /**
   * Prepend the profile data to the existing profile data.
   * Avoid overwriting the existing profile data.