You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "winterhazel (via GitHub)" <gi...@apache.org> on 2024/02/27 19:12:47 UTC
[PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
winterhazel opened a new pull request, #8713:
URL: https://github.com/apache/cloudstack/pull/8713
### Description
In any instance's page, the volumes tab always gets shown, even if the account does not have permission to list the instance's volumes.
This PR adds a check in the UI in order to hide the volumes tab if the current account does not have permission to use the `listVolumes` API.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [X] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [X] Minor
### Screenshots (if appropriate):
Before, a user without permission could access the volumes tab, but the volumes would not get listed. With these changes, the volumes tab does not get shown.
### How Has This Been Tested?
1. I created a role that had access to `listVolumes`;
2. I created an account using the role;
3. I created a VM using the account;
4. In the created account, I accessed the VM's page and verified that the volumes tab was shown;
5. I removed the access to `listVolumes` from the role created in step 1;
6. In the created account, I accessed the VM's page again and verified that the volumes tab was not shown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-1988677732
@sureshanaparti are your concerns met?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-2075150895
> @sureshanaparti are your concerns met?
Ping @sureshanaparti
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-1968418112
@blueorangutan ui
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#discussion_r1505899256
##########
ui/src/views/compute/InstanceTab.vue:
##########
@@ -33,7 +33,7 @@
<router-link :to="{ path: '/iso/' + vm.isoid }">{{ vm.isoname }}</router-link> <br/>
<barcode-outlined /> {{ vm.isoid }}
</a-tab-pane>
- <a-tab-pane :tab="$t('label.volumes')" key="volumes">
+ <a-tab-pane :tab="$t('label.volumes')" key="volumes" v-if="'listVolumes' in $store.getters.apis">
Review Comment:
I think, it's better to keep the Volumes tab and show empty list when no permissions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-1968418444
@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-1968465299
tested works in qa, but note that also the whole storage tab in the main menu has disappeared:
was this intended @winterhazel ? (no snapshots/backups)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-1968439423
UI build: :heavy_check_mark:
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/8713 (QA-JID-291)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "BryanMLima (via GitHub)" <gi...@apache.org>.
BryanMLima commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-1971658605
Manually tested this, LGTM.
---
> > > tested works in qa, but note that also the whole storage tab in the main menu has disappeared: 
> >
> >
> > @DaanHoogland the changes in this PR do not affect the storage tab in the main menu.
> > There are two APIs for listing volumes: `listVolumesMetrics` (used in the volumes listing page) and `listVolumes` (used in the instance's page, where this PR is adding a check). I suppose that the storage tab disappeared for you because the access to `listVolumesMetrics` has been denied (this is the current behavior, which has not been affected by this PR).
>
> Ok, this then is another bug, no storage page when rights to listBackups and listSnapshots do exist :(
Regarding this situation I will create an issue, may even create a PR for this, as this should be an easy fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-1969431253
> tested works in qa, but note that also the whole storage tab in the main menu has disappeared: 
>
> was this intended @winterhazel ? (no snapshots/backups)
@DaanHoogland the changes in this PR do not affect the storage tab in the main menu.
There are two APIs for listing volumes: `listVolumesMetrics` (used in the volumes listing page) and `listVolumes` (used in the instance's page, where this PR is adding a check). I suppose that the storage tab disappeared for you because the access to `listVolumesMetrics` has been denied (this is the current behavior, which has not been affected by this PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#discussion_r1506293262
##########
ui/src/views/compute/InstanceTab.vue:
##########
@@ -33,7 +33,7 @@
<router-link :to="{ path: '/iso/' + vm.isoid }">{{ vm.isoname }}</router-link> <br/>
<barcode-outlined /> {{ vm.isoid }}
</a-tab-pane>
- <a-tab-pane :tab="$t('label.volumes')" key="volumes">
+ <a-tab-pane :tab="$t('label.volumes')" key="volumes" v-if="'listVolumes' in $store.getters.apis">
Review Comment:
The other tabs in this page are hidden when the user does not have permission to list their contents. I don't see why the volumes tab should be an exception.
Showing an empty list may lead users into thinking that the instance does not have any volumes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-1969866705
> > tested works in qa, but note that also the whole storage tab in the main menu has disappeared: 
>
> @DaanHoogland the changes in this PR do not affect the storage tab in the main menu.
>
> There are two APIs for listing volumes: `listVolumesMetrics` (used in the volumes listing page) and `listVolumes` (used in the instance's page, where this PR is adding a check). I suppose that the storage tab disappeared for you because the access to `listVolumesMetrics` has been denied (this is the current behavior, which has not been affected by this PR).
Ok, this then is another bug, no storage page when rights to listBackups and listSnapshots do exist :(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-1967438424
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8713?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 4.36%. Comparing base [(`c8a4575`)](https://app.codecov.io/gh/apache/cloudstack/commit/c8a4575bcd00bc5342a9cbea9d3e6c693c126cb7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`2faf3aa`)](https://app.codecov.io/gh/apache/cloudstack/pull/8713?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #8713 +/- ##
============================================
- Coverage 30.98% 4.36% -26.62%
============================================
Files 5355 361 -4994
Lines 375780 28831 -346949
Branches 54913 5037 -49876
============================================
- Hits 116429 1259 -115170
+ Misses 243932 27433 -216499
+ Partials 15419 139 -15280
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/8713/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8713/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/8713/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.36% <ø> (ø)` | |
| [unit-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8713/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/8713?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#discussion_r1505899256
##########
ui/src/views/compute/InstanceTab.vue:
##########
@@ -33,7 +33,7 @@
<router-link :to="{ path: '/iso/' + vm.isoid }">{{ vm.isoname }}</router-link> <br/>
<barcode-outlined /> {{ vm.isoid }}
</a-tab-pane>
- <a-tab-pane :tab="$t('label.volumes')" key="volumes">
+ <a-tab-pane :tab="$t('label.volumes')" key="volumes" v-if="'listVolumes' in $store.getters.apis">
Review Comment:
I think, it's better to the Volumes tab and show empty list when no permissions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Hide volumes tab in instance page when user does not have permission to list volumes [cloudstack]
Posted by "lucas-a-martins (via GitHub)" <gi...@apache.org>.
lucas-a-martins commented on PR #8713:
URL: https://github.com/apache/cloudstack/pull/8713#issuecomment-1984256053
lgtm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org