ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/vault
File: plugins/database/mongodb/mongodb_test.go (Go)

Code before:
			assertDeepEqual(t, test.expectOpts, actual)
		})
	}
}

func appendToCertPool(t *testing.T, pool *x509.CertPool, caPem []byte) *x509.CertPool {
	t.Helper()

	ok := pool.AppendCertsFromPEM(caPem)
	if !ok {
		t.Fatalf("Unable to append cert to cert pool")
	}
	return pool
}

var cmpClientOptionsOpts = cmp.Options{
	cmpopts.IgnoreTypes(http.Transport{}),

	cmp.AllowUnexported(options.ClientOptions{}),

	cmp.AllowUnexported(tls.Config{}),
	cmpopts.IgnoreTypes(sync.Mutex{}, sync.RWMutex{}),

	// 'lazyCerts' has a func field which can't be compared.
	cmpopts.IgnoreFields(x509.CertPool{}, "lazyCerts"),
	cmp.AllowUnexported(x509.CertPool{}),
}

// Need a special comparison for ClientOptions because reflect.DeepEquals won't work in Go 1.16.
// See: https://github.com/golang/go/issues/45891
func assertDeepEqual(t *testing.T, a, b *options.ClientOptions) {
	t.Helper()

	if diff := cmp.Diff(a, b, cmpClientOptionsOpts); diff != "" {
		t.Fatalf("assertion failed: values are not equal\n--- expected\n+++ actual\n%v", diff)
	}
}

func createDBUser(t testing.TB, connURL, db, username, password string) {
	t.Helper()

	ctx, _ := context.WithTimeout(context.Background(), 10*time.Second)
	client, err := mongo.Connect(ctx, options.Client().ApplyURI(connURL))
	if err != nil {
		t.Fatal(err)
	}

	createUserCmd := &createUserCommand{
		Username: username,
		Password: password,
		Roles:    []interface{}{},

Review comment (suggestion):
It would be good if we could also check the actual error message.

Code after:
					Password:      "unittest",
				}),
			expectErr: false,
		},
	}

	for name, test := range tests {
		t.Run(name, func(t *testing.T) {
			c := new()
			c.Username = test.username
			c.Password = test.password
			c.TLSCAData = test.tlsCAData
			c.TLSCertificateKeyData = test.tlsKeyData

			actual, err := c.getTLSAuth()
			if test.expectErr && err == nil {
				t.Fatalf("err expected, got nil")
			}
			if !test.expectErr && err != nil {
				t.Fatalf("no error expected, got: %s", err)
			}
			assertDeepEqual(t, test.expectOpts, actual)
		})
	}
}

func appendToCertPool(t *testing.T, pool *x509.CertPool, caPem []byte) *x509.CertPool {
	t.Helper()

	ok := pool.AppendCertsFromPEM(caPem)
	if !ok {
		t.Fatalf("Unable to append cert to cert pool")
	}
	return pool
}

var cmpClientOptionsOpts = cmp.Options{
	cmpopts.IgnoreTypes(http.Transport{}),

	cmp.AllowUnexported(options.ClientOptions{}),

	cmp.AllowUnexported(tls.Config{}),
	cmpopts.IgnoreTypes(sync.Mutex{}, sync.RWMutex{}),

	// 'lazyCerts' has a func field which can't be compared.
	cmpopts.IgnoreFields(x509.CertPool{}, "lazyCerts"),
	cmp.AllowUnexported(x509.CertPool{}),
}

// Need a special comparison for ClientOptions because reflect.DeepEquals won't work in Go 1.16.
// See: https://github.com/golang/go/issues/45891
Repository: hashicorp/vault
File: plugins/database/mongodb/connection_producer.go (Go)

Code before:
	opts = options.Client()
	opts.SetWriteConcern(writeConcern)
	return opts, nil
}

func (c *mongoDBConnectionProducer) getTLSAuth() (opts *options.ClientOptions, err error) {
	if len(c.TLSCAData) == 0 && len(c.TLSCertificateKeyData) == 0 {
		return nil, nil
	}

	opts = options.Client()

	tlsConfig := &tls.Config{}

	if len(c.TLSCAData) > 0 {
		tlsConfig.RootCAs = x509.NewCertPool()

		ok := tlsConfig.RootCAs.AppendCertsFromPEM(c.TLSCAData)
		if !ok {
			return nil, fmt.Errorf("failed to append CA to client options")
		}
	}

	if len(c.TLSCertificateKeyData) > 0 {
		certificate, err := tls.X509KeyPair(c.TLSCertificateKeyData, c.TLSCertificateKeyData)
		if err != nil {
			return nil, fmt.Errorf("unable to load tls_certificate_key_data: %w", err)
		}

		opts.SetAuth(options.Credential{
			AuthMechanism: "MONGODB-X509",
			Username:      c.Username,
		})

		tlsConfig.Certificates = append(tlsConfig.Certificates, certificate)
	}

	opts.SetTLSConfig(tlsConfig)
	return opts, nil
}

func (c *mongoDBConnectionProducer) timeoutOpts() (opts *options.ClientOptions, err error) {
	opts = options.Client()

	if c.SocketTimeout < 0 {
		return nil, fmt.Errorf("socket_timeout must be >= 0")
	}

	if c.SocketTimeout == 0 {
		opts.SetSocketTimeout(1 * time.Minute)
	} else {

Review comment (security):
Could we use the constant for this value from `x/mongo/driver/auth`  instead? You will need to import `go.mongodb.org/mongo-driver/x/mongo/driver/auth`

```suggestion
		authMechanism := auth.MongoDBX509
```

Code after:

	opts = options.Client()
	opts.SetWriteConcern(writeConcern)
	return opts, nil
}

func (c *mongoDBConnectionProducer) getTLSAuth() (opts *options.ClientOptions, err error) {
	if len(c.TLSCAData) == 0 && len(c.TLSCertificateKeyData) == 0 {
		return nil, nil
	}

	opts = options.Client()

	tlsConfig := &tls.Config{}

	if len(c.TLSCAData) > 0 {
		tlsConfig.RootCAs = x509.NewCertPool()

		ok := tlsConfig.RootCAs.AppendCertsFromPEM(c.TLSCAData)
		if !ok {
			return nil, fmt.Errorf("failed to append CA to client options")
		}
	}

	if len(c.TLSCertificateKeyData) > 0 {
		certificate, err := tls.X509KeyPair(c.TLSCertificateKeyData, c.TLSCertificateKeyData)
		authMechanism := auth.MongoDBX509
		if err != nil {
			return nil, fmt.Errorf("unable to load tls_certificate_key_data: %w", err)
		}

		// Ensure SCRAM-SHA-256 is explicitly set if username/password auth is provided
		if c.Username != "" && c.Password != "" {
			authMechanism = auth.SCRAMSHA256
		}

		opts.SetAuth(options.Credential{
			AuthMechanism: authMechanism,
			Username:      c.Username,
			Password:      c.Password,
		})

		tlsConfig.Certificates = append(tlsConfig.Certificates, certificate)
	}

	opts.SetTLSConfig(tlsConfig)
	return opts, nil
}

func (c *mongoDBConnectionProducer) timeoutOpts() (opts *options.ClientOptions, err error) {
	opts = options.Client()
Repository: hashicorp/vault
File: plugins/database/mongodb/connection_producer.go (Go)

Code before:
		return nil, nil
	}

	opts = options.Client()

	tlsConfig := &tls.Config{}

	if len(c.TLSCAData) > 0 {
		tlsConfig.RootCAs = x509.NewCertPool()

		ok := tlsConfig.RootCAs.AppendCertsFromPEM(c.TLSCAData)
		if !ok {
			return nil, fmt.Errorf("failed to append CA to client options")
		}
	}

	if len(c.TLSCertificateKeyData) > 0 {
		certificate, err := tls.X509KeyPair(c.TLSCertificateKeyData, c.TLSCertificateKeyData)
		if err != nil {
			return nil, fmt.Errorf("unable to load tls_certificate_key_data: %w", err)
		}

		opts.SetAuth(options.Credential{
			AuthMechanism: "MONGODB-X509",
			Username:      c.Username,
		})

		tlsConfig.Certificates = append(tlsConfig.Certificates, certificate)
	}

	opts.SetTLSConfig(tlsConfig)
	return opts, nil
}

