Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VaultEncryptionHelper's validation exception message should include all validation problems #783

Closed
sleberknight opened this issue Sep 15, 2022 · 0 comments · Fixed by #808
Assignees
Labels
enhancement A request for change or improvement to an existing feature
Milestone

Comments

@sleberknight
Copy link
Member

The ValidationEncryptionHelper constructor validates its arguments. When it validates the VaultConfiguration it throws an IllegalArgumentException for the first problem it finds. So, if there is more than one problem, the calling code won't know that and will only know about the first problem.

It would be much better to validate the entire object and provide a message containing all the validation errors in the IllegalArgumentException's message.

VaultConfiguration uses the @NotBlank Java/Jakarta Beans Validation annotation on its properties, though it does not validate that those paths actually exist. However, ValidationEncryptionHelper does not use Beans Validation to validate, but checks the properties itself. IIRC the Beans Validation annotations were mainly to allow the VaultConfiguration to be validated at (Dropwizard) service startup, since it would be part of the service configuration class. The more involved validation inside ValidationEncryptionHelper also verifies that the specified files (e.g. the ansible-vault executable and the vault password file) actually exist.

Regardless of how we choose to implement the validation in ValidationEncryptionHelper, the IllegalArgumentException should contain all the problems.

@sleberknight sleberknight added the enhancement A request for change or improvement to an existing feature label Sep 15, 2022
@sleberknight sleberknight added this to the 2.4.0 milestone Sep 15, 2022
sleberknight added a commit that referenced this issue Nov 15, 2022
Change the constructor of VaultEncryptionHelper to report
all problems with the VaultConfiguration, not just the first
one it finds. These are reported in the message of the
IllegalArgumentException as a comma-separated list.

Closes #783
@sleberknight sleberknight self-assigned this Nov 15, 2022
chrisrohr pushed a commit that referenced this issue Nov 15, 2022
Change the constructor of VaultEncryptionHelper to report
all problems with the VaultConfiguration, not just the first
one it finds. These are reported in the message of the
IllegalArgumentException as a comma-separated list.

Closes #783
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A request for change or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant