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

mgr/dashboard: Allow users to change their password on the UI #28935

Merged
merged 2 commits into from Jul 18, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Next

mgr/dashboard: Allow users to change their password on the UI

Fixes: https://tracker.ceph.com/issues/40248

Signed-off-by: Volker Theile <vtheile@suse.com>
  • Loading branch information...
votdev committed Jul 9, 2019
commit ee80139fa095821d5c26c8f41021e78a2e994aa2
@@ -6,7 +6,7 @@

import jwt

from .helper import DashboardTestCase
from .helper import DashboardTestCase, JObj, JLeaf


class AuthTest(DashboardTestCase):
@@ -40,6 +40,12 @@ def test_login_valid(self):
self._post("/api/auth", {'username': 'admin', 'password': 'admin'})
self.assertStatus(201)
data = self.jsonBody()
self.assertSchema(data, JObj(sub_elems={
'token': JLeaf(str),
'username': JLeaf(str),
'permissions': JObj(sub_elems={}, allow_unknown=True),
'sso': JLeaf(bool)
}, allow_unknown=False))
self._validate_jwt_token(data['token'], "admin", data['permissions'])

def test_login_invalid(self):
@@ -136,3 +142,25 @@ def test_invalidate_token_by_admin(self):
self._get("/api/host")
self.assertStatus(200)
self.delete_user("user")

def test_check_token(self):
self.login("admin", "admin")
self._post("/api/auth/check", {"token": self.jsonBody()["token"]})
self.assertStatus(200)
data = self.jsonBody()
self.assertSchema(data, JObj(sub_elems={
"username": JLeaf(str),
"permissions": JObj(sub_elems={}, allow_unknown=True),
"sso": JLeaf(bool)
}, allow_unknown=False))
self.logout()

def test_check_wo_token(self):
self.login("admin", "admin")
self._post("/api/auth/check", {"token": ""})
self.assertStatus(200)
data = self.jsonBody()
self.assertSchema(data, JObj(sub_elems={
"login_url": JLeaf(str)
}, allow_unknown=False))
self.logout()
@@ -113,3 +113,34 @@ def test_update_user_invalid_role(self):
self.assertStatus(400)
self.assertError(code='role_does_not_exist',
component='user')

def test_change_password_from_other_user(self):
self._post('/api/user/test2/change_password', {
'old_password': 'abc',
'new_password': 'xyz'
})
self.assertStatus(400)
self.assertError(code='invalid_user_context', component='user')

def test_change_password_old_not_match(self):
self._post('/api/user/admin/change_password', {
'old_password': 'foo',
'new_password': 'bar'
})
self.assertStatus(400)
self.assertError(code='invalid_old_password', component='user')

def test_change_password(self):
self.create_user('test1', 'test1', ['read-only'])
self.login('test1', 'test1')
self._post('/api/user/test1/change_password', {
'old_password': 'test1',
'new_password': 'foo'
})
self.assertStatus(200)
self.logout()
self._post('/api/auth', {'username': 'test1', 'password': 'test1'})
self.assertStatus(400)
self.assertError(code='invalid_credentials', component='auth')
self.delete_user('test1')
self.login('admin', 'admin')
@@ -28,7 +28,8 @@ def create(self, username, password):
return {
'token': token,
'username': username,
'permissions': user_perms
'permissions': user_perms,
'sso': mgr.SSO_DB.protocol == 'saml2'
}

logger.debug('Login failed')
@@ -64,6 +65,7 @@ def check(self, token):
return {
'username': user.username,
'permissions': user.permissions_dict(),
'sso': mgr.SSO_DB.protocol == 'saml2'
}

logger.debug("AMT: user info changed after token was"
@@ -3,7 +3,7 @@

import cherrypy

from . import ApiController, RESTController
from . import BaseController, ApiController, RESTController, Endpoint
from .. import mgr
from ..exceptions import DashboardException, UserAlreadyExists, \
UserDoesNotExist
@@ -89,3 +89,24 @@ def set(self, username, password=None, name=None, email=None, roles=None):
user.set_roles(user_roles)
mgr.ACCESS_CTRL_DB.save()
return User._user_to_dict(user)


@ApiController('/user/{username}')
class UserChangePassword(BaseController):
@Endpoint('POST')
def change_password(self, username, old_password, new_password):

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 9, 2019

Contributor

POST HTTP Verb is for "changing" or "updating" something, no need to emphasize that with the change_password endpoint. And also password is not a stand-alone resource, but a 1:1 property of the User resource.

So I think this should be just POST /user/<:id> and in HTTP body let's embed the updated properties: in this case password (the new one), and no need to provide the old one (as we only need the hash to check whether it's the same, right?).

This comment has been minimized.

Copy link
@votdev

votdev Jul 9, 2019

Author Contributor

It is not possible to use POST /user/<:id> because this requires Scope.USER privileges which for example a read-only user does not have. In this case such a user is not able to change his password.

This comment has been minimized.

Copy link
@votdev

votdev Jul 9, 2019

Author Contributor

Requiring the old password for validation is a common security thing. All services (e.g. GitHub, Jira, Trello) i know require the input of the old password when changing the password.

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 9, 2019

Contributor

It is not possible to use POST /user/<:id> because this requires Scope.USER privileges which for example a read-only user does not have. In this case such a user is not able to change his password.

Mmm, interesting issue, as in this case User is the resource where auth rights emanate (kind of vicious circle).

Would it make sense then to have an aliased URL /user/self (not sure if CherryPy mapper will allow this), so all users (regardless their permissions) can interact with it:

@ApiController('/user/self')
class SelfUser(User):
  def update(self, old_password, new_password):
    super(SelfUser, self).set(JwtManager.get_token_from_header()['username'], ...)

This comment has been minimized.

Copy link
@votdev

votdev Jul 10, 2019

Author Contributor

This will collide with the scenario when there is a user that is called self. Every endpoint that is /user/xxx except /user/<namename> can not be implemented, the parameter after /user is always a username.

session_username = JwtManager.get_username()
if username != session_username:
raise DashboardException(msg='Invalid user context',
code='invalid_user_context',
component='user')
try:
user = mgr.ACCESS_CTRL_DB.get_user(session_username)
except UserDoesNotExist:
raise cherrypy.HTTPError(404)
if not user.compare_password(old_password):
raise DashboardException(msg='Invalid old password',
code='invalid_old_password',
component='user')
user.set_password(new_password)

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 9, 2019

Contributor

Rather than using a method here, Python provides @property for dealing with getter-setter logic. However I understand in this case with the current design it's not straight-forward, as we are mixing here: 1 controller + 1 thin model User (why in services/access_control?) + 2 different adapters (JWTManager + ACCESS_CTRL_DB):

@property
def password(self):
  return self.__password

@password.setter
def password(self, value):
  if not password_valid(value):
    raise WhateverEx
  self.__password = value
  self.refreshLastUpdate()
  self.save() # if we don't want to perform per-field DB saves, we may force this happen externally

This comment has been minimized.

Copy link
@votdev

votdev Jul 9, 2019

Author Contributor

I've choosen NOT to use @property here because other setters in this class are also not implemented this way. This is only to keep exiting style and do not mix up various ones. Could be done in a follow up cleanup PR if wanted.

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 9, 2019

Contributor

No problem.

This comment has been minimized.

Copy link
@LenzGr

LenzGr Jul 12, 2019

Contributor

I'd suggest to submit a cleanup issue on our tracker to take care of this.

mgr.ACCESS_CTRL_DB.save()
@@ -23,6 +23,7 @@ import { NfsListComponent } from './ceph/nfs/nfs-list/nfs-list.component';
import { PerformanceCounterComponent } from './ceph/performance-counter/performance-counter/performance-counter.component';
import { LoginComponent } from './core/auth/login/login.component';
import { SsoNotFoundComponent } from './core/auth/sso/sso-not-found/sso-not-found.component';
import { UserPasswordFormComponent } from './core/auth/user-password-form/user-password-form.component';
import { ForbiddenComponent } from './core/forbidden/forbidden.component';
import { NotFoundComponent } from './core/not-found/not-found.component';
import { ActionLabels, URLVerbs } from './shared/constants/app.constants';
@@ -209,14 +210,28 @@ const routes: Routes = [
},
loadChildren: './ceph/rgw/rgw.module#RoutedRgwModule'
},
// Dashboard Settings
// User/Role Management
{
path: 'user-management',
canActivate: [AuthGuardService],
canActivateChild: [AuthGuardService],
data: { breadcrumbs: 'User management', path: null },
loadChildren: './core/auth/auth.module#RoutedAuthModule'
},
// User Profile
{
path: 'user-profile',
canActivate: [AuthGuardService],
canActivateChild: [AuthGuardService],
data: { breadcrumbs: 'User profile', path: null },
children: [
{
path: URLVerbs.EDIT,
component: UserPasswordFormComponent,
data: { breadcrumbs: ActionLabels.EDIT }
}
]
},
// NFS
{
path: 'nfs/501/:message',
@@ -17,6 +17,7 @@ import { RoleListComponent } from './role-list/role-list.component';
import { SsoNotFoundComponent } from './sso/sso-not-found/sso-not-found.component';
import { UserFormComponent } from './user-form/user-form.component';
import { UserListComponent } from './user-list/user-list.component';
import { UserPasswordFormComponent } from './user-password-form/user-password-form.component';
import { UserTabsComponent } from './user-tabs/user-tabs.component';

@NgModule({
@@ -39,7 +40,8 @@ import { UserTabsComponent } from './user-tabs/user-tabs.component';
SsoNotFoundComponent,
UserTabsComponent,
UserListComponent,
UserFormComponent
UserFormComponent,
UserPasswordFormComponent
]
})
export class AuthModule {}
@@ -48,7 +48,7 @@ export class LoginComponent implements OnInit {
window.location.replace(login.login_url);
}
} else {
this.authStorageService.set(login.username, token, login.permissions);
this.authStorageService.set(login.username, token, login.permissions, login.sso);
this.router.navigate(['']);
}
});
@@ -0,0 +1,110 @@
<div class="col-sm-12 col-lg-6">
<form #frm="ngForm"
[formGroup]="userForm"
novalidate>
<div class="card">
<div i18n="form title|Example: Create Pool@@formTitle"
class="card-header">{{ action | titlecase }} {{ resource | upperFirst }}</div>

<div class="card-body">
<!-- Old password -->
<div class="form-group row">
<label class="col-form-label col-sm-3"
for="oldpassword">
<ng-container i18n>Old password</ng-container>
<span class="required"></span>
</label>
<div class="col-sm-9">
<div class="input-group">
<input class="form-control"
type="password"
placeholder="Old password..."
id="oldpassword"
formControlName="oldpassword"
autocomplete="off"
autofocus>
<span class="input-group-append">
<button class="btn btn-light"
cdPasswordButton="oldpassword">
</button>
</span>
</div>
<span class="invalid-feedback"
*ngIf="userForm.showError('oldpassword', frm, 'required')"
i18n>This field is required.</span>
</div>
</div>

<!-- New password -->
<div class="form-group row">
<label class="col-form-label col-sm-3"
for="newpassword">
<ng-container i18n>New password</ng-container>
<span class="required"></span>
</label>
<div class="col-sm-9">
<div class="input-group">
<input class="form-control"
type="password"
placeholder="Password..."
id="newpassword"
autocomplete="new-password"
formControlName="newpassword">
<span class="input-group-append">
<button class="btn btn-light"
cdPasswordButton="newpassword">
</button>
</span>
</div>
<span class="invalid-feedback"
*ngIf="userForm.showError('newpassword', frm, 'required')"
i18n>This field is required.</span>
<span class="invalid-feedback"
*ngIf="userForm.showError('newpassword', frm, 'notmatch')"
i18n>The old and new passwords must be different.</span>
</div>
</div>

<!-- Confirm new password -->
<div class="form-group row">
<label class="col-form-label col-sm-3"
for="confirmnewpassword">
<ng-container i18n>Confirm new password</ng-container>
<span class="required"></span>
</label>
<div class="col-sm-9">
<div class="input-group">
<input class="form-control"
type="password"
autocomplete="off"
placeholder="Confirm new password..."
id="confirmnewpassword"
formControlName="confirmnewpassword">
<span class="input-group-append">
<button class="btn btn-light"
cdPasswordButton="confirmnewpassword">
</button>
</span>
</div>
<span class="invalid-feedback"
*ngIf="userForm.showError('confirmnewpassword', frm, 'required')"
i18n>This field is required.</span>
<span class="invalid-feedback"
*ngIf="userForm.showError('confirmnewpassword', frm, 'match')"
i18n>Password confirmation doesn't match the new password.</span>
</div>
</div>
</div>

<div class="card-footer">
<div class="button-group text-right">
<cd-submit-button (submitAction)="onSubmit()"
[form]="userForm"
i18n="form action button|Example: Create Pool@@formActionButton">
{{ action | titlecase }} {{ resource | upperFirst }}
</cd-submit-button>
</div>
</div>
</div>
</form>
</div>
@@ -0,0 +1,85 @@
import { HttpClientTestingModule, HttpTestingController } from '@angular/common/http/testing';
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { ReactiveFormsModule } from '@angular/forms';
import { Router } from '@angular/router';
import { RouterTestingModule } from '@angular/router/testing';

import { ToastrModule } from 'ngx-toastr';

import { configureTestBed, FormHelper, i18nProviders } from '../../../../testing/unit-test-helper';
import { ComponentsModule } from '../../../shared/components/components.module';
import { CdFormGroup } from '../../../shared/forms/cd-form-group';
import { AuthStorageService } from '../../../shared/services/auth-storage.service';
import { SharedModule } from '../../../shared/shared.module';
import { UserPasswordFormComponent } from './user-password-form.component';

describe('UserPasswordFormComponent', () => {
let component: UserPasswordFormComponent;
let fixture: ComponentFixture<UserPasswordFormComponent>;
let form: CdFormGroup;
let formHelper: FormHelper;
let httpTesting: HttpTestingController;
let router: Router;
let authStorageService: AuthStorageService;

configureTestBed(
{
imports: [
HttpClientTestingModule,
RouterTestingModule,
ReactiveFormsModule,
ComponentsModule,
ToastrModule.forRoot(),
SharedModule
],
declarations: [UserPasswordFormComponent],
providers: i18nProviders
},
true
);

beforeEach(() => {
fixture = TestBed.createComponent(UserPasswordFormComponent);
component = fixture.componentInstance;
form = component.userForm;
httpTesting = TestBed.get(HttpTestingController);
router = TestBed.get(Router);
authStorageService = TestBed.get(AuthStorageService);
spyOn(router, 'navigate');
fixture.detectChanges();
formHelper = new FormHelper(form);
});

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

it('should validate old password required', () => {
formHelper.expectErrorChange('oldpassword', '', 'required');
formHelper.expectValidChange('oldpassword', 'foo');
});

it('should validate password match', () => {
formHelper.setValue('newpassword', 'aaa');
formHelper.expectErrorChange('confirmnewpassword', 'bbb', 'match');
formHelper.expectValidChange('confirmnewpassword', 'aaa');
});

it('should submit', () => {
spyOn(authStorageService, 'getUsername').and.returnValue('xyz');
formHelper.setMultipleValues({
oldpassword: 'foo',
newpassword: 'bar'
});
formHelper.setValue('confirmnewpassword', 'bar', true);
component.onSubmit();
const request = httpTesting.expectOne('api/user/xyz/change_password');
expect(request.request.method).toBe('POST');
expect(request.request.body).toEqual({
old_password: 'foo',
new_password: 'bar'
});
request.flush({});
expect(router.navigate).toHaveBeenCalledWith(['/logout']);
});
});
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.