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 group.Update() when users list is populated #449

Merged
merged 7 commits into from
Mar 21, 2022

Conversation

adambarreiro
Copy link
Collaborator

@adambarreiro adambarreiro commented Mar 18, 2022

Description

Let's consider that we're working with LDAP users and groups. When a group is read from LDAP, and it contains users, the attribute UsersList is populated with the names of these users. The problem is, if the group is updated, and the users list is sent back, VCD will throw this error:

HTTP 400 Bad Request: Invalid content was found starting with element 'UsersList'. No child element is expected at this point

The proposed fix is to send this attribute always to nil in the group.Update() function.

I've also checked what happens with user.Update(), as common sense would suggest that the behaviour is the same, but it is not. In fact, there's already a check kind of similar to the one proposed, in user.go:414.

In this case, user.Update() worked out of the box, so I haven't touched that part. What I did, in the other hand, is enrich the group test with an user.Update() call, to have this behaviour covered as well.

Signed-off-by: abarreiro <abarreiro@vmware.com>
abarreiro added 2 commits March 18, 2022 14:59
Signed-off-by: abarreiro <abarreiro@vmware.com>
Signed-off-by: abarreiro <abarreiro@vmware.com>
@adambarreiro adambarreiro requested a review from mikeletux March 18, 2022 14:03
@adambarreiro adambarreiro marked this pull request as ready for review March 18, 2022 14:04
@adambarreiro adambarreiro removed the request for review from dataclouder March 18, 2022 14:05
@Didainius
Copy link
Collaborator

Also add a note to changelog.

@adambarreiro adambarreiro self-assigned this Mar 21, 2022
abarreiro added 3 commits March 21, 2022 10:05
Signed-off-by: abarreiro <abarreiro@vmware.com>
Signed-off-by: abarreiro <abarreiro@vmware.com>
#
Signed-off-by: abarreiro <abarreiro@vmware.com>
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

p.s. Found during one of the test-fests.

Signed-off-by: abarreiro <abarreiro@vmware.com>
@adambarreiro
Copy link
Collaborator Author

Also add a note to changelog.

Done, thanks!

Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

Thanks. LDAP related tests pass.

Copy link
Contributor

@mikeletux mikeletux left a comment

Choose a reason for hiding this comment

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

LGTM! Approved 😄

@adambarreiro adambarreiro merged commit 24fc146 into vmware:main Mar 21, 2022
@adambarreiro adambarreiro deleted the fix-user-list-update branch March 21, 2022 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants