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

Fix bug where updates to config would fail is password isn't provided #91

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

jasonodonnell
Copy link
Contributor

If a user tries to update the config and doesn't provide the binddn or bindpass, it will fail with an error: cannot derive UserBindDN. This PR adds a read operation to update to first check storage for a config and pass it into the config builder. I've added a test to cover this, which fails without the patch:

$ go test -v -run TestBackend
=== RUN   TestBackend
=== RUN   TestBackend/write_config
=== RUN   TestBackend/read_config
=== RUN   TestBackend/delete_config
=== RUN   TestBackend/update_config
    backend_test.go:119: cannot derive UserBindDN
=== RUN   TestBackend/plant_config
=== RUN   TestBackend/write_role
=== RUN   TestBackend/read_role
=== RUN   TestBackend/list_roles
=== RUN   TestBackend/delete_role
=== RUN   TestBackend/plant_role
=== RUN   TestBackend/read_cred
=== RUN   TestBackend/rotate_role_creds
=== RUN   TestBackend/rollback_role_creds
=== RUN   TestBackend/discard_WAL
=== RUN   TestBackend/rotate_root_creds
=== RUN   TestBackend/rotate_root_creds_with_write
--- FAIL: TestBackend (0.01s)
    --- PASS: TestBackend/write_config (0.00s)
    --- PASS: TestBackend/read_config (0.00s)
    --- PASS: TestBackend/delete_config (0.00s)
    --- FAIL: TestBackend/update_config (0.00s)
    --- PASS: TestBackend/plant_config (0.00s)
    --- PASS: TestBackend/write_role (0.00s)
    --- PASS: TestBackend/read_role (0.00s)
    --- PASS: TestBackend/list_roles (0.00s)
    --- PASS: TestBackend/delete_role (0.00s)
    --- PASS: TestBackend/plant_role (0.00s)
    --- PASS: TestBackend/read_cred (0.00s)
    --- PASS: TestBackend/rotate_role_creds (0.00s)
    --- PASS: TestBackend/rollback_role_creds (0.00s)
    --- PASS: TestBackend/discard_WAL (0.00s)
    --- PASS: TestBackend/rotate_root_creds (0.00s)
    --- PASS: TestBackend/rotate_root_creds_with_write (0.00s)
FAIL
exit status 1
FAIL	github.com/hashicorp/vault-plugin-secrets-ad/plugin	1.123s

@jasonodonnell jasonodonnell requested a review from a team December 1, 2022 19:33
Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jasonodonnell jasonodonnell merged commit 20ddb69 into main Dec 1, 2022
@jasonodonnell jasonodonnell deleted the config-bug branch December 1, 2022 19:42
jasonodonnell added a commit that referenced this pull request Dec 1, 2022
…#91)

* Fix bug where updates to config would fail is password isn't provided

* Make test clearer
jasonodonnell added a commit that referenced this pull request Dec 1, 2022
…#91)

* Fix bug where updates to config would fail is password isn't provided

* Make test clearer
jasonodonnell added a commit that referenced this pull request Dec 1, 2022
…#91)

* Fix bug where updates to config would fail is password isn't provided

* Make test clearer
jasonodonnell added a commit that referenced this pull request Dec 1, 2022
…#91) (#92)

* Fix bug where updates to config would fail is password isn't provided

* Make test clearer
jasonodonnell added a commit that referenced this pull request Dec 1, 2022
… isn't provided (#94)

* Fix bug where updates to config would fail is password isn't provided (#91)

* Fix bug where updates to config would fail is password isn't provided

* Make test clearer

* Fix test
jasonodonnell added a commit that referenced this pull request Dec 1, 2022
…#91) (#93)

* Fix bug where updates to config would fail is password isn't provided

* Make test clearer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants