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

engine: Check concrete disk permissions firstly on TransferImageStatus #946

Conversation

0ffer
Copy link
Contributor

@0ffer 0ffer commented May 23, 2024

There is some situations when storageDomainId is not come with parameters (image upload cancel and pause operations) for TransferImageStatusCommand. For this operations checked CREATE_DISK permission on SYSTEM_OBJECT (i.e. system-wide). Problem starts when we give permissions for user only on concrete storage domain object (not system-wide). Then permission check failed for operations without storage domain id info in parameters. Here I just add check permission for disk before other objects.

Are you the owner of the code you are sending in, or do you have permission of the owner?

y

@sandrobonazzola
Copy link
Member

@dupondje @BrooklynDewolf can you please review?

@BrooklynDewolf
Copy link
Contributor

Code looks good to me. Nice cleanup :)

@sandrobonazzola
Copy link
Member

/OST

@sandrobonazzola sandrobonazzola force-pushed the bugfix/check-concrete-disk-perms-on-status-command branch from 19f6162 to 49f91aa Compare June 3, 2024 10:11
@michalskrivanek
Copy link
Member

/ost

@sandrobonazzola
Copy link
Member

/ost

@sandrobonazzola sandrobonazzola force-pushed the bugfix/check-concrete-disk-perms-on-status-command branch from 49f91aa to fa6fea1 Compare June 5, 2024 06:52
@sandrobonazzola
Copy link
Member

/ost

@sandrobonazzola
Copy link
Member

OST is failing test_cold_incremental_backup_vm2 with engine error :

2024-06-05 07:55:49,863Z ERROR [org.ovirt.engine.core.bll.storage.backup.HybridBackupCommand] (default task-1) [full_cold_vm_backup] Command 'org.ovirt.engine.core.bll.storage.backup.HybridBackupCommand' failed: CallableStatementCallback; SQL [{call insertvmbackup(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)}]; ERROR: duplicate key value violates unique constraint "vm_backups_pkey"
  Detail: Key (backup_id)=(1733241d-4a0b-4205-9e52-5383eb7a82f2) already exists.

Not sure how much this failure is related to this PR.

@sandrobonazzola sandrobonazzola self-requested a review June 5, 2024 14:52
@BrooklynDewolf
Copy link
Contributor

@sandrobonazzola This was an issue related to an earlier commit that has been merged into the master branch. It has been fixed in #948

@sandrobonazzola sandrobonazzola force-pushed the bugfix/check-concrete-disk-perms-on-status-command branch from fa6fea1 to 5fcc46f Compare June 6, 2024 13:02
…sCommand

There is some situations when storageDomainId is not come with parameters (image upload cancel and pause operations). For this operations checked CREATE_DISK permission on SYSTEM_OBJECT (i.e. system-wide).
Problem starts when we give permissions for user only on concrete storage domain object (not system-wide). Then permission check failed for operations without storage domain id info in parameters. Here I just add check permission for disk before other objects.

Signed-off-by: Stanislav Melnichuk <melnichuk.stas@gmail.com>
@sandrobonazzola sandrobonazzola force-pushed the bugfix/check-concrete-disk-perms-on-status-command branch from 5fcc46f to 28e8695 Compare June 7, 2024 09:11
@sandrobonazzola
Copy link
Member

/ost

@sandrobonazzola
Copy link
Member

/ost

@sandrobonazzola
Copy link
Member

OST passed on run 113878 , merging based on previous reviews.

@sandrobonazzola sandrobonazzola merged commit 1043c13 into oVirt:master Jun 10, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants