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

mgr/dashboard: Ceph dashboard user management from the UI #22758

Merged
merged 7 commits into from Jul 25, 2018

Conversation

Projects
None yet
7 participants
@ricardoasmarques
Member

ricardoasmarques commented Jun 28, 2018

Users with 'user' scope permissions will be able to manage ceph dashboard users from the UI.

This PR includes both frontend and backend implementation.

screenshot from 2018-06-28 10-30-59

screenshot from 2018-06-28 10-44-39

@LenzGr LenzGr changed the title from Ceph dashboard user management from the UI to mgr/dashboard: Ceph dashboard user management from the UI Jun 28, 2018

@LenzGr LenzGr added the feature label Jun 28, 2018

@votdev

This comment has been minimized.

Contributor

votdev commented Jun 28, 2018

I'd expect the form can't be submitted without selecting a role. Currently kicked out my admin after removing the 'adminstrator' role and pressing 'Update User'. :-\

@votdev

This comment has been minimized.

Contributor

votdev commented Jun 28, 2018

Strange render issue.
fireshot capture 39 - ceph - http___localhost_4200_ _users_add

from ..services.access_control import ACCESS_CTRL_DB, SYSTEM_ROLES
from . import ApiController, RESTController
from ..security import Scope

This comment has been minimized.

@rjfd

rjfd Jun 28, 2018

Contributor

nit: re-order the imports to:

from . import ApiController, RESTController
from ..security import Scope
from ..services.access_control import ACCESS_CTRL_DB, SYSTEM_ROLES
from ..services.access_control import ACCESS_CTRL_DB, SYSTEM_ROLES
from . import ApiController, RESTController
from ..security import Scope
from ..tools import Session

This comment has been minimized.

@rjfd

rjfd Jun 28, 2018

Contributor

nit: re-order the commits

raise cherrypy.HTTPError(404)
ACCESS_CTRL_DB.save()
def set(self, username=None, password=None, name=None, email=None, roles=None):

This comment has been minimized.

@rjfd

rjfd Jun 28, 2018

Contributor

the username parameter cannot be optional since it corresponds to the RESOURCE_ID of this REST controller. (I know the controller infrastructure should do these kind of validations, but we still haven't reached that point)

except UserDoesNotExist:
raise cherrypy.HTTPError(404)
user_roles = []
if roles:

This comment has been minimized.

@rjfd

rjfd Jun 28, 2018

Contributor

the code in this conditional block is the same as in the create method. you could extract this into a class method.

@votdev

This comment has been minimized.

Contributor

votdev commented Jun 28, 2018

Strange render issue in role dropdown.
fireshot capture 40 - ceph - http___localhost_4200_ _users_add

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jun 28, 2018

@votdev @ricardoasmarques

I'd expect the form can't be submitted without selecting a role. Currently kicked out my admin after removing the 'adminstrator' role and pressing 'Update User'. :-\

I think there are two different issues:
The first one is: creating a user without any role associated is allowed by the CLI implementation, so I'm not against of also allowing it in the web UI.

The second one is: maybe we shouldn't allow a user to edit the roles of its own user.

<cd-select-badges
[data]="userForm.controls.roles.value"
[options]="allRoles"
emptyMessage="There are no roles."></cd-select-badges>

This comment has been minimized.

@callithea

callithea Jun 29, 2018

Member

I think "There are no roles." can be a bit misleading (no roles available at all).
Maybe it should be rephrased to something like "Currently no roles selected." or "No roles have been selected." ?

@Devp00l

Frontend tests are missing

it('should create', () => {
expect(component).toBeTruthy();
});

This comment has been minimized.

@Devp00l

Devp00l Jun 29, 2018

Contributor

Could you please add some tests here?

this.createForm();
}
createForm() {

This comment has been minimized.

@Devp00l

Devp00l Jun 29, 2018

Contributor

Could you tests that the right form is created for the specific cases?

);
}
ngOnInit() {

This comment has been minimized.

@Devp00l

Devp00l Jun 29, 2018

Contributor

Could you tests the correct component initialization?

getRequest(): UserFormModel {
const userFormModel = new UserFormModel();
userFormModel.username = this.userForm.get('username').value;

This comment has been minimized.

@Devp00l

Devp00l Jun 29, 2018

Contributor

How about combining these lines to:
['username', 'password', 'name', 'email', 'roles'].forEach(key=> userFormModel[key] = this.userForm.get(key).value)

This comment has been minimized.

@ricardoasmarques
);
}
submit() {

This comment has been minimized.

@Devp00l

Devp00l Jun 29, 2018

Contributor

Could you please test the submission cases?

);
}
deleteUserModal() {

This comment has been minimized.

@Devp00l

Devp00l Jun 29, 2018

Contributor

Could you please add tests for the deletion?

it('should create', () => {
expect(component).toBeTruthy();
});

This comment has been minimized.

@Devp00l

Devp00l Jun 29, 2018

Contributor

Please add tests for every functionality of the shared component.

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Jul 2, 2018

Member

@Devp00l I've added tests to the cd-select-badges component.

@capri1989

This comment has been minimized.

Contributor

capri1989 commented Jul 2, 2018

Maybe I've missed it but what's about the documentation of the different roles? Is it already documented somewhere? Right now I have an idea but I'm not completely sure which roles I need in which circumstances. Any kind of help or redirect to an existing documentation would be helpful.

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Jul 2, 2018

Maybe I've missed it but what's about the documentation of the different roles?

It's in the manual already: http://docs.ceph.com/docs/master/mgr/dashboard/#user-and-role-management

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-user-management branch from d340747 to e131e9a Jul 2, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 2, 2018

@capri1989 @LenzGr Maybe the description of each role (in the backend) should be improved, but I think this doesn't have to be part of this PR - https://github.com/ceph/ceph/blob/master/src/pybind/mgr/dashboard/services/access_control.py#L88

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 2, 2018

@votdev @rjfd I've addressed all your comments.

With regards to the removal of own user permissions, I've added the following warning if logged-in user removes "user" read or update permissions for his own user:

screenshot from 2018-07-02 11-56-37

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-user-management branch 2 times, most recently from 3d36608 to c09538e Jul 2, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 2, 2018

@Devp00l I've added frontend unit tests

@Devp00l

The UI LGTM, but there are some things you can refactor.

}
createForm() {
this.userForm = new FormGroup(

This comment has been minimized.

@Devp00l

Devp00l Jul 4, 2018

Contributor

You can now use CdFormGroup :) with it you can make the template expressions a lot shorter and more readable :)

spyOn(userService, 'create').and.callThrough();
spyOn(userService, 'update').and.callThrough();
spyOn(TestBed.get(RoleService), 'list').and.callFake(() => of(fakeRoles));
spyOn(TestBed.get(AuthStorageService), 'getUsername').and.callFake(() => of(fakeUsername));

This comment has been minimized.

@Devp00l

Devp00l Jul 4, 2018

Contributor

Thank you for that many tests :) But they need a bit of refactor:

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/core/auth/user-form/user-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/core/auth/user-form/user-form.component.spec.ts
index 1dbb152..0b54470 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/core/auth/user-form/user-form.component.spec.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/core/auth/user-form/user-form.component.spec.ts
@@ -7,10 +7,8 @@ import { RouterTestingModule } from '@angular/router/testing';
 import { ToastModule } from 'ng2-toastr';
 import { of } from 'rxjs';
 
-import { RoleService } from '../../../shared/api/role.service';
 import { UserService } from '../../../shared/api/user.service';
 import { ComponentsModule } from '../../../shared/components/components.module';
-import { AuthStorageService } from '../../../shared/services/auth-storage.service';
 import { SharedModule } from '../../../shared/shared.module';
 import { configureTestBed } from '../../../shared/unit-test-helper';
 import { UserFormComponent } from './user-form.component';
@@ -22,18 +20,6 @@ describe('UserFormComponent', () => {
   let fixture: ComponentFixture<UserFormComponent>;
   let userService: UserService;
 
-  const fakeRoles = [
-    {
-      name: 'administrator',
-      description: 'Administrator',
-      scopes_permissions: {
-        user: ['create', 'delete', 'read', 'update'],
-        osd: ['create', 'delete', 'read', 'update']
-      }
-    }
-  ];
-  const fakeUsername = 'user1';
-
   configureTestBed({
     imports: [
       HttpClientTestingModule,
@@ -51,10 +37,6 @@ describe('UserFormComponent', () => {
     component = fixture.componentInstance;
     form = component.userForm;
     userService = TestBed.get(UserService);
-    spyOn(userService, 'create').and.callThrough();
-    spyOn(userService, 'update').and.callThrough();
-    spyOn(TestBed.get(RoleService), 'list').and.callFake(() => of(fakeRoles));
-    spyOn(TestBed.get(AuthStorageService), 'getUsername').and.callFake(() => of(fakeUsername));
     fixture.detectChanges();
   });
 
@@ -66,6 +48,7 @@ describe('UserFormComponent', () => {
   describe('create mode', () => {
     let router: Router;
     const setUrl = (url) => Object.defineProperty(router, 'url', { value: url });
+
     beforeEach(() => {
       router = TestBed.get(Router);
       setUrl('/users/add');
@@ -98,7 +81,7 @@ describe('UserFormComponent', () => {
       form.get('confirmpassword').setValue('bbb');
       expect(form.get('confirmpassword').hasError('match')).toBeTruthy();
       form.get('confirmpassword').setValue('aaa');
-      expect(form.get('confirmpassword').hasError('match')).toBeFalsy();
+      expect(form.get('confirmpassword').valid).toBeTruthy();
     });
 
     it('should validate email', () => {
@@ -111,20 +94,18 @@ describe('UserFormComponent', () => {
     });
 
     it('should submit', () => {
-      form.get('username').setValue('user0');
-      form.get('name').setValue('User 0');
-      form.get('password').setValue('pass0');
-      form.get('confirmpassword').setValue('pass0');
-      form.get('email').setValue('user0@email.com');
-      form.get('roles').setValue(['administrator']);
-      component.submit();
-      expect(userService.create).toHaveBeenCalledWith({
+      spyOn(userService, 'create').and.callThrough();
+      const user: UserFormModel = {
         username: 'user0',
         password: 'pass0',
         name: 'User 0',
         email: 'user0@email.com',
         roles: ['administrator']
-      });
+      };
+      Object.keys(user).forEach((k) => form.get(k).setValue(user[k]));
+      form.get('confirmpassword').setValue(user.password);
+      component.submit();
+      expect(userService.create).toHaveBeenCalledWith(user);
     });
   });
 
@@ -146,14 +127,13 @@ describe('UserFormComponent', () => {
 
     it('should disable fields if editing', () => {
       expect(form.get('username').disabled).toBeTruthy();
-      expect(form.get('name').disabled).toBeFalsy();
-      expect(form.get('password').disabled).toBeFalsy();
-      expect(form.get('confirmpassword').disabled).toBeFalsy();
-      expect(form.get('email').disabled).toBeFalsy();
-      expect(form.get('roles').disabled).toBeFalsy();
+      ['name', 'password', 'confirmpassword', 'email', 'roles'].forEach((controlName) =>
+        expect(form.get(controlName).disabled).toBeFalsy()
+      );
     });
 
     it('should set control values', () => {
+      // Change to getValue through usage of CdFormGroup
       expect(form.get('username').value).toBe(user.username);
       expect(form.get('name').value).toBe(user.name);
       expect(form.get('password').value).toBe('');
@@ -167,13 +147,14 @@ describe('UserFormComponent', () => {
     });
 
     it('should validate password not required', () => {
-      form.get('password').setValue('');
-      form.get('confirmpassword').setValue('');
-      expect(form.get('password').hasError('required')).toBeFalsy();
-      expect(form.get('confirmpassword').hasError('required')).toBeFalsy();
+      ['password', 'confirmpassword'].forEach((controlName) => {
+        form.get(controlName).setValue('');
+        expect(form.get(controlName).hasError('required')).toBeFalsy();
+      });
     });
 
     it('should submit', () => {
+      spyOn(userService, 'update').and.callThrough();
       component.submit();
       expect(userService.update).toHaveBeenCalledWith({
         username: 'user1',
for (const role of this.allRoles) {
if (roles.indexOf(role.name) !== -1 && role.scopes_permissions['user']) {
const userPermissions = role.scopes_permissions['user'];
if (userPermissions.indexOf('read') !== -1) {

This comment has been minimized.

@Devp00l

Devp00l Jul 4, 2018

Contributor

This function should be tested and refactored.
Here is a diff I created during testing:

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/core/auth/user-form/user-form.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/core/auth/user-form/user-form.component.ts
index bb23732..120567a 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/core/auth/user-form/user-form.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/core/auth/user-form/user-form.component.ts
@@ -45,7 +45,7 @@ export class UserFormComponent implements OnInit {
   }
 
   createForm() {
-    this.userForm = new FormGroup(
+    this.userForm = new FormGroup( // Use CdFormGroup here
       {
         username: new FormControl('', {
           validators: [Validators.required]
@@ -84,13 +84,15 @@ export class UserFormComponent implements OnInit {
   }
 
   initAdd() {
-    this.userForm.get('password').setValidators([Validators.required]);
-    this.userForm.get('confirmpassword').setValidators([Validators.required]);
+    ['password', 'confirmpassword'].forEach((controlName) =>
+      this.userForm.get(controlName).setValidators([Validators.required])
+    );
   }
 
   initEdit() {
-    this.userForm.get('password').setValidators([]);
-    this.userForm.get('confirmpassword').setValidators([]);
+    ['password', 'confirmpassword'].forEach((controlName) =>
+      this.userForm.get(controlName).setValidators([])
+    );
     this.disableForEdit();
     this.route.params.subscribe((params: { username: string }) => {
       const username = params.username;
@@ -105,16 +107,15 @@ export class UserFormComponent implements OnInit {
   }
 
   setResponse(response: UserFormModel) {
-    this.userForm.get('username').setValue(response.username);
-    this.userForm.get('name').setValue(response.name);
-    this.userForm.get('email').setValue(response.email);
-    this.userForm.get('roles').setValue(response.roles);
+    ['name', 'username', 'email', 'roles'].forEach((key) =>
+      this.userForm.get(key).setValue(response[key])
+    );
   }
 
   getRequest(): UserFormModel {
     const userFormModel = new UserFormModel();
     ['username', 'password', 'name', 'email', 'roles'].forEach(
-      (key) => (userFormModel[key] = this.userForm.get(key).value)
+      (key) => (userFormModel[key] = this.userForm.get(key).value) //getValue
     );
     return userFormModel;
   }
@@ -137,7 +138,7 @@ export class UserFormComponent implements OnInit {
   }
 
   editAction() {
-    if (this.isRemoveSelfUserReadUpdatePermission()) {
+    if (this.isUserRemovingNeededRolePermissions()) {
       const initialState = {
         titleText: 'Update user',
         buttonText: 'Continue',
@@ -153,31 +154,25 @@ export class UserFormComponent implements OnInit {
       };
       this.modalRef = this.modalService.show(ConfirmationModalComponent, { initialState });
     } else {
+      // only this path is tested
       this.doEditAction();
     }
   }
 
-  private isRemoveSelfUserReadUpdatePermission(): boolean {
+  // This should be tested a test
+  private isUserRemovingNeededRolePermissions(): boolean {
     const isCurrentUser =
       this.authStorageService.getUsername() === this.userForm.get('username').value;
     return isCurrentUser && !this.hasUserReadUpdatePermissions(this.userForm.get('roles').value);
   }
 
   private hasUserReadUpdatePermissions(roles) {
-    let hasReadPermission = false;
-    let hasUpdatePermission = false;
     for (const role of this.allRoles) {
       if (roles.indexOf(role.name) !== -1 && role.scopes_permissions['user']) {
         const userPermissions = role.scopes_permissions['user'];
-        if (userPermissions.indexOf('read') !== -1) {
-          hasReadPermission = true;
-        }
-        if (userPermissions.indexOf('update') !== -1) {
-          hasUpdatePermission = true;
-        }
+        return ['read', 'update'].every((permission) => userPermissions[permission])
       }
     }
-    return hasReadPermission && hasUpdatePermission;
   }
 
   private doEditAction() {
return this.http.get(`api/user/${username}`);
}
create(user: UserFormModel) {

This comment has been minimized.

@Devp00l

Devp00l Jul 4, 2018

Contributor

As we have API tests for all other services it would be kind to maintain that even so it's not very fancy ;)

}
private updateOptions() {
this.data.splice(0, this.data.length);

This comment has been minimized.

@Devp00l

Devp00l Jul 4, 2018

Contributor

This function could be refactored. Here my diff:

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/components/select-badges/select-badges.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/components/select-badges/select-badges.component.ts
index 01d0904..d6a626a 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/shared/components/select-badges/select-badges.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/components/select-badges/select-badges.component.ts
@@ -14,7 +14,7 @@ export class SelectBadgesComponent implements OnChanges {
   constructor() {}
 
   ngOnChanges() {
-    if (this.options) {
+    if (this.data.length > 0) {
       this.options.forEach((option) => {
         if (this.data.indexOf(option.name) !== -1) {
           option.selected = true;
@@ -24,12 +24,7 @@ export class SelectBadgesComponent implements OnChanges {
   }
 
   private updateOptions() {
-    this.data.splice(0, this.data.length);
-    this.options.forEach((option: SelectBadgesOption) => {
-      if (option.selected) {
-        this.data.push(option.name);
-      }
-    });
+    this.data = this.options.filter(option => option.selected).map(option => option.name);
   }
 
   selectOption(option: SelectBadgesOption) {

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Jul 6, 2018

Member

this.data = ... will not work here because we cannot assign a new reference to a field that is an @Input of the component.

This comment has been minimized.

@Devp00l

Devp00l Jul 23, 2018

Contributor

Ok, than this function is fine with me, but could you change line 17 as the if clause will always resolve to true unless you give it a undefined input. IMO this.data.length > 0 would make more sense there.

This comment has been minimized.

@Devp00l

Devp00l Jul 25, 2018

Contributor

@Devp00l yes, the input can be undefined if you are waiting for a response from server to populate the options. I'd prefer to keep this check (in line 17) so that the parent component don't have to initialize the options with an empty array just to prevent an error in the select-badges.component. With this check, the component is more flexible and easy to use.

Understood.
How about:

if (!this.options || !this.data || this.data.length === 0) {
  return;
}
...// inner of condition

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-user-management branch from c09538e to 3ab35cd Jul 6, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 6, 2018

@Devp00l I've addressed your comments

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Jul 7, 2018

LGTM. One (minor) nit: when creating and editing a user's details, I'd suggest to skip the "show password" icons when moving from one input field to the next using the Tab key. Am I right that one currently can only assign existing roles to a user? Do you plan to provide roles management in a separate PR?

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 9, 2018

@LenzGr Yes, role management will be submitted in a separate PR. It's currently a work in progress - https://tracker.ceph.com/issues/24447

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Jul 9, 2018

Another observation: with multiple user accounts, we need a way to display the name of the currently logged in user. I created #24822 on the tracker for that.

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Jul 9, 2018

If I were to make changes to my own account, I need to log out and log in again in order for the changes to take full effect, correct? Would it make sense to display a corresponding notification?

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 21, 2018

If I were to make changes to my own account, I need to log out and log in again in order for the changes to take full effect, correct? Would it make sense to display a corresponding notification?

@LenzGr I've addressed your comment.
If current logged in user roles have been changes, user will be automaticaly logged out:

screenshot_20180721_113132

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 21, 2018

... when creating and editing a user's details, I'd suggest to skip the "show password" icons when moving from one input field to the next using the Tab key

@LenzGr done

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 21, 2018

All comments on this PR have been addressed.

@LenzGr

LenzGr approved these changes Jul 23, 2018

LGTM - thank you!

}
private updateOptions() {
this.data.splice(0, this.data.length);

This comment has been minimized.

@Devp00l

Devp00l Jul 23, 2018

Contributor

Ok, than this function is fine with me, but could you change line 17 as the if clause will always resolve to true unless you give it a undefined input. IMO this.data.length > 0 would make more sense there.

SharedModule
],
declarations: [UserFormComponent, FakeComponent]
});

This comment has been minimized.

@Devp00l

Devp00l Jul 23, 2018

Contributor

Change it to }, true); as the edit mode tests will fail, except the first, without a reset of all loaded modules.

This comment has been minimized.

@ricardoasmarques
if (roles.indexOf(role.name) !== -1 && role.scopes_permissions['user']) {
const userPermissions = role.scopes_permissions['user'];
return ['read', 'update'].every((permission) => {
return userPermissions.indexOf(permission) !== -1;

This comment has been minimized.

@Devp00l

Devp00l Jul 23, 2018

Contributor

return userPermission[permission] would be as sufficient, as it would return undefined if not there which would resolve to false.

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Jul 24, 2018

Member

Note that userPermission is an array, so your suggestion does not work here, because

['read', 'update'].every((permission) => { 
  return ["create", "delete", "read", "update"].indexOf(permission) !== -1 
});

returns true, while:

['read', 'update'].every((permission) => { 
  return ["create", "delete", "read", "update"][permission]
});

returns false.

This comment has been minimized.

@Devp00l

Devp00l Jul 25, 2018

Contributor

If I understand it correctly userPermissions is not a permission object? That's a bit strange as we use permission objects anywhere else. INMHO it makes sense to only use one method how checkout permissions.

private isUserRemovingNeededRolePermissions(): boolean {
const isCurrentUser = this.isCurrentUser();
return isCurrentUser && !this.hasUserReadUpdatePermissions(this.userForm.get('roles').value);

This comment has been minimized.

@Devp00l

Devp00l Jul 23, 2018

Contributor

There are a view this.userForm.get(xyz).value in this component, which you can now replace with this.userForm.getValue(xyz)

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Jul 24, 2018

Member

From CdFormGroup.getValue documentation:

Get the value of a control if it has none it will return false

Note that hasUserReadUpdatePermissions is expecting an array, that's why I would prefer to not use CdFormGroup.getValue for production code.

IMO we should change the implementation of CdFormGroup.getValue in the future, to not transform none value into false value. Thoughts?

This comment has been minimized.

@Devp00l

Devp00l Jul 25, 2018

Contributor

Hm seems like the comment is deprecated - it will only return false in case of an empty string. So it's save to use here.

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 24, 2018

Ok, than this function is fine with me, but could you change line 17 as the if clause will always resolve to true unless you give it a undefined input. IMO this.data.length > 0 would make more sense there.

@Devp00l yes, the input can be undefined if you are waiting for a response from server to populate the options. I'd prefer to keep this check (in line 17) so that the parent component don't have to initialize the options with an empty array just to prevent an error in the select-badges.component. With this check, the component is more flexible and easy to use.

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-user-management branch from 20ff0b2 to e7edb9a Jul 24, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 24, 2018

@Devp00l I've addressed your comments.

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-user-management branch from e7edb9a to 8c9e45c Jul 24, 2018

private isUserRemovingNeededRolePermissions(): boolean {
const isCurrentUser = this.isCurrentUser();
return isCurrentUser && !this.hasUserReadUpdatePermissions(this.userForm.get('roles').value);

This comment has been minimized.

@Devp00l

Devp00l Jul 25, 2018

Contributor

Hm seems like the comment is deprecated - it will only return false in case of an empty string. So it's save to use here.

if (roles.indexOf(role.name) !== -1 && role.scopes_permissions['user']) {
const userPermissions = role.scopes_permissions['user'];
return ['read', 'update'].every((permission) => {
return userPermissions.indexOf(permission) !== -1;

This comment has been minimized.

@Devp00l

Devp00l Jul 25, 2018

Contributor

If I understand it correctly userPermissions is not a permission object? That's a bit strange as we use permission objects anywhere else. INMHO it makes sense to only use one method how checkout permissions.

}
private updateOptions() {
this.data.splice(0, this.data.length);

This comment has been minimized.

@Devp00l

Devp00l Jul 25, 2018

Contributor

@Devp00l yes, the input can be undefined if you are waiting for a response from server to populate the options. I'd prefer to keep this check (in line 17) so that the parent component don't have to initialize the options with an empty array just to prevent an error in the select-badges.component. With this check, the component is more flexible and easy to use.

Understood.
How about:

if (!this.options || !this.data || this.data.length === 0) {
  return;
}
...// inner of condition
@rjfd

rjfd approved these changes Jul 25, 2018

lgtm

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-user-management branch from 8c9e45c to bf3ef2a Jul 25, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 25, 2018

@Devp00l I've addressed your comments.

@Devp00l

LGTM 💯

@LenzGr LenzGr added the needs-rebase label Jul 25, 2018

ricardoasmarques added some commits Jun 18, 2018

mgr/dashboard: Add match validator
Signed-off-by: Ricardo Marques <rimarques@suse.com>
mgr/dashboard: Add user management API
Fixes: https://tracker.ceph.com/issues/24446

Signed-off-by: Ricardo Marques <rimarques@suse.com>
mgr/dashboard: Add asserError helper method
Signed-off-by: Ricardo Marques <rimarques@suse.com>

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-user-management branch from bf3ef2a to ca873e9 Jul 25, 2018

ricardoasmarques added some commits Jun 18, 2018

mgr/dashboard: Add cd-select-badges component
Signed-off-by: Ricardo Marques <rimarques@suse.com>
mgr/dashboard: Ceph dashboard user management
Fixes: https://tracker.ceph.com/issues/24446

Signed-off-by: Ricardo Marques <rimarques@suse.com>
mgr/dashboard: Add 'onCancel' output for 'ConfirmationModal'
Signed-off-by: Ricardo Marques <rimarques@suse.com>
mgr/dashboard: Skip tab focus on "show/hide password" button
Signed-off-by: Ricardo Marques <rimarques@suse.com>

@ricardoasmarques ricardoasmarques force-pushed the ricardoasmarques:wip-user-management branch from ca873e9 to 7f5298a Jul 25, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Jul 25, 2018

@LenzGr This PR is now rebased and passed all checks.

@LenzGr LenzGr merged commit 2fa86d1 into ceph:master Jul 25, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment