ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/vault
File: tools/pipeline/internal/pkg/changed/checkers.go (Go)

Code before:
	FileGroupCheckerDir,
	FileGroupCheckerFileName,
	FileGroupCheckerFileGo,
	FileGroupCheckerFileProto,
}

// FileGroupCheckerDir is a file group checker that groups based on the files directory
func FileGroupCheckerDir(ctx context.Context, file *File) FileGroups {
	name := file.Name()
	groups := FileGroups{}

	for dir, groups := range map[string]FileGroups{
		".github":   groups.Add(FileGroupPipeline),
		"changelog": groups.Add(FileGroupChangelog),
		"enos":      groups.Add(FileGroupEnos),
		"tools":     groups.Add(FileGroupTools),
		"ui":        groups.Add(FileGroupWebUI),
		"website":   groups.Add(FileGroupDocs),
	} {
		if strings.HasPrefix(name, dir+string(os.PathSeparator)) {
			return groups
		}
	}

	return nil
}

// FileGroupCheckerFileName is a file group checker that groups based on the files name
func FileGroupCheckerFileName(ctx context.Context, file *File) FileGroups {
	name := file.Name()
	groups := FileGroups{}

	switch {
	case strings.HasPrefix(name, "buf."):
		return groups.Add(FileGroupProto)
	case strings.HasPrefix(name, "CHANGELOG"):
		return groups.Add(FileGroupChangelog)
	case strings.HasPrefix(name, "CODEOWNERS"):
		return groups.Add(FileGroupPipeline)
	case strings.HasSuffix(name, "go.mod") || strings.HasSuffix(name, "go.sum"):
		return groups.Add(FileGroupGoModules, FileGroupGoApp)
	}

	return nil
}

// FileGroupCheckerFileGo is a file group checker that groups based on the files extension being .go
func FileGroupCheckerFileGo(ctx context.Context, file *File) FileGroups {
	name := file.Name()

	ext := filepath.Ext(name)

Review comment (suggestion):
```suggestion
// GroupFiles takes a context, a slice of files, and one-to-many file group checkers and adds group
```

Code after:
	FileGroupCheckerApp,
	FileGroupCheckerAutopilot,
	FileGroupCheckerChangelog,
	FileGroupCheckerCommunity,
	FileGroupCheckerDocs,
	FileGroupCheckerEnos,
	FileGroupCheckerEnterprise,
	FileGroupCheckerGoToolchain,
	FileGroupCheckerPipeline,
	FileGroupCheckerProto,
	FileGroupCheckerWebUI,
}

// Group takes a context, a file, and one-to-many file group checkers and adds group metadata to
// the file.
func Group(ctx context.Context, file *File, checkers ...FileGroupCheck) {
	if file == nil || len(checkers) < 1 {
		return
	}

	for _, check := range checkers {
		file.Groups = file.Groups.Add(check(ctx, file)...)
	}
}

// GroupFiles takes a context, a slice of files, and one-to-many file group checkers and adds group
// metadata to the files.
func GroupFiles(ctx context.Context, files []*File, checkers ...FileGroupCheck) {
	for _, file := range files {
		Group(ctx, file, checkers...)
	}
}

// FileGroupCheckerApp is a file group checker that groups based on the file being part of the Vault
// Go app
func FileGroupCheckerApp(ctx context.Context, file *File) FileGroups {
	name := file.Name()
	ext := filepath.Ext(name)

	switch {
	case hasBaseDir(name, filepath.Join("tools", "pipeline")):
		return nil
	case
		ext == ".go",
		strings.HasSuffix(name, "go.mod"),
		strings.HasSuffix(name, "go.sum"):
		return FileGroups{FileGroupGoApp}
	default:
		return nil
	}
}
Repository: hashicorp/vault
File: ui/app/components/auth/form/base.ts (TypeScript)

Code before:

  login = task(
    waitFor(async (data) => {
      try {
        const authResponse = await this.auth.authenticate({
          clusterId: this.args.cluster.id,
          backend: this.args.authType,
          data,
          selectedAuth: this.args.authType,
        });

        // responsible for redirect after auth data is persisted
        this.onSuccess(authResponse);
      } catch (error) {
        this.onError(error as Error);
      }
    })
  );

  // if we move auth service authSuccess method here (or to each auth method component)
  // then call that before calling parent this.args.onSuccess
  onSuccess(authResponse: object) {
    //  responsible for redirect after auth data is persisted
    this.args.onSuccess(authResponse, this.args.authType);
  }

  onError(error: Error) {
    if (!this.auth.mfaErrors) {
      const errorMessage = `Authentication failed: ${this.auth.handleError(error)}`;
      this.args.onError(errorMessage);
    }
  }
}


Review comment (security):
Could `this.args.onSuccess(authResponse)` be called directly from the login method instead?

Code after:
    }

    this.login.unlinked().perform(data);
  }

  login = task(
    waitFor(async (formData) => {
      try {
        const authResponse = await this.auth.authenticate({
          clusterId: this.args.cluster.id,
          backend: this.args.authType,
          data: formData,
          selectedAuth: this.args.authType,
        });

        this.handleAuthResponse(authResponse);
      } catch (error) {
        this.onError(error as Error);
      }
    })
  );

  handleAuthResponse(authResponse: AuthData) {
    // calls onAuthResponse in parent auth/page.js component
    this.args.onSuccess(authResponse);
  }

  onError(error: Error) {
    if (!this.auth.mfaErrors) {
      const errorMessage = `Authentication failed: ${this.auth.handleError(error)}`;
      this.args.onError(errorMessage);
    }
  }
}
Repository: hashicorp/vault
File: sdk/helper/certutil/helpers.go (Go)

Code before:
					return urls, errors.New("x509: invalid CRL distribution point")
				}
				for !dpNameDER.Empty() {
					if !dpNameDER.PeekASN1Tag(asn1.TagOID | 128) { // Context is GeneralName
						break
					}
					var uri cryptobyte.String
					if !dpNameDER.ReadASN1(&uri, asn1.TagOID|128) { // Context is GeneralName
						return urls, errors.New("x509: invalid CRL distribution point")
					}
					urls = append(urls, string(uri))
				}
			}
			return urls, nil
		}
	}
	fmt.Print(logCert)
	return urls, nil
}

func AddDeltaCRLExtension(data *CreationBundle, certTemplate *x509.Certificate) {
	if data.Params != nil && data.Params.URLs != nil && data.Params.URLs.DeltaCRLDistributionPoints != nil &&
		len(data.Params.URLs.DeltaCRLDistributionPoints) > 0 {
		extension, err := CreateDeltaCRLExtension(data.Params.URLs.DeltaCRLDistributionPoints)
		if err != nil {
			return
		}
		if certTemplate.ExtraExtensions == nil {
			certTemplate.ExtraExtensions = []pkix.Extension{extension}
		} else {
			certTemplate.ExtraExtensions = append(certTemplate.ExtraExtensions, extension)
		}
	}
}


Review comment (bug):
Do we really want to hide the error here? Should we not at least return a warning?

Code after:
				if !dpDER.ReadOptionalASN1(&dpNameDER, &dpNamePresent, asn1.ClassUniversal|32|128) {
					return urls, errors.New("x509: invalid CRL distribution point")
				}
				if !dpNamePresent {
					continue
				}
				if !dpNameDER.ReadASN1(&dpNameDER, asn1.ClassUniversal|32|128) {
					return urls, errors.New("x509: invalid CRL distribution point")
				}
				for !dpNameDER.Empty() {
					if !dpNameDER.PeekASN1Tag(asn1.TagOID | 128) { // Context is GeneralName
						break
					}
					var uri cryptobyte.String
					if !dpNameDER.ReadASN1(&uri, asn1.TagOID|128) { // Context is GeneralName
						return urls, errors.New("x509: invalid CRL distribution point")
					}
					urls = append(urls, string(uri))
				}
			}
			return urls, nil
		}
	}
	return urls, nil
}

func AddDeltaCRLExtension(data *CreationBundle, certTemplate *x509.Certificate) error {
	if data.Params != nil && data.Params.URLs != nil && data.Params.URLs.DeltaCRLDistributionPoints != nil &&
		len(data.Params.URLs.DeltaCRLDistributionPoints) > 0 {
		extension, err := CreateDeltaCRLExtension(data.Params.URLs.DeltaCRLDistributionPoints)
		if err != nil {
			return fmt.Errorf("failed to create delta crl extension: %w", err)
		}
		certTemplate.ExtraExtensions = append(certTemplate.ExtraExtensions, extension)
	}
	return nil
}
Repository: hashicorp/vault
File: builtin/logical/database/path_roles_test.go (Go)

Code before:
		Operation: logical.ReadOperation,
		Path:      "static-creds/" + roleName,
		Storage:   s,
	})
	if err != nil || (resp != nil && resp.IsError()) {
		t.Fatal(resp, err)
	}
	return resp
}

func updateStaticRoleWithData(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string, d map[string]interface{}) {
	t.Helper()

	mockDB.On("UpdateUser", mock.Anything, mock.Anything).
		Return(v5.UpdateUserResponse{}, nil).
		Once()

	req := &logical.Request{
		Operation: logical.UpdateOperation,
		Path:      "static-roles/" + roleName,
		Storage:   storage,
		Data:      d,
	}

	resp, err := b.HandleRequest(context.Background(), req)
	if err != nil || (resp != nil && resp.IsError()) {
		t.Fatal(resp, err)
	}
}

const testRoleStaticCreate = `
CREATE ROLE "{{name}}" WITH
  LOGIN
  PASSWORD '{{password}}';
`

const testRoleStaticUpdate = `
ALTER USER "{{name}}" WITH PASSWORD '{{password}}';
`

const testRoleStaticUpdateRotation = `
ALTER USER "{{name}}" WITH PASSWORD '{{password}}';GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA public TO "{{name}}";
`


Review comment (suggestion):
This changes seems a bit odd. Could we change the `require.NoError` to `assert.NoError`, check `resp`? It is possible to call `t.Fatal` with a `nil` error.

Code after:
		Once()
	resp, err := b.HandleRequest(context.Background(), &logical.Request{
		Operation: logical.CreateOperation,
		Path:      "static-roles/" + roleName,
		Storage:   s,
		Data:      data,
	})
	if err != nil || (resp != nil && resp.IsError()) {
		t.Fatal(resp, err)
	}
}

func readStaticCred(t *testing.T, b *databaseBackend, s logical.Storage, mockDB *mockNewDatabase, roleName string) *logical.Response {
	t.Helper()
	mockDB.On("UpdateUser", mock.Anything, mock.Anything).
		Return(v5.UpdateUserResponse{}, nil).
		Once()
	resp, err := b.HandleRequest(context.Background(), &logical.Request{
		Operation: logical.ReadOperation,
		Path:      "static-creds/" + roleName,
		Storage:   s,
	})
	if err != nil || (resp != nil && resp.IsError()) {
		t.Fatal(resp, err)
	}
	return resp
}

func updateStaticRoleWithData(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string, d map[string]interface{}) {
	t.Helper()

	mockDB.On("UpdateUser", mock.Anything, mock.Anything).
		Return(v5.UpdateUserResponse{}, nil).
		Once()

	req := &logical.Request{
		Operation: logical.UpdateOperation,
		Path:      "static-roles/" + roleName,
		Storage:   storage,
		Data:      d,
	}

	resp, err := b.HandleRequest(context.Background(), req)
	assert.NoError(t, err, "unexpected error")
	if resp != nil {
		assert.NoError(t, resp.Error(), "unexpected error in response")
	}

	if t.Failed() {
		require.FailNow(t, "failed to update static role: %s", roleName)
	}
Repository: hashicorp/vault
File: ui/types/vault/app-types.ts (TypeScript)

Code before:
  formFields: Array<FormField>;
  formFieldGroups: FormFieldGroups;
  allFields: Array<FormField>;
};

export type WithValidationsModel = Model & {
  validate(): ModelValidations;
};

export type WithFormFieldsAndValidationsModel = WithFormFieldsModel & {
  validate(): ModelValidations;
};

export interface Breadcrumb {
  label: string;
  route?: string;
  icon?: string;
  model?: string;
  models?: string[];
  linkExternal?: boolean;
}

export interface TtlEvent {
  enabled: boolean;
  seconds: number;
  timeString: string;
  goSafeTimeString: string;
}

export interface EngineOwner extends Owner {
  mountPoint: string;
}

export interface SearchSelectOption {
  name: string;
  id: string;
}

// Generic interfaces
export interface StringMap {
  [key: string]: string;
}


Review comment (suggestion):
the key here would be the path tied to the capability, right? I know I built parts of this service, just trying to remember 😂 

Code after:
  formFields: Array<FormField>;
  formFieldGroups: FormFieldGroups;
  allFields: Array<FormField>;
};

export type WithValidationsModel = Model & {
  validate(): ModelValidations;
};

export type WithFormFieldsAndValidationsModel = WithFormFieldsModel & {
  validate(): ModelValidations;
};

// capabilities
export interface Capabilities {
  canCreate: boolean;
  canDelete: boolean;
  canList: boolean;
  canPatch: boolean;
  canRead: boolean;
  canSudo: boolean;
  canUpdate: boolean;
}

export interface CapabilitiesMap {
  [key: string]: Capabilities;
}

export type CapabilityTypes =
  | 'root'
  | 'sudo'
  | 'deny'
  | 'create'
  | 'read'
  | 'update'
  | 'delete'
  | 'list'
  | 'patch';
export interface CapabilitiesData {
  [key: string]: CapabilityTypes[];
}

export interface Breadcrumb {
  label: string;
  route?: string;
  icon?: string;
  model?: string;
  models?: string[];
  linkExternal?: boolean;
}
Repository: hashicorp/vault
File: ui/app/routes/vault/cluster/secrets/backends.ts (TypeScript)

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

import type Store from '@ember-data/store';

export default class ClientsRoute extends Route {
  @service declare readonly store: Store;

  model() {
    return this.store.query('secret-engine', {});
  }
}


Review comment (security):
This is my bad because I didn't make the change in my spike, but can you update the name here to match the route, so 
```suggestion
export default class SecretsBackends extends Route {
```

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

import type Store from '@ember-data/store';

export default class SecretsBackends extends Route {
  @service declare readonly store: Store;

  model() {
    return this.store.query('secret-engine', {});
  }
}
Repository: hashicorp/vault
File: ui/lib/config-ui/addon/routes.js (JavaScript)

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

import buildRoutes from 'ember-engines/routes';

export default buildRoutes(function () {
  this.route('messages', function () {
    this.route('create');
    this.route('message', { path: '/:id' }, function () {
      this.route('details');
      this.route('edit');
    });
  });

  this.route('login-settings', function () {
    this.route('rule', { path: '/:name' }, function () {
      this.route('details');
      this.route('list');
    });
  });
});


Review comment (suggestion):
the list view is `login-settings` so this shouldn't be here
```suggestion
```

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

import buildRoutes from 'ember-engines/routes';

export default buildRoutes(function () {
  this.route('messages', function () {
    this.route('create');
    this.route('message', { path: '/:id' }, function () {
      this.route('details');
      this.route('edit');
    });
  });

  this.route('login-settings', function () {
    this.route('rule', { path: '/:name' }, function () {
      this.route('details');
    });
  });
});
Repository: hashicorp/vault
File: ui/app/components/auth/fields.ts (TypeScript)

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

// TODO pending feedback from the security team, we may keep autocomplete="off" for login fields

import Component from '@glimmer/component';

interface Args {
  loginFields: Field[];
}

interface Field {
  name: string; // sets input name
  label?: string; // label will be "name" capitalized unless label exists
  helperText?: string;
}

export default class AuthFields extends Component<Args> {
  // token or password should render as "password" types, otherwise render text inputs
  setInputType = (field: string) => (['token', 'password'].includes(field) ? 'password' : 'text');

  setAutocomplete = (fieldName: string) => {
    switch (fieldName) {
      case 'password':
        return 'current-password';
      case 'token':
        return 'off';
      default:
        return fieldName;
    }
  };
}


Review comment (security):
This is small enough it could happen in the component, but I thought the interfaces might be helpful in the future: 
https://github.com/hashicorp/vault/blob/e95f1455cc2b79a9cdc323aaa0c4f6099fb41102/ui/app/components/auth/fields.ts#L8-L16

If they seem unnecessary, happy to remove the backing class!

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

import Component from '@glimmer/component';

interface Args {
  loginFields: Field[];
}

interface Field {
  name: string; // sets input name
  label?: string; // label will be "name" capitalized unless label exists
  helperText?: string;
}

export default class AuthFields extends Component<Args> {
  // token or password should render as "password" types, otherwise render text inputs
  setInputType = (field: string) => (['token', 'password'].includes(field) ? 'password' : 'text');
}
Repository: hashicorp/vault
File: ui/lib/config-ui/addon/routes/login-settings/index.js (JavaScript)

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRoute extends Route {
  @service api;
  @service capabilities;
  @service store;

  async model() {
    const adapter = this.store.adapterFor('application');

    const { data } = await adapter.ajax('/v1/sys/config/ui/login/default-auth', 'GET', {
      data: { list: true },
    });

    // this makes sense with data structure atm, but to be revisited
    const loginRules = [];
    data.keys.forEach((rule) => {
      loginRules.push({ name: rule, namespace: data.key_info[rule].namespace });
    });

    return { loginRules };
  }
}


Review comment (suggestion):
we'll hopefully get to replace this with the api service when the backend API is built! But this looks great for now

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRoute extends Route {
  @service api;
  @service capabilities;
  @service store;

  async model() {
    const adapter = this.store.adapterFor('application');

    const { data } = await adapter.ajax('/v1/sys/config/ui/login/default-auth', 'GET', {
      data: { list: true },
    });

    const loginRules = this.api.keyInfoToArray({ keyInfo: data.key_info, keys: data.keys });

    return { loginRules };
  }
}
Repository: hashicorp/vault
File: ui/lib/config-ui/addon/routes/login-settings/index.js (JavaScript)

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRoute extends Route {
  @service api;
  @service capabilities;
  @service store;

  async model() {
    const adapter = this.store.adapterFor('application');

    const { data } = await adapter.ajax('/v1/sys/config/ui/login/default-auth', 'GET', {
      data: { list: true },
    });

    // this makes sense with data structure atm, but to be revisited
    const loginRules = [];
    data.keys.forEach((rule) => {
      loginRules.push({ name: rule, namespace: data.key_info[rule].namespace });
    });

    return { loginRules };
  }
}


Review comment (suggestion):
This logic is fairly common and @zofskeez made a util here: https://github.com/hashicorp/vault/blob/main/ui/app/services/api.ts#L187 if you want to try using that here

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRoute extends Route {
  @service api;
  @service capabilities;
  @service store;

  async model() {
    const adapter = this.store.adapterFor('application');

    const { data } = await adapter.ajax('/v1/sys/config/ui/login/default-auth', 'GET', {
      data: { list: true },
    });

    const loginRules = this.api.keyInfoToArray({ keyInfo: data.key_info, keys: data.keys });

    return { loginRules };
  }
}
Repository: hashicorp/vault
File: ui/lib/config-ui/addon/routes/login-settings/index.js (JavaScript)

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRoute extends Route {
  @service api;
  @service capabilities;
  @service store;

  async model() {
    const adapter = this.store.adapterFor('application');

    const { data } = await adapter.ajax('/v1/sys/config/ui/login/default-auth', 'GET', {
      data: { list: true },
    });

    // this makes sense with data structure atm, but to be revisited
    const loginRules = [];
    data.keys.forEach((rule) => {
      loginRules.push({ name: rule, namespace: data.key_info[rule].namespace });
    });

    return { loginRules };
  }
}


Review comment (refactor):
I think we can remove this, too since the service is injected directly into the component.

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

import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class LoginSettingsRoute extends Route {
  @service api;
  @service capabilities;
  @service store;

  async model() {
    const adapter = this.store.adapterFor('application');

    const { data } = await adapter.ajax('/v1/sys/config/ui/login/default-auth', 'GET', {
      data: { list: true },
    });

    const loginRules = this.api.keyInfoToArray({ keyInfo: data.key_info, keys: data.keys });

    return { loginRules };
  }
}