Updates to groups without ID are not consistent.

[bruj0]

When using the entity/group endpoint and NOT providing the ID as required the entity groups are not updated.

id (string: <optional>) - ID of the group. If set, updates the corresponding existing group.

https://www.vaultproject.io/api-docs/secret/identity/group#create-a-group

The bug happens when we don't provide an ID but we include the name of an already existing group which results in an inconsistent update.

Originally reported by @luke-clifton

Reproduction, in this case, the group has 3 entities and we remove 1.
The entity still shows the group ID when reading the properties but the group no longer shows the entity ID.

+ vault read identity/group/name/cdaa75b0-0cc8-4902-bfe4-5a3878013528 -format=json
+ jq .data.member_entity_ids
[
  "0ebdf3f1-6067-36ed-3bb1-5a087395a0ff",
  "28b2bf74-db92-c004-53bf-9140851e1784",
  "7ff2f3b2-3b2c-82a0-b6fa-046025111339"
]
+ vault write identity/group name=cdaa75b0-0cc8-4902-bfe4-5a3878013528 member_entity_ids=28b2bf74-db92-c004-53bf-9140851e1784,0ebdf3f1-6067-36ed-3bb1-5a087395a0ff
Key     Value
---     -----
id      57cde975-abf9-5ad9-39bd-e32f613acd20
name    cdaa75b0-0cc8-4902-bfe4-5a3878013528
+ vault read identity/entity/id/7ff2f3b2-3b2c-82a0-b6fa-046025111339 -format=json
+ jq .data.group_ids
[
  "57cde975-abf9-5ad9-39bd-e32f613acd20"
]
+ vault read identity/group/name/cdaa75b0-0cc8-4902-bfe4-5a3878013528 -format=json
+ jq .data.member_entity_ids
[
  "0ebdf3f1-6067-36ed-3bb1-5a087395a0ff",
  "28b2bf74-db92-c004-53bf-9140851e1784"
]

This PR is a draft until I add unit testing.

[luke-clifton]

Using the non-cloned groupByName result will always lead to the bug. You need to either clone the groupByName as I did in #10582 or you need to fail on that case.

The case you added essentially makes the group.ID == "" case dead code which solves the bug as well because the only way group.ID == "" can currently occur is when it is a newGroup. However that case should be removed entirely.

[bruj0]

For me the bug is not that the update is not happening but that it should return an error, the documentation says it needs an ID to do the update.

[luke-clifton]

Yep, agreed, I listed this as an alternate in my PR. But the code path for group.ID == "" is wrong and should be deleted (or return an error). The case you added will always trigger instead of group.ID == "", so that case is now dead code. It should not be left in. If, in the future, it was triggered again somehow, then this bug would re-appear.

Setting group = groupByName is always wrong, and if it's not wrong, it should be cloned.

[luke-clifton]

Is there any progress on this? It does allow someone who has access to this API to give themselves permanent, un-audited access to that group.

[aphorise]

The conditions in the code: vault/identity_store_groups.go remain the same as when this issue was raised - the last proposed solution also doesnt seem to robust enough?

Hey @luke-clifton are you able to draft a PR with what you think is still pending? - I'm not sure I understand fully the cloning or any comparative groupByName / ID that you're after (as you'd noted above).

[heatherezell]

Closing due to age and draft status