func (c *mongoDBConnectionProducer) timeoutOpts() (opts *options.ClientOptions, err error) {
	opts = options.Client()

	if c.SocketTimeout < 0 {
		return nil, fmt.Errorf("socket_timeout must be >= 0")
	}

	if c.SocketTimeout == 0 {
		opts.SetSocketTimeout(1 * time.Minute)
	} else {
		opts.SetSocketTimeout(c.SocketTimeout)
	}

	if c.ConnectTimeout == 0 {
		opts.SetConnectTimeout(1 * time.Minute)
	} else {
		opts.SetConnectTimeout(c.ConnectTimeout)

Review comment (security):
Could we use the constant for this value from `x/mongo/driver/auth`  instead? You will need to import `go.mongodb.org/mongo-driver/x/mongo/driver/auth`


```suggestion
			authMechanism := auth.SCRAMSHA256
```

Code after:
	if len(c.TLSCAData) == 0 && len(c.TLSCertificateKeyData) == 0 {
		return nil, nil
	}

	opts = options.Client()

	tlsConfig := &tls.Config{}

	if len(c.TLSCAData) > 0 {
		tlsConfig.RootCAs = x509.NewCertPool()

		ok := tlsConfig.RootCAs.AppendCertsFromPEM(c.TLSCAData)
		if !ok {
			return nil, fmt.Errorf("failed to append CA to client options")
		}
	}

	if len(c.TLSCertificateKeyData) > 0 {
		certificate, err := tls.X509KeyPair(c.TLSCertificateKeyData, c.TLSCertificateKeyData)
		authMechanism := auth.MongoDBX509
		if err != nil {
			return nil, fmt.Errorf("unable to load tls_certificate_key_data: %w", err)
		}

		// Ensure SCRAM-SHA-256 is explicitly set if username/password auth is provided
		if c.Username != "" && c.Password != "" {
			authMechanism = auth.SCRAMSHA256
		}

		opts.SetAuth(options.Credential{
			AuthMechanism: authMechanism,
			Username:      c.Username,
			Password:      c.Password,
		})

		tlsConfig.Certificates = append(tlsConfig.Certificates, certificate)
	}

	opts.SetTLSConfig(tlsConfig)
	return opts, nil
}

func (c *mongoDBConnectionProducer) timeoutOpts() (opts *options.ClientOptions, err error) {
	opts = options.Client()

	if c.SocketTimeout < 0 {
		return nil, fmt.Errorf("socket_timeout must be >= 0")
	}

	if c.SocketTimeout == 0 {
		opts.SetSocketTimeout(1 * time.Minute)
Repository: hashicorp/vault
File: ui/app/components/generate-credentials-totp.js (JavaScript)

Code before:
      while (this.elapsedTime <= totpCodePeriod) {
        yield timeout(1000);
        this.elapsedTime += 1;
      }

      if (this.elapsedTime > totpCodePeriod) {
        this.elapsedTime = 0;
        this.generateTotpCode(backendPath, keyName);
        this.startTimer.perform();
      }
    }
  }

  async generateTotpCode(backend, keyName) {
    // TODO improvement: refreshing does not currently result in a new code
    try {
      const totpCode = await this.store.adapterFor('totp-key').generateCode(backend, keyName);
      this.totpCode = totpCode.code;
    } catch (e) {
      // swallow error, non-essential data
      return;
    }
  }

  @action redirectPreviousPage() {
    window.history.back();
  }
}


Review comment (security):
Is it possible to accomplish this utilizing the router instead? like 
```
export default class GenerateCredentialsTotp extends Component {
  @service router;
  
  ...
  
  @action redirectPreviousPage() {
    router.goBack();
  }
```

Code after:
    const { backendPath, keyName, totpCodePeriod } = this.args;
    if (totpCodePeriod) {
      this.generateTotpCode(backendPath, keyName);
      while (this.elapsedTime <= totpCodePeriod) {
        yield timeout(1000);
        this.elapsedTime += 1;
      }

      if (this.elapsedTime > totpCodePeriod) {
        this.elapsedTime = 0;
        this.generateTotpCode(backendPath, keyName);
        this.startTimer.perform();
      }
    }
  }

  async generateTotpCode(backend, keyName) {
    // TODO improvement: refreshing does not currently result in a new code
    try {
      const totpCode = await this.store.adapterFor('totp-key').generateCode(backend, keyName);
      this.totpCode = totpCode.code;
    } catch (e) {
      // swallow error, non-essential data
      return;
    }
  }

  @action redirectPreviousPage() {
    const { backRoute, keyName } = this.args;
    if (backRoute === SHOW_ROUTE) {
      this.router.transitionTo(this.args.backRoute, keyName);
    } else {
      this.router.transitionTo(this.args.backRoute);
    }
  }
}
Repository: hashicorp/vault
File: ui/app/components/generate-credentials-totp.js (JavaScript)

Code before:
        yield timeout(1000);
        this.elapsedTime += 1;
      }

      if (this.elapsedTime > totpCodePeriod) {
        this.elapsedTime = 0;
        this.generateTotpCode(backendPath, keyName);
        this.startTimer.perform();
      }
    }
  }

  async generateTotpCode(backend, keyName) {
    // TODO improvement: refreshing does not currently result in a new code
    try {
      const totpCode = await this.store.adapterFor('totp-key').generateCode(backend, keyName);
      this.totpCode = totpCode.code;
    } catch (e) {
      // swallow error, non-essential data
      return;
    }
  }

  @action redirectPreviousPage() {
    window.history.back();
  }
}


Review comment (security):
Curious on the decision to not use the router here and thinking of some potential issues. One possibility is a user direct linking from somewhere other than Vault and if they are authenticated then `history.back()` would then take them out of Vault. Another case might be if there was previously a redirect to the login route (token expired) and on successful login the user is returned here, they would then be taken back to the login.

Code after:
    if (totpCodePeriod) {
      this.generateTotpCode(backendPath, keyName);
      while (this.elapsedTime <= totpCodePeriod) {
        yield timeout(1000);
        this.elapsedTime += 1;
      }

      if (this.elapsedTime > totpCodePeriod) {
        this.elapsedTime = 0;
        this.generateTotpCode(backendPath, keyName);
        this.startTimer.perform();
      }
    }
  }

  async generateTotpCode(backend, keyName) {
    // TODO improvement: refreshing does not currently result in a new code
    try {
      const totpCode = await this.store.adapterFor('totp-key').generateCode(backend, keyName);
      this.totpCode = totpCode.code;
    } catch (e) {
      // swallow error, non-essential data
      return;
    }
  }

  @action redirectPreviousPage() {
    const { backRoute, keyName } = this.args;
    if (backRoute === SHOW_ROUTE) {
      this.router.transitionTo(this.args.backRoute, keyName);
    } else {
      this.router.transitionTo(this.args.backRoute);
    }
  }
}
Repository: hashicorp/vault
File: ui/lib/core/addon/utils/paginate-list.ts (TypeScript)

Code before:
  filter?: string;
  filterKey?: string;
};

/**
 * Util to paginate data set based on page number and size
 * If filter is provided, it will filter the data prior to paginating
 */

export const paginate = (data: PaginatedData, options: PaginateOptions = {}) => {
  const { page = 1, pageSize = DEFAULT_PAGE_SIZE, filter, filterKey } = options;
  let filteredData = data;

  if (Array.isArray(data)) {
    // filter data before paginating if filter is provided
    if (filter) {
      filteredData = data.filter((item) => {
        const filterValue = filterKey ? item[filterKey] : item;
        if (typeof filterValue === 'string') {
          return filterValue.toLowerCase().includes(filter.toLowerCase());
        }
        return false;
      });
    }

    const lastPage = Math.floor(filteredData.length / pageSize);
    const start = (page - 1) * pageSize;
    const end = start + pageSize;
    filteredData = filteredData.slice(start, end);
    // add meta data previously from lazyPaginatedQuery since components expect it
    Object.defineProperty(filteredData, 'meta', {
      value: {
        currentPage: page,
        lastPage,
        nextPage: page + 1,
        prevPage: page - 1,
        total: data.length,
        filteredTotal: filteredData.length,
        pageSize,
      },
    });
  }

  return filteredData;
};


Review comment (suggestion):
Wouldn't we want to use `Math.ceil` here? Since if there was a remainder, we want that to go to the following page? I see you have a test below covering this case, so I'm probably just missing something 🤔 


Code after:
  filter?: string;
  filterKey?: string;
};

/**
 * Util to paginate data set based on page number and size
 * If filter is provided, it will filter the data prior to paginating
 */

export const paginate = (data: PaginatedData, options: PaginateOptions = {}) => {
  const { page = 1, pageSize = DEFAULT_PAGE_SIZE, filter, filterKey } = options;
  let filteredData = data;

  if (Array.isArray(data)) {
    // filter data before paginating if filter is provided
    if (filter) {
      filteredData = data.filter((item) => {
        const filterValue = filterKey ? item[filterKey] : item;
        if (typeof filterValue === 'string') {
          return filterValue.toLowerCase().includes(filter.toLowerCase());
        }
        return false;
      });
    }

    const lastPage = Math.ceil(filteredData.length / pageSize);
    const start = (page - 1) * pageSize;
    const end = start + pageSize;
    filteredData = filteredData.slice(start, end);
    // add meta data previously from lazyPaginatedQuery since components expect it
    Object.defineProperty(filteredData, 'meta', {
      value: {
        currentPage: page,
        lastPage,
        nextPage: page + 1,
        prevPage: page - 1,
        total: data.length,
        filteredTotal: filteredData.length,
        pageSize,
      },
    });
  }

  return filteredData;
};
Repository: hashicorp/vault
File: ui/ember-cli-build.js (JavaScript)

Code before:
  'ember-test-selectors': {
    strip: isProd,
  },
  'ember-composable-helpers': {
    except: ['array'],
  },
  'ember-cli-deprecation-workflow': {
    enabled: true,
  },
};

module.exports = function (defaults) {
  const app = new EmberApp(defaults, appConfig);

  app.import('node_modules/jsonlint/lib/jsonlint.js');
  app.import('node_modules/codemirror/addon/lint/lint.css');
  app.import('node_modules/codemirror/lib/codemirror.css');
  app.import('node_modules/text-encoder-lite/text-encoder-lite.js');
  app.import('node_modules/jsondiffpatch/dist/jsondiffpatch.umd.js');
  app.import('node_modules/jsondiffpatch/dist/formatters-styles/html.css');

  app.import('app/styles/bulma/bulma-radio-checkbox.css');
  app.import(
    'node_modules/@hashicorp/design-system-components/dist/styles/@hashicorp/design-system-components.css'
  );

  return app.toTree();
};


Review comment (suggestion):
Let's chat about this. I suspect there is no work-around, but I'd like to understand why we can't use HDS css.

Code after:
  },
  'ember-test-selectors': {
    strip: isProd,
  },
  'ember-composable-helpers': {
    except: ['array'],
  },
  'ember-cli-deprecation-workflow': {
    enabled: true,
  },
};

module.exports = function (defaults) {
  const app = new EmberApp(defaults, appConfig);

  app.import('node_modules/jsonlint/lib/jsonlint.js');
  app.import('node_modules/codemirror/addon/lint/lint.css');
  app.import('node_modules/codemirror/lib/codemirror.css');
  app.import('node_modules/text-encoder-lite/text-encoder-lite.js');
  app.import('node_modules/jsondiffpatch/dist/jsondiffpatch.umd.js');
  app.import('node_modules/jsondiffpatch/dist/formatters-styles/html.css');

  app.import('app/styles/bulma/bulma-radio-checkbox.css');
  app.import(
    'node_modules/@hashicorp/design-system-components/dist/styles/@hashicorp/design-system-components.css'
  );

  return app.toTree();
};
Repository: hashicorp/vault
File: ui/lib/core/addon/components/enable-input.ts (TypeScript)

Code before:
    label?: string;
    helpText?: string;
    subText?: string;
    possibleValues?: string[];
  };
}

/**
 * @module EnableInput
 * EnableInput components render a disabled input with a hardcoded masked value beside an "Edit" button to "enable" the input.
 * Clicking "Edit" hides the disabled input and renders the yielded component. This way any data management is handled by the parent.
 * These are useful for editing inputs of sensitive values not returned by the API. The extra click ensures the user is intentionally editing the field.
 *
 * @example
 <EnableInput class="field" @attr={{attr}}>
  <FormField @attr={{attr}} @model={{@destination}} @modelValidations={{this.modelValidations}} />
 </EnableInput>

// without passing @attr
 <EnableInput @label="AWS password">
  <Input @type="text" />
 </EnableInput>

 * @param {object} [attr] - used to generate label for `ReadonlyFormField`, `name` key is required. Can be an attribute from a model exported with expandAttributeMeta.
 * @param {string} [label] - required if no attr passed. Used to ensure a11y conformance for the readonly input.
 * @param {boolean} [disabled] - to be used in specific scenarios where a user can visually see but not interact with the input field. ie. disabling a field on edit
 */

export default class EnableInputComponent extends Component<Args> {
  @tracked enable = false;
}


Review comment (suggestion):
To indicate it's false by default in the docs you can do something like 
```suggestion
 * @param {boolean} [disabled=false] - to be used in specific scenarios where a user can visually see but not interact with the input field. ie. disabling a field on edit
```

Code after:
    label?: string;
    helpText?: string;
    subText?: string;
    possibleValues?: string[];
  };
}

/**
 * @module EnableInput
 * EnableInput components render a disabled input with a hardcoded masked value beside an "Edit" button to "enable" the input.
 * Clicking "Edit" hides the disabled input and renders the yielded component. This way any data management is handled by the parent.
 * These are useful for editing inputs of sensitive values not returned by the API. The extra click ensures the user is intentionally editing the field.
 *
 * @example
 <EnableInput class="field" @attr={{attr}}>
  <FormField @attr={{attr}} @model={{@destination}} @modelValidations={{this.modelValidations}} />
 </EnableInput>

// without passing @attr
 <EnableInput @label="AWS password">
  <Input @type="text" />
 </EnableInput>

 * @param {object} [attr] - used to generate label for `ReadonlyFormField`, `name` key is required. Can be an attribute from a model exported with expandAttributeMeta.
 * @param {string} [label] - required if no attr passed. Used to ensure a11y conformance for the readonly input.
 * @param {boolean} [disabled=false] - to be used in specific scenarios where a user can visually see but not interact with the input field. ie. disabling a field on edit
 */

export default class EnableInputComponent extends Component<Args> {
  @tracked enable = false;
}