[GH-ISSUE #236] user not found even when it exists #49

Closed
opened 2026-02-28 00:40:21 +03:00 by kerem · 7 comments
Owner

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 daf13c586a by using the new listUsersPartial() function in CheckUserExistence(), instead of listUsersFull() which is the renamed version of the original listUsers() function.

This causes the error user does not exist or has insufficient permissions on proxmox: <user>

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 daf13c586aac9d95cacd1a3a96018de2f5d0761c by using the new `listUsersPartial()` function in `CheckUserExistence()`, instead of `listUsersFull()` which is the renamed version of the original `listUsers()` function. This causes the error `user does not exist or has insufficient permissions on proxmox: <user>`
kerem closed this issue 2026-02-28 00:40:21 +03:00
Author
Owner

@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

<!-- gh-comment-id:1426994538 --> @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
Author
Owner

@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 to listUsersPartial().
Proxmox documentation on listing user information

I ran the Integrations tests in test/cli against 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@pam
It returned a list of permissions

ran the following commands in the new CLI
-d -i list users retruns the output of listUsersPartial()
Result:

[
{"user":{"name":"bla","realm":"pve"},"enable":true,"expire":0},
{"user":{"name":"root","realm":"pam"},"email":"admin@proxmox.com","enable":true,"expire":0},
{"user":{"name":"test","realm":"pve"},"enable":true,"expire":0}
]

-d -i list users --groups retruns the output of listUsersFull()
Result:

[
{"user":{"name":"bla","realm":"pve"},"enable":true,"expire":0,"groups":[]},
{"user":{"name":"root","realm":"pam"},"email":"admin@proxmox.com","enable":true,"expire":0,"groups":[]},
{"user":{"name":"test","realm":"pve"},"enable":true,"expire":0,"groups":["bla","test"]}
]

All test where performed with root@pam user
Also 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 users retruns the output of listUsersPartial()
-d -i list users --groups retruns the output of listUsersFull()
Their output except for the group memberships should be the same

You can enable the new CLI with

export NEW_CLI=true
<!-- gh-comment-id:1427008201 --> @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 to `listUsersPartial()`. [Proxmox documentation on listing user information](https://pve.proxmox.com/pve-docs/api-viewer/#/access/users) I ran the Integrations tests in `test/cli` against 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@pam` It returned a list of permissions ran the following commands in the new CLI `-d -i list users` retruns the output of `listUsersPartial()` Result: ```json [ {"user":{"name":"bla","realm":"pve"},"enable":true,"expire":0}, {"user":{"name":"root","realm":"pam"},"email":"admin@proxmox.com","enable":true,"expire":0}, {"user":{"name":"test","realm":"pve"},"enable":true,"expire":0} ] ``` `-d -i list users --groups` retruns the output of `listUsersFull()` Result: ```json [ {"user":{"name":"bla","realm":"pve"},"enable":true,"expire":0,"groups":[]}, {"user":{"name":"root","realm":"pam"},"email":"admin@proxmox.com","enable":true,"expire":0,"groups":[]}, {"user":{"name":"test","realm":"pve"},"enable":true,"expire":0,"groups":["bla","test"]} ] ``` All test where performed with `root@pam` user Also 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 users` retruns the output of `listUsersPartial()` `-d -i list users --groups` retruns the output of `listUsersFull()` Their output except for the group memberships should be the same You can enable the new CLI with ```bash export NEW_CLI=true ```
Author
Owner

@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:

All test where performed with root@pam user

And listUsersPartial() does not include this group and token information.

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!

<!-- gh-comment-id:1427083987 --> @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](https://github.com/Telmate/terraform-provider-proxmox/blob/master/docs/index.md#creating-the-proxmox-user-and-role-for-terraform). The user has all the permissions listed [here](https://github.com/Telmate/terraform-provider-proxmox/blob/9dbcaa018f72d901c65cb126364b97f9b810a50c/proxmox/provider.go#L190-L211), 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: >All test where performed with root@pam user >And listUsersPartial() does not include this group and token information. 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!
Author
Owner

@Tinyblargon commented on GitHub (Feb 12, 2023):

proxmox test

Created user test@pve and gave it the following permissions on /:

Datastore.AllocateSpace
Datastore.Audit
Pool.Allocate
Sys.Audit
Sys.Console
Sys.Modify
VM.Allocate
VM.Audit
VM.Clone
VM.Config.CDROM
VM.Config.CPU
VM.Config.Cloudinit
VM.Config.Disk
VM.Config.HWType
VM.Config.Memory
VM.Config.Network
VM.Config.Options
VM.Migrate
VM.Monitor
VM.PowerMgmt

Running -debug -insecure testUserPermissions test@pve / returned the following output

[Datastore.AllocateSpace Datastore.Audit Pool.Allocate Sys.Audit Sys.Console Sys.Modify VM.Allocate VM.Audit VM.Clone VM.Config.CDROM VM.Config.CPU VM.Config.Cloudinit VM.Config.Disk VM.Config.HWType VM.Config.Memory VM.Config.Network VM.Config.Options VM.Migrate VM.Monitor VM.PowerMgmt]

terraform test

Also made a local build of the Terraform provider using github.com/Telmate/proxmox-api-go@1b03d8c3c7 and github.com/Telmate/terraform-provider-proxmox@5211ef3a71 and tried the following:

terraform {
  required_providers {
    proxmox = {
      source  = "terraform.local/local/proxmox"
      version = "1.0.0"
    }
  }
}

provider "proxmox" {
  pm_user = "test@pve"
  pm_password = "password"
  pm_api_url = "https://proxmox-ip/api2/json"
  pm_tls_insecure = "true"
}

resource "proxmox_vm_qemu" "cloudinit-test" {
    name = "terraform-test-vm"
    target_node = "pve"
    clone = "linux-cloudinit-template"
    os_type = "cloud-init"
}

@tnyeanderson
I'm unable to recreate this issue.

If you could run -d -i list users in 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.

<!-- gh-comment-id:1427124491 --> @Tinyblargon commented on GitHub (Feb 12, 2023): ## proxmox test Created user test@pve and gave it the following permissions on `/`: ``` Datastore.AllocateSpace Datastore.Audit Pool.Allocate Sys.Audit Sys.Console Sys.Modify VM.Allocate VM.Audit VM.Clone VM.Config.CDROM VM.Config.CPU VM.Config.Cloudinit VM.Config.Disk VM.Config.HWType VM.Config.Memory VM.Config.Network VM.Config.Options VM.Migrate VM.Monitor VM.PowerMgmt ``` Running `-debug -insecure testUserPermissions test@pve /` returned the following output ``` [Datastore.AllocateSpace Datastore.Audit Pool.Allocate Sys.Audit Sys.Console Sys.Modify VM.Allocate VM.Audit VM.Clone VM.Config.CDROM VM.Config.CPU VM.Config.Cloudinit VM.Config.Disk VM.Config.HWType VM.Config.Memory VM.Config.Network VM.Config.Options VM.Migrate VM.Monitor VM.PowerMgmt] ``` ## terraform test Also made a local build of the Terraform provider using https://github.com/Telmate/proxmox-api-go/commit/1b03d8c3c7a3c468ae8f19ef4c4e5341f0385218 and https://github.com/Telmate/terraform-provider-proxmox/commit/5211ef3a71b51cb1c05a5b7565dddfedbf7caf79 and tried the following: ``` terraform { required_providers { proxmox = { source = "terraform.local/local/proxmox" version = "1.0.0" } } } provider "proxmox" { pm_user = "test@pve" pm_password = "password" pm_api_url = "https://proxmox-ip/api2/json" pm_tls_insecure = "true" } resource "proxmox_vm_qemu" "cloudinit-test" { name = "terraform-test-vm" target_node = "pve" clone = "linux-cloudinit-template" os_type = "cloud-init" } ``` @tnyeanderson I'm unable to recreate this issue. If you could run `-d -i list users` in 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.
Author
Owner

@tnyeanderson commented on GitHub (Feb 12, 2023):

Here is the output:

$ export PM_API_URL=https://node1:8006/api2/json
$ export PM_API_TOKEN_SECRET=<redacted>
$ export PM_API_TOKEN_ID=terraform-prov@pve!tftoken
$ export PM_USER=$PM_API_TOKEN_ID
$ export PM_PASS=$PM_API_TOKEN_SECRET
$ go run . -d -i list users | jq
[
  {
    "user": {
      "name": "root",
      "realm": "pam"
    },
    "email": "<redacted>",
    "enable": true,
    "expire": 0
  },
  {
    "user": {
      "name": "terraform-prov",
      "realm": "pve"
    },
    "enable": true,
    "expire": 0
  }
]
$ go run . -d -i list users --groups | jq
[
  {
    "user": {
      "name": "root",
      "realm": "pam"
    },
    "email": "<redacted>",
    "enable": true,
    "expire": 0,
    "groups": []
  },
  {
    "user": {
      "name": "terraform-prov",
      "realm": "pve"
    },
    "enable": true,
    "expire": 0,
    "groups": []
  }
]

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 from b41fb5743 to daf13c586a breaks it.

Using the CLI is not an accurate test for this, namely because the CLI calls ListUsers which transforms the listUsersPartial or listUsersFull output with a call to ConfigUser{}.mapToArray(userList) before returning it, where the CheckUserExistence function uses the listUsersPartial/Full output directly. And there is a difference! The lack of tokens and userid fields!

After this bit of digging, I found the real issue. When CheckUserExistence sends the []interface{} result of listUsersPartial to ItemInKeyOfArray(list, "userid", userId.ToString()), userId.ToString() resolves to terraform-prov@pve!tftoken (I assume with your non-token test, it is just test@pve without a !tokenid suffix). 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 the tokens map for each user. Per your initial response, this tokens map is not included with listUsersPartial but it is included with listUsersFull.

This leads to the discussion of what should be done about this. There are a few problems with the current code:

  1. ItemInKeyOfArray is 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:
    git grep ItemIn
    proxmox/client.go:      existance = ItemInKeyOfArray(list["data"].([]interface{}), "plugin", id)
    proxmox/client.go:      existance = ItemInKeyOfArray(list["data"].([]interface{}), "id", id)
    proxmox/client.go:      existance = ItemInKeyOfArray(list["data"].([]interface{}), "storage", id)
    proxmox/config_group.go:        return ItemInKeyOfArray(paramArray, "groupid", string(group)), nil
    proxmox/config_user.go: existence = ItemInKeyOfArray(list, "userid", userId.ToString())
    proxmox/util.go:func ItemInKeyOfArray(array []interface{}, key, value string) (existance bool) {
    
  2. The CheckUserExistence function (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 of userId.ToString() in the call to ItemInKeyOfArray. Optionally create a CheckTokenExistence function that relies on listUsersFull if needed.
    b. Continue as-is but require the use of listUsersFull in CheckUserExistence

I 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!

<!-- gh-comment-id:1427136926 --> @tnyeanderson commented on GitHub (Feb 12, 2023): Here is the output: ``` $ export PM_API_URL=https://node1:8006/api2/json $ export PM_API_TOKEN_SECRET=<redacted> $ export PM_API_TOKEN_ID=terraform-prov@pve!tftoken $ export PM_USER=$PM_API_TOKEN_ID $ export PM_PASS=$PM_API_TOKEN_SECRET $ go run . -d -i list users | jq [ { "user": { "name": "root", "realm": "pam" }, "email": "<redacted>", "enable": true, "expire": 0 }, { "user": { "name": "terraform-prov", "realm": "pve" }, "enable": true, "expire": 0 } ] $ go run . -d -i list users --groups | jq [ { "user": { "name": "root", "realm": "pam" }, "email": "<redacted>", "enable": true, "expire": 0, "groups": [] }, { "user": { "name": "terraform-prov", "realm": "pve" }, "enable": true, "expire": 0, "groups": [] } ] ``` 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 https://github.com/Telmate/terraform-provider-proxmox/commit/5211ef3a71b51cb1c05a5b7565dddfedbf7caf79), and going from b41fb5743 to daf13c586a breaks it. Using the CLI is not an accurate test for this, namely because the CLI calls `ListUsers` which transforms the `listUsersPartial` or `listUsersFull` output with a call to `ConfigUser{}.mapToArray(userList)` before returning it, where the `CheckUserExistence` function uses the `listUsersPartial/Full` output directly. And there is a difference! The lack of `tokens` and `userid` fields! After this bit of digging, I found the real issue. When `CheckUserExistence` sends the `[]interface{}` result of `listUsersPartial` to `ItemInKeyOfArray(list, "userid", userId.ToString())`, `userId.ToString()` resolves to `terraform-prov@pve!tftoken` (I assume with your non-token test, it is just `test@pve` without a `!tokenid` suffix). In your case, this is caught by the [first conditional](https://github.com/Telmate/proxmox-api-go/blob/1b03d8c3c7a3c468ae8f19ef4c4e5341f0385218/proxmox/util.go#L32-L37), so the user gets found. However, in my case, the [second conditional](https://github.com/Telmate/proxmox-api-go/blob/1b03d8c3c7a3c468ae8f19ef4c4e5341f0385218/proxmox/util.go#L38-L49) is used, where it checks for the existence of the `tokens` map for each user. Per your initial response, this `tokens` map *is not included with `listUsersPartial` but it is included with `listUsersFull`*. This leads to the discussion of what should be done about this. There are a few problems with the current code: 1. `ItemInKeyOfArray` is 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: ``` git grep ItemIn proxmox/client.go: existance = ItemInKeyOfArray(list["data"].([]interface{}), "plugin", id) proxmox/client.go: existance = ItemInKeyOfArray(list["data"].([]interface{}), "id", id) proxmox/client.go: existance = ItemInKeyOfArray(list["data"].([]interface{}), "storage", id) proxmox/config_group.go: return ItemInKeyOfArray(paramArray, "groupid", string(group)), nil proxmox/config_user.go: existence = ItemInKeyOfArray(list, "userid", userId.ToString()) proxmox/util.go:func ItemInKeyOfArray(array []interface{}, key, value string) (existance bool) { ``` 2. The `CheckUserExistence` function (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 of `userId.ToString()` in the call to `ItemInKeyOfArray`. Optionally create a `CheckTokenExistence` function that relies on `listUsersFull` if needed. b. Continue as-is but require the use of `listUsersFull` in `CheckUserExistence` I 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!
Author
Owner

@Tinyblargon commented on GitHub (Feb 13, 2023):

@tnyeanderson
Thank you for your clear and extensive explanation.
I was unaware that ItemInKeyOfArray checked 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 listUsersPartial to listUsersFull in CheckUserExistence, but put a TODO in CheckUserExistence linking to this thread.
I agree with you that a refactor is needed, checking for the token should be removed from CheckUserExistence and either be put in its own fuction or become a part of Client.GetUserPermissions.

The userId.ToString() fuction should be kept as this project should provide both ways to convert between user@realm and userId{}

When i wrote ItemInKeyOfArray it was intended to check if a interface key exists in a map[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.

<!-- gh-comment-id:1428022035 --> @Tinyblargon commented on GitHub (Feb 13, 2023): @tnyeanderson Thank you for your clear and extensive explanation. I was unaware that `ItemInKeyOfArray` checked 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 `listUsersPartial` to `listUsersFull` in `CheckUserExistence`, but put a `TODO` in `CheckUserExistence` linking to this thread. I agree with you that a refactor is needed, checking for the token should be removed from `CheckUserExistence` and either be put in its own fuction or become a part of `Client.GetUserPermissions`. The `userId.ToString()` fuction should be kept as this project should provide both ways to convert between `user@realm` and `userId{}` When i wrote `ItemInKeyOfArray` it was intended to check if a interface key exists in a `map[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.
Author
Owner

@tnyeanderson commented on GitHub (Feb 13, 2023):

This was done as converting the all users into a struct would have been quite performance intensive.

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:

BenchmarkUnmarshalStruct-12             1000000000           0.3961 ns/op
BenchmarkUnmarshalInterface-12          1000000000           0.3914 ns/op

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!

<!-- gh-comment-id:1428832647 --> @tnyeanderson commented on GitHub (Feb 13, 2023): >This was done as converting the all users into a struct would have been quite performance intensive. 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](https://github.com/tnyeanderson/zet/tree/main/20230213174906). 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: ``` BenchmarkUnmarshalStruct-12 1000000000 0.3961 ns/op BenchmarkUnmarshalInterface-12 1000000000 0.3914 ns/op ``` 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!
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
starred/proxmox-api-go#49
No description provided.