ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/vault
File: ui/app/components/mount-backend/type-form.js (JavaScript)

Code before:

import Component from '@glimmer/component';
import { service } from '@ember/service';
import { filterEnginesByMountType } from 'vault/utils/all-engines-metadata';

/**
 *
 * @module MountBackendTypeForm
 * MountBackendTypeForm components are used to display type options for
 * mounting either an auth method or secret engine.
 *
 * @example
 * ```js
 * <MountBackend::TypeForm @setMountType={{this.setMountType}} @mountType="secret" />
 * ```
 * @param {CallableFunction} setMountType - function will receive the mount type string. Should update the model type value
 * @param {string} [mountType=auth] - mount type can be `auth` or `secret`
 */

export default class MountBackendTypeForm extends Component {
  @service version;

  get secretEngines() {
    // If an enterprise license is present, return all secret engines;
    // otherwise, return only the secret engines supported in OSS.
    return filterEnginesByMountType('secret', this.version.isEnterprise);
  }

  get authMethods() {
    // If an enterprise license is present, return all auth methods;
    // otherwise, return only the auth methods supported in OSS.
    return filterEnginesByMountType('auth', this.version.isEnterprise);
  }

  get mountTypes() {
    return this.args.mountType === 'secret' ? this.secretEngines : this.authMethods;
  }
}


Review comment (security):
```suggestion
    return filterEnginesByMountType({ mountType = 'secret', includeEnterprise = this.version.isEnterprise });
```

Code after:

import Component from '@glimmer/component';
import { service } from '@ember/service';
import { filterEnginesByMountCategory } from 'vault/utils/all-engines-metadata';

/**
 *
 * @module MountBackendTypeForm
 * MountBackendTypeForm components are used to display type options for
 * mounting either an auth method or secret engine.
 *
 * @example
 * ```js
 * <MountBackend::TypeForm @setMountType={{this.setMountType}} @mountCategory="secret" />
 * ```
 * @param {CallableFunction} setMountType - function will receive the mount type string. Should update the model type value
 * @param {string} [mountCategory=auth] - mount category type can be `auth` or `secret`
 */

export default class MountBackendTypeForm extends Component {
  @service version;

  get secretEngines() {
    // If an enterprise license is present, return all secret engines;
    // otherwise, return only the secret engines supported in OSS.
    return filterEnginesByMountCategory({ mountCategory: 'secret', isEnterprise: this.version.isEnterprise });
  }

  get authMethods() {
    // If an enterprise license is present, return all auth methods;
    // otherwise, return only the auth methods supported in OSS.
    return filterEnginesByMountCategory({ mountCategory: 'auth', isEnterprise: this.version.isEnterprise });
  }

  get mountTypes() {
    return this.args.mountCategory === 'secret' ? this.secretEngines : this.authMethods;
  }
}
Repository: hashicorp/vault
File: ui/app/components/mount-backend/type-form.js (JavaScript)

Code before:
 *
 * @module MountBackendTypeForm
 * MountBackendTypeForm components are used to display type options for
 * mounting either an auth method or secret engine.
 *
 * @example
 * ```js
 * <MountBackend::TypeForm @setMountType={{this.setMountType}} @mountType="secret" />
 * ```
 * @param {CallableFunction} setMountType - function will receive the mount type string. Should update the model type value
 * @param {string} [mountType=auth] - mount type can be `auth` or `secret`
 */

export default class MountBackendTypeForm extends Component {
  @service version;

  get secretEngines() {
    // If an enterprise license is present, return all secret engines;
    // otherwise, return only the secret engines supported in OSS.
    return filterEnginesByMountType('secret', this.version.isEnterprise);
  }

  get authMethods() {
    // If an enterprise license is present, return all auth methods;
    // otherwise, return only the auth methods supported in OSS.
    return filterEnginesByMountType('auth', this.version.isEnterprise);
  }

  get mountTypes() {
    return this.args.mountType === 'secret' ? this.secretEngines : this.authMethods;
  }
}


Review comment (security):
```suggestion
    return filterEnginesByMountType({ mountType = 'auth', includeEnterprise = this.version.isEnterprise });
```

Code after:
 *
 * @module MountBackendTypeForm
 * MountBackendTypeForm components are used to display type options for
 * mounting either an auth method or secret engine.
 *
 * @example
 * ```js
 * <MountBackend::TypeForm @setMountType={{this.setMountType}} @mountCategory="secret" />
 * ```
 * @param {CallableFunction} setMountType - function will receive the mount type string. Should update the model type value
 * @param {string} [mountCategory=auth] - mount category type can be `auth` or `secret`
 */

export default class MountBackendTypeForm extends Component {
  @service version;

  get secretEngines() {
    // If an enterprise license is present, return all secret engines;
    // otherwise, return only the secret engines supported in OSS.
    return filterEnginesByMountCategory({ mountCategory: 'secret', isEnterprise: this.version.isEnterprise });
  }

  get authMethods() {
    // If an enterprise license is present, return all auth methods;
    // otherwise, return only the auth methods supported in OSS.
    return filterEnginesByMountCategory({ mountCategory: 'auth', isEnterprise: this.version.isEnterprise });
  }

  get mountTypes() {
    return this.args.mountCategory === 'secret' ? this.secretEngines : this.authMethods;
  }
}
Repository: hashicorp/vault
File: ui/app/models/mfa-login-enforcement.js (JavaScript)

Code before:
      targets = addToArray(targets, {
        key: 'auth_method_types',
        icon,
        title: type,
        subTitle: `All ${type} mounts (${mountCount})`,
      });
    });

    for (const key of ['identity_entities', 'identity_groups']) {
      (await this[key]).forEach((model) => {
        targets = addToArray(targets, {
          key,
          icon: 'user',
          link: 'vault.cluster.access.identity.show',
          linkModels: [key.split('_')[1], model.id, 'details'],
          title: model.name,
          subTitle: model.id,
        });
      });
    }

    return targets;
  }

  iconForMount(type) {
    const mountableMethods = filterEnginesByMountType('auth');
    const mount = mountableMethods.find((method) => method.type === type);
    return mount ? mount.glyph || mount.type : 'token';
  }
}


Review comment (security):
Gut check me, I have includeEnterprise as false, but it kind of feels like it should be true? 
```suggestion
    const mountableMethods = filterEnginesByMountType({ mountType = 'auth', includeEnterprise = false });
```

Code after:
      targets = addToArray(targets, {
        key: 'auth_method_types',
        icon,
        title: type,
        subTitle: `All ${type} mounts (${mountCount})`,
      });
    });

    for (const key of ['identity_entities', 'identity_groups']) {
      (await this[key]).forEach((model) => {
        targets = addToArray(targets, {
          key,
          icon: 'user',
          link: 'vault.cluster.access.identity.show',
          linkModels: [key.split('_')[1], model.id, 'details'],
          title: model.name,
          subTitle: model.id,
        });
      });
    }

    return targets;
  }

  iconForMount(type) {
    const mountableMethods = filterEnginesByMountCategory({ mountCategory: 'auth', isEnterprise: true });
    const mount = mountableMethods.find((method) => method.type === type);
    return mount ? mount.glyph || mount.type : 'token';
  }
}
Repository: hashicorp/vault
File: ui/app/utils/all-engines-metadata.ts (TypeScript)

Code before:
  category?: string;
  displayName: string;
  glyph?: string;
  mountGroup: string[];
  requiresEnterprise?: boolean;
  type: string;
  value?: string;
}

/**
 * @param mountGroup - Given mount group to filter by, e.g., 'auth' or 'secret'.
 * @param isEnterprise - Optional boolean to indicate if enterprise engines should be included in the results.
 * @returns Filtered array of engines that match the given mount type
 */
export function filterEnginesByMountType(mountGroup: 'auth' | 'secret', isEnterprise?: boolean) {
  return isEnterprise
    ? ALL_ENGINES.filter((engine) => engine.mountGroup.includes(mountGroup))
    : ALL_ENGINES.filter((engine) => engine.mountGroup.includes(mountGroup) && !engine.requiresEnterprise);
}

export const ALL_ENGINES: EngineDisplayData[] = [
  {
    category: 'cloud',
    displayName: 'AliCloud',
    glyph: 'alibaba-color',
    mountGroup: ['auth', 'secret'],
    type: 'alicloud',
  },
  {
    category: 'generic',
    displayName: 'AppRole',
    glyph: 'cpu',
    mountGroup: ['auth'],
    type: 'approle',
    value: 'approle',
  },
  {
    category: 'cloud',
    displayName: 'AWS',
    glyph: 'aws-color',
    mountGroup: ['auth', 'secret'],
    type: 'aws',
  },
  {
    category: 'cloud',
    displayName: 'Azure',
    glyph: 'azure-color',
    mountGroup: ['auth', 'secret'],
    type: 'azure',
  },
  {

Review comment (suggestion):
we use mountType higher up. I realize mountType returns a single value and mountGroup returns an array. But to draw a connection between these can we rename this to `mountTypes` (plural)

Code after:
  category?: string;
  displayName: string;
  glyph?: string;
  mountCategory: string[];
  requiresEnterprise?: boolean;
  type: string;
  value?: string;
}

/**
 * @param mountCategory - Given mount category to filter by, e.g., 'auth' or 'secret'.
 * @param isEnterprise - Optional boolean to indicate if enterprise engines should be included in the results.
 * @returns Filtered array of engines that match the given mount category
 */
export function filterEnginesByMountCategory({
  mountCategory,
  isEnterprise = false,
}: {
  mountCategory: 'auth' | 'secret';
  isEnterprise: boolean;
}) {
  return isEnterprise
    ? ALL_ENGINES.filter((engine) => engine.mountCategory.includes(mountCategory))
    : ALL_ENGINES.filter(
        (engine) => engine.mountCategory.includes(mountCategory) && !engine.requiresEnterprise
      );
}

export const ALL_ENGINES: EngineDisplayData[] = [
  {
    category: 'cloud',
    displayName: 'AliCloud',
    glyph: 'alibaba-color',
    mountCategory: ['auth', 'secret'],
    type: 'alicloud',
  },
  {
    category: 'generic',
    displayName: 'AppRole',
    glyph: 'cpu',
    mountCategory: ['auth'],
    type: 'approle',
    value: 'approle',
  },
  {
    category: 'cloud',
    displayName: 'AWS',
    glyph: 'aws-color',
    mountCategory: ['auth', 'secret'],
    type: 'aws',
  },
Repository: hashicorp/vault
File: ui/app/utils/all-engines-metadata.ts (TypeScript)

Code before:
/**
 * Copyright (c) HashiCorp, Inc.
 * SPDX-License-Identifier: BUSL-1.1
 */

/**
 * These are all the secret and auth methods, including enterprise.
 */

export interface EngineDisplayData {
  category?: string;
  displayName: string;
  glyph?: string;
  mountGroup: string[];
  requiresEnterprise?: boolean;
  type: string;
  value?: string;
}

/**
 * @param mountGroup - Given mount group to filter by, e.g., 'auth' or 'secret'.
 * @param isEnterprise - Optional boolean to indicate if enterprise engines should be included in the results.
 * @returns Filtered array of engines that match the given mount type
 */
export function filterEnginesByMountType(mountGroup: 'auth' | 'secret', isEnterprise?: boolean) {
  return isEnterprise
    ? ALL_ENGINES.filter((engine) => engine.mountGroup.includes(mountGroup))
    : ALL_ENGINES.filter((engine) => engine.mountGroup.includes(mountGroup) && !engine.requiresEnterprise);
}

export const ALL_ENGINES: EngineDisplayData[] = [
  {
    category: 'cloud',
    displayName: 'AliCloud',
    glyph: 'alibaba-color',
    mountGroup: ['auth', 'secret'],
    type: 'alicloud',
  },
  {
    category: 'generic',
    displayName: 'AppRole',
    glyph: 'cpu',
    mountGroup: ['auth'],
    type: 'approle',
    value: 'approle',
  },
  {
    category: 'cloud',
    displayName: 'AWS',
    glyph: 'aws-color',

Review comment (security):
I also think the name of this function should maybe match our new verbiage of `mountGroup` - what do you think? Otherwise on initial read would think I can pass any engine type (i.e. `aws`) and it returns data for that type
```suggestion
export function filterEnginesByMountGroup(mountGroup: 'auth' | 'secret', isEnterprise?: boolean) {
```

Code after:
/**
 * Copyright (c) HashiCorp, Inc.
 * SPDX-License-Identifier: BUSL-1.1
 */

/**
 * These are all the secret and auth methods, including enterprise.
 */

export interface EngineDisplayData {
  category?: string;
  displayName: string;
  glyph?: string;
  mountCategory: string[];
  requiresEnterprise?: boolean;
  type: string;
  value?: string;
}

/**
 * @param mountCategory - Given mount category to filter by, e.g., 'auth' or 'secret'.
 * @param isEnterprise - Optional boolean to indicate if enterprise engines should be included in the results.
 * @returns Filtered array of engines that match the given mount category
 */
export function filterEnginesByMountCategory({
  mountCategory,
  isEnterprise = false,
}: {
  mountCategory: 'auth' | 'secret';
  isEnterprise: boolean;
}) {
  return isEnterprise
    ? ALL_ENGINES.filter((engine) => engine.mountCategory.includes(mountCategory))
    : ALL_ENGINES.filter(
        (engine) => engine.mountCategory.includes(mountCategory) && !engine.requiresEnterprise
      );
}

export const ALL_ENGINES: EngineDisplayData[] = [
  {
    category: 'cloud',
    displayName: 'AliCloud',
    glyph: 'alibaba-color',
    mountCategory: ['auth', 'secret'],
    type: 'alicloud',
  },
  {
    category: 'generic',
    displayName: 'AppRole',
    glyph: 'cpu',
Repository: hashicorp/vault
File: ui/app/components/namespace-picker.ts (TypeScript)

Code before:
      this.canRefreshNamespaces = namespacePermission.get('canList');
      this.canManageNamespaces = true;
    } catch (error) {
      // If the findRecord call fails, the user lacks permissions to refresh or manage namespaces.
      this.canRefreshNamespaces = this.canManageNamespaces = false;
    } finally {
      waiter.endAsync(waiterToken);
    }
  }

  @action
  focusSearchInput(element: HTMLInputElement): void {
    // On mount, cursor should default to the search input field
    element.focus();
  }

  @action
  async loadOptions(): Promise<void> {
    try {
      await this.namespace?.findNamespacesForUser?.perform();
      this.errorLoadingNamespaces = '';
    } catch (error) {
      this.errorLoadingNamespaces = errorMessage(error);
    }

    await this.fetchListCapability();
  }

  @action
  loadMore(): void {
    // Increase the batch size to load more items
    this.batchSize += 200;
  }

  @action
  setupScrollListener(element: HTMLElement): void {
    element.addEventListener('scroll', this.onScroll);
  }

  @action
  onScroll(event: Event): void {
    const element = event.target as HTMLElement;

    // Check if the user has scrolled to the bottom
    if (element.scrollTop + element.clientHeight >= element.scrollHeight) {
      this.loadMore();
    }
  }

  @action
  async onChange(selected: NamespaceOption): Promise<void> {

Review comment (suggestion):
I didn't see a reason for this to be re-requested when the options are loaded since it's unlikely the capability would need to update when the options change.

Code after:
    this.canManageNamespaces = canList;
  }

  @action
  focusSearchInput(element: HTMLInputElement): void {
    // On mount, cursor should default to the search input field
    element.focus();
  }

  @action
  async loadOptions(): Promise<void> {
    try {
      await this.namespace?.findNamespacesForUser?.perform();
      this.errorLoadingNamespaces = '';
    } catch (error) {
      this.errorLoadingNamespaces = errorMessage(error);
    }
  }

  @action
  loadMore(): void {
    // Increase the batch size to load more items
    this.batchSize += 200;
  }

  @action
  setupScrollListener(element: HTMLElement): void {
    element.addEventListener('scroll', this.onScroll);
  }

  @action
  onScroll(event: Event): void {
    const element = event.target as HTMLElement;

    // Check if the user has scrolled to the bottom
    if (element.scrollTop + element.clientHeight >= element.scrollHeight) {
      this.loadMore();
    }
  }

  @action
  async onChange(selected: NamespaceOption): Promise<void> {
    this.searchInput = '';
    this.router.transitionTo('vault.cluster.dashboard', { queryParams: { namespace: selected.path } });
  }

  @action
  async onKeyDown(event: KeyboardEvent): Promise<void> {
    if (event.key === keys.ENTER && this.searchInput?.trim()) {
      const matchingNamespace = this.allNamespaces.find((ns) => ns.label === this.searchInput.trim());
Repository: hashicorp/vault
File: builtin/credential/ldap/path_login.go (Go)

Code before:
				Description: "DN (distinguished name) to be used for login.",
			},

			"password": {
				Type:        framework.TypeString,
				Description: "Password for this user.",
			},
		},

		Callbacks: map[logical.Operation]framework.OperationFunc{
			logical.UpdateOperation:         b.pathLogin,
			logical.AliasLookaheadOperation: b.pathLoginAliasLookahead,
		},

		HelpSynopsis:    pathLoginSyn,
		HelpDescription: pathLoginDesc,
	}
}

func (b *backend) pathLoginAliasLookahead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
	username := d.Get("username").(string)
	cfg, err := b.Config(ctx, req)
	if err != nil {
		return nil, err
	}
	if !*cfg.CaseSensitiveNames {
		username = strings.ToLower(username)
	}
	if username == "" {
		return nil, fmt.Errorf("missing username")
	}

	return &logical.Response{
		Auth: &logical.Auth{
			Alias: &logical.Alias{
				Name: username,
			},
		},
	}, nil
}

func (b *backend) pathLogin(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
	cfg, err := b.Config(ctx, req)
	if err != nil {
		return nil, err
	}
	if cfg == nil {
		return logical.ErrorResponse("auth method not configured"), nil
	}

	// Check for a CIDR match.

Review comment (suggestion):
Is it possible that that `cfg` can be nil here? If so, we might want to guard against the nil pointer panic on line 54

Code after:
				Description: "DN (distinguished name) to be used for login.",
			},

			"password": {
				Type:        framework.TypeString,
				Description: "Password for this user.",
			},
		},

		Callbacks: map[logical.Operation]framework.OperationFunc{
			logical.UpdateOperation:         b.pathLogin,
			logical.AliasLookaheadOperation: b.pathLoginAliasLookahead,
		},

		HelpSynopsis:    pathLoginSyn,
		HelpDescription: pathLoginDesc,
	}
}

func (b *backend) pathLoginAliasLookahead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
	username := d.Get("username").(string)
	if username == "" {
		return nil, fmt.Errorf("missing username")
	}

	cfg, err := b.Config(ctx, req)
	if err != nil {
		return nil, err
	}

	caseSensitive := cfg != nil && cfg.CaseSensitiveNames != nil && *cfg.CaseSensitiveNames
	// if the username is not configured to be case-sensitive, we normalize it to lower case.
	if !caseSensitive {
		username = strings.ToLower(username)
	}
	return &logical.Response{
		Auth: &logical.Auth{
			Alias: &logical.Alias{
				Name: username,
			},
		},
	}, nil
}

func (b *backend) pathLogin(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
	cfg, err := b.Config(ctx, req)
	if err != nil {
		return nil, err
	}
	if cfg == nil {
		return logical.ErrorResponse("auth method not configured"), nil
Repository: hashicorp/vault
File: builtin/credential/ldap/path_login.go (Go)

Code before:
				Description: "DN (distinguished name) to be used for login.",
			},

			"password": {
				Type:        framework.TypeString,
				Description: "Password for this user.",
			},
		},

		Callbacks: map[logical.Operation]framework.OperationFunc{
			logical.UpdateOperation:         b.pathLogin,
			logical.AliasLookaheadOperation: b.pathLoginAliasLookahead,
		},

		HelpSynopsis:    pathLoginSyn,
		HelpDescription: pathLoginDesc,
	}
}

func (b *backend) pathLoginAliasLookahead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
	username := d.Get("username").(string)
	cfg, err := b.Config(ctx, req)
	if err != nil {
		return nil, err
	}
	if !*cfg.CaseSensitiveNames {
		username = strings.ToLower(username)
	}
	if username == "" {
		return nil, fmt.Errorf("missing username")
	}

	return &logical.Response{
		Auth: &logical.Auth{
			Alias: &logical.Alias{
				Name: username,
			},
		},
	}, nil
}

func (b *backend) pathLogin(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
	cfg, err := b.Config(ctx, req)
	if err != nil {
		return nil, err
	}
	if cfg == nil {
		return logical.ErrorResponse("auth method not configured"), nil
	}

	// Check for a CIDR match.

Review comment (suggestion):
This looks like another potential nil pointer dereference. You will want to guard against this runtime error with a nil check.

Code after:
				Description: "DN (distinguished name) to be used for login.",
			},

			"password": {
				Type:        framework.TypeString,
				Description: "Password for this user.",
			},
		},

		Callbacks: map[logical.Operation]framework.OperationFunc{
			logical.UpdateOperation:         b.pathLogin,
			logical.AliasLookaheadOperation: b.pathLoginAliasLookahead,
		},

		HelpSynopsis:    pathLoginSyn,
		HelpDescription: pathLoginDesc,
	}
}

func (b *backend) pathLoginAliasLookahead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
	username := d.Get("username").(string)
	if username == "" {
		return nil, fmt.Errorf("missing username")
	}

	cfg, err := b.Config(ctx, req)
	if err != nil {
		return nil, err
	}

	caseSensitive := cfg != nil && cfg.CaseSensitiveNames != nil && *cfg.CaseSensitiveNames
	// if the username is not configured to be case-sensitive, we normalize it to lower case.
	if !caseSensitive {
		username = strings.ToLower(username)
	}
	return &logical.Response{
		Auth: &logical.Auth{
			Alias: &logical.Alias{
				Name: username,
			},
		},
	}, nil
}

func (b *backend) pathLogin(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
	cfg, err := b.Config(ctx, req)
	if err != nil {
		return nil, err
	}
	if cfg == nil {
		return logical.ErrorResponse("auth method not configured"), nil