Skip to content

Conversation

@ravening
Copy link
Member

Description

Provide an api support to update volume name by all users
Can be done both through api and UI

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)
  • 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
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

Screenshot 2021-01-14 at 10 25 29

How Has This Been Tested?

Through API

update volume id=3e8f9553-e1c9-4ac0-bb43-10d7d928daf2 name=test-admin

Copy link
Contributor

@RodrigoDLopez RodrigoDLopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to understand in which scenario it is necessary to give freedom for ordinary users to change names within the environment.
You can explain it to me because I couldn't reach a conclusion on my own.

type = CommandType.STRING,
description = "The chain info of the volume",
since = "4.4")
since = "4.4", authorized = {RoleType.Admin})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but I believe that only administrators can execute the updateVolume command.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

admins and regular users can run this command. but regular users can only change name

description = "an optional field, whether to the display the volume to the end user or not.", authorized = {RoleType.Admin})
private Boolean displayVolume;

@Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "new name of the volume", since = "4.16")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You put roleAdmin in all the other parameters of this API, but not this one.
Any reason for this behavior? users without administrator permission will not be able to run this command. And ordinary users shouldn't be able to change the name of what doesn't belong to them.

It would be nice to capitalize the n in new as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other parameters have sensitive info whereas the "name" parameter is not critical.
So regular users can change only name but not other important values

Account caller = CallContext.current().getCallingAccount();
if (!_accountMgr.isRootAdmin(caller.getId())) {
if (path != null || state != null || storageId != null || displayVolume != null || customId != null || chainInfo != null) {
throw new InvalidParameterValueException("The domain admin and normal user are not allowed to update volume except volume name");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make sense to allow ordinary users the power to change names within the environment?

@rohityadavcloud rohityadavcloud added this to the 4.16.0.0 milestone Jan 18, 2021
@ravening
Copy link
Member Author

I am trying to understand in which scenario it is necessary to give freedom for ordinary users to change names within the environment.
You can explain it to me because I couldn't reach a conclusion on my own.

@RodrigoDLopez there is no harm in changing the volume name. so anybody can do it

Copy link
Contributor

@RodrigoDLopez RodrigoDLopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ravening I know that changing volume names does no harm, but I don't like empowering users even if they are cosmetic powers.
But you have my approval and my +1

@weizhouapache
Copy link
Member

code lgtm

@mib1185
Copy link
Contributor

mib1185 commented Jan 26, 2021

Hi @ravening
why did you close this PR before merge? 🤔

@ravening
Copy link
Member Author

Hi @ravening
why did you close this PR before merge? 🤔

@mib1185 sorry it got closed after some rebase with my branch.. I have opened new pr with ui changes in #4618

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants