ECOPROJECT-4271 | fix: implement permissions check for vsphere root level read#183
ECOPROJECT-4271 | fix: implement permissions check for vsphere root level read#183itroyano wants to merge 1 commit intokubev2v:mainfrom
Conversation
| return nil | ||
| } | ||
|
|
||
| func (c *VSphereCollector) checkReadOnlyPermissions(ctx context.Context, client *govmomi.Client, username string) error { |
There was a problem hiding this comment.
It looks like the function actually check if the user has (amongst possible other priviliges) a read permission, right?
In that case the name is misleading, maybe a better name would be "hasReadPermissions"?
| return fmt.Errorf("failed to check read-only permissions on vCenter root level: %w", err) | ||
| } | ||
|
|
||
| if len(hasPrivileges) == 0 || !hasPrivileges[0] { |
There was a problem hiding this comment.
This is a nitpick :-) feel free to ignore.
I'm not familiar with this library, so I don't know if this is the idiomatic way to test this - but assiging meaning to an index of a slice might be brittle. Should someone later try to expand the function by adding more "priviliges" to the authMgr.HasPrivilegeOnEntity call.
I would consider adding a comment above either the authMgr.HasPrivilegeOnEntity call, or asserting that the length of the result slice is no bigger than 1.
There was a problem hiding this comment.
Hi @amalimov
Apologies I haven't provided proper context here.
The issue in title happens because the user creds are correct AND have read permissions, but the read permissions do not extend to the vCenter root level.
What ends up happening is the parser func (called after verifyCreds) gets a null "cluster" column and throws the error in the ticket.
I'll add the necessary changes.
There was a problem hiding this comment.
adding the description to the PR now
|
/lgtm |
|
looks good, just added two minor suggestions (feel free to ignore) |
|
Just a thought here: |
| return nil | ||
| } | ||
|
|
||
| func (c *VSphereCollector) checkReadOnlyPermissions(ctx context.Context, client *govmomi.Client, username string) error { |
There was a problem hiding this comment.
can you reuse that helper function ?
https://github.com/kubev2v/assisted-migration-agent/blob/main/pkg/vmware/auth.go#L15
| } | ||
|
|
||
| func (c *VSphereCollector) checkReadOnlyPermissions(ctx context.Context, client *govmomi.Client, username string) error { | ||
| if client == nil { |
| authMgr := object.NewAuthorizationManager(client.Client) | ||
|
|
||
| // Check if user has basic read privileges on root folder | ||
| hasPrivileges, err := authMgr.HasPrivilegeOnEntity(ctx, rootFolder, username, []string{ |
There was a problem hiding this comment.
nitpick: you could extract rootFolder directly here..
itroyano
force-pushed
the
verifyReadPermissions
branch
2 times, most recently
from
18d4b51 to
aa7f318
Compare
|
Code doesn't work... it reached logging "vCenter read-only permissions verified" with my known-bad user, and I got the bug error again Expected result was "user is missing required privileges" |
itroyano
force-pushed
the
verifyReadPermissions
branch
2 times, most recently
from
be0a3c2 to
c76ad07
Compare
Signed-off-by: Igor Troyanovsky <itroyano@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
itroyano
force-pushed
the
verifyReadPermissions
branch
from
c76ad07 to
767921b
Compare
4 participants
The issue in title happens because the user creds are correct AND have read permissions, but the read permissions do not extend to the vCenter root level.
What ends up happening is the parser func (called after verifyCreds) can get a null "cluster" column and throws the error in the ticket.
It could potentially throw other related errors we want to avoid getting.
Signed-off-by: Igor Troyanovsky itroyano@redhat.com
Assisted-by: claude
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED