mirror of
https://github.com/Telmate/proxmox-api-go.git
synced 2026-04-25 23:45:55 +03:00
[GH-ISSUE #236] user not found even when it exists #49
Labels
No labels
good first issue
issue/confirmed
issue/critical
proposal/accepted
pull-request
type/bug
type/enhancement
type/feature
type/question
type/refactoring
type/testing
type/testing
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/proxmox-api-go#49
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @tnyeanderson on GitHub (Feb 12, 2023).
Original GitHub issue: https://github.com/Telmate/proxmox-api-go/issues/236
A bug was introduced in
daf13c586aby using the newlistUsersPartial()function inCheckUserExistence(), instead oflistUsersFull()which is the renamed version of the originallistUsers()function.This causes the error
user does not exist or has insufficient permissions on proxmox: <user>@mleone87 commented on GitHub (Feb 12, 2023):
can you confirm that was not intended by you @Tinyblargon ? Didn't want to make back and forth with code changes
@Tinyblargon commented on GitHub (Feb 12, 2023):
@mleone87
The intention was to reduce the amount of data retrieved.
As
listUsersFull()includes the group and token information of all users.And
listUsersPartial()does not include this group and token information.The
CheckUserExistence()does not need to know the groups a member is part of or the current session tokens a user has. Thats why i changed it tolistUsersPartial().Proxmox documentation on listing user information
I ran the Integrations tests in
test/cliagainst a PVE 7.3.6 and the user tests work. (can make a mess when tests fail. run against a test PVE)I manual tested the
CheckUserExistence()on my and it works as expected.ran the following command in the old CLI
-debug -insecure testUserPermissions root@pamIt returned a list of permissions
ran the following commands in the new CLI
-d -i list usersretruns the output oflistUsersPartial()Result:
-d -i list users --groupsretruns the output oflistUsersFull()Result:
All test where performed with
root@pamuserAlso noticed that when the user has insufficient rights the list of users will only include themselves.
@tnyeanderson
Which PVE version are you using?
What permissions does the user have you use to execute the command? is it root@pam?
Can you switch to the new CLI and run the following commands?
-d -i list usersretruns the output oflistUsersPartial()-d -i list users --groupsretruns the output oflistUsersFull()Their output except for the group memberships should be the same
You can enable the new CLI with
@tnyeanderson commented on GitHub (Feb 12, 2023):
Proxmox version 7.3-4
I am doing it (via terraform) with a user created specifically for this purpose in the way laid out in the terraform provider documentation.
The user has all the permissions listed here, evidenced by the fact that the user existence/permission check succeeds when
listUsersFull()is used, and why #237 fixes this issue.I think you've described why it doesn't work yourself:
The CLI tests should probably be performed as a non-root, "token-ized" user as otherwise it seems that misleading results can occur.
If you still need the info above, I can probably get it for you later tonight. Just sending this quickly to get the gears turning :) thanks!
@Tinyblargon commented on GitHub (Feb 12, 2023):
proxmox test
Created user test@pve and gave it the following permissions on
/:Running
-debug -insecure testUserPermissions test@pve /returned the following outputterraform test
Also made a local build of the Terraform provider using
github.com/Telmate/proxmox-api-go@1b03d8c3c7andgithub.com/Telmate/terraform-provider-proxmox@5211ef3a71and tried the following:@tnyeanderson
I'm unable to recreate this issue.
If you could run
-d -i list usersin the new CLI.And post the output of
-debug -insecure testUserPermissions usernam@pam /with usernam@pam replaced by your userid.That would be very helpful.
@tnyeanderson commented on GitHub (Feb 12, 2023):
Here is the output:
Your test does not capture the use of tokens as I described.
I bisected the issue to the commit mentioned earlier and just verified the same (with
github.com/Telmate/terraform-provider-proxmox@5211ef3a71), and going fromb41fb5743todaf13c586abreaks it.Using the CLI is not an accurate test for this, namely because the CLI calls
ListUserswhich transforms thelistUsersPartialorlistUsersFulloutput with a call toConfigUser{}.mapToArray(userList)before returning it, where theCheckUserExistencefunction uses thelistUsersPartial/Fulloutput directly. And there is a difference! The lack oftokensanduseridfields!After this bit of digging, I found the real issue. When
CheckUserExistencesends the[]interface{}result oflistUsersPartialtoItemInKeyOfArray(list, "userid", userId.ToString()),userId.ToString()resolves toterraform-prov@pve!tftoken(I assume with your non-token test, it is justtest@pvewithout a!tokenidsuffix). In your case, this is caught by the first conditional, so the user gets found. However, in my case, the second conditional is used, where it checks for the existence of thetokensmap for each user. Per your initial response, thistokensmap is not included withlistUsersPartialbut it is included withlistUsersFull.This leads to the discussion of what should be done about this. There are a few problems with the current code:
ItemInKeyOfArrayis doing too much (it isn't just checking for the specified key, but also for a "token" member with specific structure), and therefore isn't doing what it says. This function could be refactored into a generic check function (doing what the current function name implies), and a separate function for checking the token stuff. Then in cases where both are needed they can be called in sequence or another wrapper function could call both. It is used 5 times in the code:CheckUserExistencefunction (used three times in the code) could be refactored to either:a. Check only for existence of the user, not the token, by using
fmt.Sprintf("%s@%s", userId.Name, userId.Realm)in place ofuserId.ToString()in the call toItemInKeyOfArray. Optionally create aCheckTokenExistencefunction that relies onlistUsersFullif needed.b. Continue as-is but require the use of
listUsersFullinCheckUserExistenceI think there is a difference between checking if a user exists or whether a token exists, and the function names should do what they imply. Therefore I am in favor of the refactor in both cases.
Additionally, the extensive use of under-defined
interface{}types throughout makes it difficult. I am a little confused why something like the API response for listing users doesn't have a well-defined data type which we unmarshal from the JSON, which would help when debugging these sorts of issues. It took me quite a while just to get an inkling of a grasp on how the response was being parsed/processed.Let me know what you both think. And thanks for the hard work on this project!
@Tinyblargon commented on GitHub (Feb 13, 2023):
@tnyeanderson
Thank you for your clear and extensive explanation.
I was unaware that
ItemInKeyOfArraychecked for the token, i should have checked that before i made the change. But it explains the miscommunication in this tread.As for a solution.
I think its best to quickly fix it by changing
listUsersPartialtolistUsersFullinCheckUserExistence, but put aTODOinCheckUserExistencelinking to this thread.I agree with you that a refactor is needed, checking for the token should be removed from
CheckUserExistenceand either be put in its own fuction or become a part ofClient.GetUserPermissions.The
userId.ToString()fuction should be kept as this project should provide both ways to convert betweenuser@realmanduserId{}When i wrote
ItemInKeyOfArrayit was intended to check if a interface key exists in amap[string]interface{}and its value matched the given value. This was done as converting the all users into a struct would have been quite performance intensive.@tnyeanderson commented on GitHub (Feb 13, 2023):
If I'm understanding you correctly... I think your fears are unfounded, and I created a benchmark to demonstrate it. See the full writeup here.
tldr: safe to say, unmarshaling the JSON into a struct has practically zero performance impact compared to unmarshaling into an interface, even with a rather large dataset:
That being the case, I think creating actual types for the API responses and config resources will make this project much more maintainable in the long run and might even decrease the size of the codebase quite a bit. It's also just the "right" way to do things :)
However, in terms of this issue, I've added the TODO entry to the PR so we should be good to merge and close and figure out the rest of the plan later.
Thanks again everyone!