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

Conversation

@votdev
Copy link
Contributor

commented Jul 9, 2019

Auswahl_002

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

Signed-off-by: Volker Theile vtheile@suse.com

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
@LenzGr

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

The changes to src/dmclock and zstd are likely unrelated and should be removed from your commit (but the "Unmodified Submodules" check also captured that)

@LenzGr LenzGr requested review from tspmelo and epuertat Jul 9, 2019

@votdev votdev force-pushed the votdev:issue_40248 branch 3 times, most recently from 67d44f3 to df5c41d Jul 9, 2019

@votdev votdev requested review from Devp00l and LenzGr Jul 9, 2019

@votdev votdev marked this pull request as ready for review Jul 9, 2019

@votdev votdev force-pushed the votdev:issue_40248 branch from df5c41d to 73e1bcb Jul 9, 2019

@epuertat
Copy link
Contributor

left a comment

I have a couple of comments on the REST endpoint and the Python logic to deal with it.

:return: `True` if the passwords are equal, otherwise `False`.
:rtype: bool
"""
pass_hash = password_hash(password, self.password)

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 9, 2019

Contributor

Shouldn't we rename self.password to something like self.password_hash? The name is misleading (it's not the password) and, worse, also gives the impression that we are storing passwords in plain text.

This comment has been minimized.

Copy link
@votdev

votdev Jul 9, 2019

Author Contributor

You're right, but IMO this is not part of this PR because it would be a refactoring of existing code that should be done in a new cleanup PR.

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 9, 2019

Contributor

Ok, though I think that the overhead of opening a PR for changing this is an overkill.

This comment has been minimized.

Copy link
@votdev

votdev Jul 10, 2019

Author Contributor

I thought we came to the conclusion to reduce code cleanup in feature PR's because of reducing the backport effort.

:rtype: bool
"""
pass_hash = password_hash(password, self.password)
return pass_hash == self.password

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 9, 2019

Contributor

One-liner? And for the sake of clarity I'd add the salt= keyword argument, as I find this bcrypt syntax specially twisted.

Suggested change
return pass_hash == self.password
return password_hash(password, salt=self.password) == self.password

This comment has been minimized.

Copy link
@votdev

votdev Jul 9, 2019

Author Contributor

Could be done, but i prefer to be able to see what is returned by the method while debugging.

This comment has been minimized.

Copy link
@votdev

votdev Jul 9, 2019

Author Contributor

And for the sake of clarity I'd add the salt= keyword argument, as I find this bcrypt syntax specially twisted.

Do you mean

Suggested change
return pass_hash == self.password
pass_hash = password_hash(password, salt_password=self.password)
return pass_hash == self.password

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 9, 2019

Contributor

Yeah, sorry I messed the bcrypt.hashpw syntax with this one.

@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.

raise DashboardException(msg='Old password do not match',
code='old_password_do_not_match',
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.

@votdev votdev requested a review from rjfd Jul 9, 2019

@LenzGr

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

I'd suggest to change the subject of this PR and the git commit from "As a user, I want to change my password" to something else, e.g. "Allow users to change their password on the UI"

@votdev votdev changed the title mgr/dashboard: As a user, I want to change my password mgr/dashboard: Allow users to change their password on the UI Jul 9, 2019

@votdev votdev force-pushed the votdev:issue_40248 branch from 73e1bcb to 70f41b9 Jul 9, 2019

@ricardoasmarques

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Shouldn't the "Change password" action be hidden when SSO is enabled?

@epuertat

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Shouldn't the "Change password" action be hidden when SSO is enabled?

And I think it should be disabled also in plain HTTP (I don't think we should send passwords in plain-text over the wire).

@votdev

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Shouldn't the "Change password" action be hidden when SSO is enabled?

And I think it should be disabled also in plain HTTP (I don't think we should send passwords in plain-text over the wire).

In this case we must also hide the user creation dialog because this also sends the password in plain-text.

If a customer decides to use HTTP only then he has to know and life with the security issues this will impact. Everything else is paternalism, either if it is well meant.

@votdev

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Shouldn't the "Change password" action be hidden when SSO is enabled?

Good point, will implement this if the UI supports this out-of-the-box easily.

i18n>This field is required.</span>
<span class="help-block"
*ngIf="userForm.showError('confirmnewpassword', frm, 'match')"
i18n>Password confirmation doesn't match the newpassword.</span>

This comment has been minimized.

Copy link
@rjfd

rjfd Jul 10, 2019

Contributor

s/newpassword/new password

@rjfd

rjfd approved these changes Jul 10, 2019

Copy link
Contributor

left a comment

lgtm

@LenzGr

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

And I think it should be disabled also in plain HTTP (I don't think we should send passwords in plain-text over the wire).

This might actually be the case if the dashboard is behind a proxy server that terminates SSL connections. I agree with @votdev that this behavior might be surprising.

@votdev votdev force-pushed the votdev:issue_40248 branch 2 times, most recently from a9768d9 to 2f5f7f4 Jul 10, 2019

@s0nea s0nea added the needs-rebase label Jul 11, 2019

@votdev votdev force-pushed the votdev:issue_40248 branch from 2f5f7f4 to a1cb206 Jul 11, 2019

@votdev votdev removed the needs-rebase label Jul 11, 2019

@votdev votdev force-pushed the votdev:issue_40248 branch from a1cb206 to e31fef0 Jul 11, 2019

@@ -0,0 +1,117 @@
<div class="col-sm-12 col-lg-6">
<form class="form-horizontal"

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 12, 2019

Contributor

form-horizontal doesn't exists anymore.

Suggested change
<form class="form-horizontal"
<form class="form"
#frm="ngForm"
[formGroup]="userForm"
novalidate>
<div class="panel panel-default">

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 12, 2019

Contributor

panel has been dropped in bootstrap 4, we need to use the card class.
Please check rgw-bucket-form.component.html on how that is implemented.
You also need to change the header, body and footers.

Suggested change
<div class="panel panel-default">
<div class="card">
<div class="panel-body">

<!-- Old password -->
<div class="form-group"

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 12, 2019

Contributor

To have a horizontal form we now need to add the row class to each form-group.

Suggested change
<div class="form-group"
<div class="form-group row"

<!-- Old password -->
<div class="form-group"
[ngClass]="{'has-error': userForm.showError('oldpassword', frm)}">

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 12, 2019

Contributor

Validation classes are added automatically now, we don't need to set it anymore.

Suggested change
[ngClass]="{'has-error': userForm.showError('oldpassword', frm)}">
>
<!-- Old password -->
<div class="form-group"
[ngClass]="{'has-error': userForm.showError('oldpassword', frm)}">
<label class="control-label col-sm-3"

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 12, 2019

Contributor

This class has changed.

Suggested change
<label class="control-label col-sm-3"
<label class="col-form-label col-sm-3"
autofocus>
<span class="input-group-btn">
<button type="button"
class="btn btn-default"

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 12, 2019

Contributor

btn-default has been dropped.
We now have btn-primary (blue) btn-secondary (red) and btn-light ( white )

Suggested change
class="btn btn-default"
class="btn btn-light"
</button>
</span>
</div>
<span class="help-block"

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 12, 2019

Contributor
Suggested change
<span class="help-block"
<span class="invalid-feedback"
<cd-submit-button (submitAction)="onSubmit()"
[form]="userForm"
i18n="form action button|Example: Create Pool@@formActionButton"
type="button">

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 12, 2019

Contributor

This is not needed and will cause visual problems in some browsers.

Suggested change
type="button">
>
@@ -19,6 +19,15 @@
<strong>{{ username }}</strong></a>
</li>
<li class="dropdown-divider"></li>
<li role="menuitem"

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 12, 2019

Contributor
Suggested change
<li role="menuitem"
role="menuitem">
<a class="dropdown-item"
routerLink="/user-management/users/edit">
<i [ngClass]="[icons.lock, icons.width]"></i>

This comment has been minimized.

Copy link
@tspmelo

tspmelo Jul 12, 2019

Contributor

I have extended all fa icons with the fa-fw, so we don't need to manually add that anymore.

Suggested change
<i [ngClass]="[icons.lock, icons.width]"></i>
<i [ngClass]="[icons.lock]"></i>

@votdev votdev force-pushed the votdev:issue_40248 branch from e31fef0 to c8ce31e Jul 15, 2019

All comments are addressed.

@ricardoasmarques
Copy link
Member

left a comment

One last issue I found.

If I'm logged in with a user that doesn't have "read" permission on "user" scope, I can change the password (which is correct):

Screenshot from 2019-07-16 11-06-47

But if I click on the "Users" link in the breadcrumb I get a "Forbidden" error:

Screenshot from 2019-07-16 11-06-53

Perhaps, we should not have breadcrumb in this page (regardless of user permissions).

@rjfd

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

Perhaps, we should not have breadcrumb in this page (regardless of user permissions).

I think the problem is the context of that change password form that is wrong. Instead of being part of User Management > Users > Edit, this page should belong to the user profile context. Something like this: User Profile > Edit.

@votdev votdev force-pushed the votdev:issue_40248 branch from c8ce31e to ee80139 Jul 17, 2019

@votdev

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

Perhaps, we should not have breadcrumb in this page (regardless of user permissions).

I think the problem is the context of that change password form that is wrong. Instead of being part of User Management > Users > Edit, this page should belong to the user profile context. Something like this: User Profile > Edit.

I've relocated the password change dialog to /user-profile/edit. Maybe more user profile settings will be added in future.

@ricardoasmarques
Copy link
Member

left a comment

lgtm

mgr/dashboard: Add SSO guard service.
Signed-off-by: Volker Theile <vtheile@suse.com>

@rjfd rjfd referenced this pull request Jul 17, 2019

Closed

mgr/dashboard: Support minimum password complexity rules #28694

0 of 3 tasks complete

@ricardoasmarques ricardoasmarques merged commit caea8fe into ceph:master Jul 18, 2019

5 of 6 checks passed

make check (arm64) make check failed
Details
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
ceph dashboard tests ceph dashboard tests succeeded
Details
make check make check succeeded
Details

@votdev votdev deleted the votdev:issue_40248 branch Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.