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: Force change the password #28405

Closed
wants to merge 1 commit into from

Conversation

@dziomdziorae
Copy link

commented Jun 5, 2019

Forcing changing password:
With creation of the user there is new flag, which is called needChangePassword. If the flag is true it navigates the user for changing password.

Fixes: https://tracker.ceph.com/issues/24655
Signed-off-by: Elzbieta Dziomdziora elzbieta.dziomdziora@ts.fujitsu.com

mgr/dashboard: Force change the password
Fixes: https://tracker.ceph.com/issues/24655
Signed-off-by: Dziomdziora, Elżbieta <elzbieta.dziomdziora@ts.fujitsu.coom>

@dziomdziorae dziomdziorae force-pushed the dziomdziorae:ticket-24655 branch from 5dfc099 to 61ba70b Jun 5, 2019

@LenzGr LenzGr added the feature label Jun 6, 2019

@LenzGr LenzGr requested review from rjfd, tspmelo, votdev, epuertat, alfonsomthd and bk201 Jun 6, 2019

@LenzGr

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

make check currently fails in the run-tox-mgr-dashboard test. There is a script run-tox.sh in the dashboard source directory that you can run to verify the Python code. Can you please address the issues that have been reported by the script? Thank you!

@dziomdziorae

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

Hello,
I will add as soon as I can. I have some problems with docker and runing the script.

@epuertat
Copy link
Contributor

left a comment

Thanks for your contribution!

I have a few comments/requests here. I'm aware that part of them emanate from the existing design of the auth controller, but it'd be a chance to start improving the overall design. Any doubt you have around my comments, do not hesitate to ask.

@@ -79,6 +84,10 @@ def set(self, username, password=None, name=None, email=None, roles=None):
user = mgr.ACCESS_CTRL_DB.get_user(username)
except UserDoesNotExist:
raise cherrypy.HTTPError(404)
if not password and not name and not email and (user.to_dict())['roles'] == roles:

This comment has been minimized.

Copy link
@epuertat

epuertat Jun 10, 2019

Contributor

Is it not the same as this (I mean without the to_dict() step)?

Suggested change
if not password and not name and not email and (user.to_dict())['roles'] == roles:
if not password and not name and not email and user.roles == roles:

This comment has been minimized.

Copy link
@dziomdziorae

dziomdziorae Jun 17, 2019

Author

No, because user.roles contains the object and address, but to_dict() returns actual string value.

@@ -48,15 +48,19 @@ 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.permission,
Boolean(this.authService.getNeedChangePassword(login.username)));

This comment has been minimized.

Copy link
@epuertat

epuertat Jun 10, 2019

Contributor

If you define the method as returning Boolean you don't need to type-cast here.

return this.http
.post('api/auth/getNeedChangePassword', {username: username})
.toPromise()
.then((resp: any) => {

This comment has been minimized.

Copy link
@epuertat

epuertat Jun 10, 2019

Contributor

As we have defined a type LoginResponse, why not enforcing that instead of using any?

if(localStorage.getItem('need_change_password') == 'false'){
return;
}
return this.router.navigate(['/user-management/users/edit/'+ this.getUsername()]);;

This comment has been minimized.

Copy link
@epuertat

epuertat Jun 10, 2019

Contributor

I'm curious here: if the user doesn't (want to) change their password, and manually reset LocalStorage, would this policy be enforced by the back-end as well, or it's just a one-time thing (at login step)?

self.needChangePassword = not self.needChangePassword
self.refreshLastUpdate()

def set_roles(self, roles):

This comment has been minimized.

Copy link
@epuertat

epuertat Jun 10, 2019

Contributor

This is not syntactically correct in Python. A void function needs at least an statement (pass).

Suggested change
def set_roles(self, roles):
def set_roles(self, roles): pass

However, that will silently fail, so it's usually better to trigger an Exception like NotImplementedError.

This comment has been minimized.

Copy link
@dziomdziorae

dziomdziorae Jun 11, 2019

Author

I will remove it, this declaration was copied by mistake.

self.update_needChangePassword()

def update_needChangePassword(self):
self.needChangePassword = not self.needChangePassword

This comment has been minimized.

Copy link
@epuertat

epuertat Jun 10, 2019

Contributor

IMHO this is rather tricky. This basically flip-flops between True and False, so you cannot set the desired state of the flag. Why not explicitly setting it to a specific value?

Optionally, as this is basically a setter-like method, why not using @property:

@property
def change_password(self):
    return self._change_password
 
@change_password.setter
def change_password(self, value):
  self._change_password = value
  self.refreshLastUpdate()
login(credentials: Credentials) {
getNeedChangePassword(username: string) {
return this.http
.post('api/auth/getNeedChangePassword', {username: username})

This comment has been minimized.

Copy link
@epuertat

epuertat Jun 10, 2019

Contributor

getNeedChangePassword does not follow REST conventions for naming Resources: the action get is stated by the HTTP method (GET) so no need to include it in the name, and regarding NeedChangePassword, it's rather a property of a User (the actual Resource here), than a resource by itself (GET /api/auth/user/user_id). However, I'm not sure if this issue comes from this very PR or it's already present in the design of this controller.

@@ -81,3 +82,13 @@ def check(self, token):
return {
'login_url': self._get_login_url()
}

@RESTController.Collection('POST')
def getNeedChangePassword(self, username):

This comment has been minimized.

Copy link
@p-na

p-na Jun 11, 2019

Contributor

Looks like the method name should be written in snake_case rather then lowerCamelCase.

@@ -48,6 +48,11 @@ def create(self, username=None, password=None, name=None, email=None, roles=None
raise DashboardException(msg='Username is required',
code='username_required',
component='user')
if not password:
raise DashboardException(msg='Password is required',

This comment has been minimized.

Copy link
@ricardoasmarques

ricardoasmarques Jun 13, 2019

Member

We cannot make the password field required because we may be using Single-Sign-On

This comment has been minimized.

Copy link
@dziomdziorae

dziomdziorae Jun 17, 2019

Author

@ricardoasmarques ok, but what if someone will create user without password? Then it is not possible to login on that user.

This comment has been minimized.

Copy link
@ricardoasmarques

ricardoasmarques Jun 17, 2019

Member

@dziomdziorae If the user doesn't have a password, he will not be able to authenticate "locally" [1], he will only be able to authenticate "externally" (SSO) [2].

[1] "locally" - using ceph dashboard internal user database
[2] "externally" - using an external identity provider when Single-Sign-On is enabled - http://docs.ceph.com/docs/master/mgr/dashboard/#enabling-single-sign-on-sso

This comment has been minimized.

Copy link
@dziomdziorae

dziomdziorae Jun 17, 2019

Author

Ok, thank you for explanation

@dziomdziorae

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

@epuertat Can you add description, what exactly should be added/changed in the ticket description (https://tracker.ceph.com/issues/24655) according to newest agreement on authentication?
I will start changes for your current comments.

@tspmelo

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

@dziomdziorae @epuertat I added a comment [1] to the tracker issue.
Please review it and let me know what you think.

[1] https://tracker.ceph.com/issues/24655#note-7

@dziomdziorae

This comment has been minimized.

Copy link
Author

commented Jun 17, 2019

Looks good, I will do that :)

@@ -181,13 +181,26 @@ def __init__(self, username, password, name=None, email=None, roles=None,
self.refreshLastUpdate()
else:
self.lastUpdate = lastUpdate
self.needChangePassword = needChangePassword

This comment has been minimized.

Copy link
@s0nea

s0nea Jul 3, 2019

Member

I do have a general question on how to recognize if the password needs to be changed or not.

With regards to http://tracker.ceph.com/issues/40329 which is about a password expiration time-frame, I'm wondering if it would be possible to use lastPasswordUpdate field instead of needChangePassword. The lastPasswordUpdate field would be a time field similar to lastUpdate. After the user creation the field can be set to None to make sure the user changes the password at his first login. After that the lastPasswordUpdate will be set to the current time.
This would help me with implementing the password expiration feature because I don't need to specify a separate field for that.

It's just an idea. What do you think? Maybe there is an issue with my proposal I currently don't see.

This comment has been minimized.

Copy link
@dziomdziorae

dziomdziorae Jul 31, 2019

Author

Yes, I think it sounds good.

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

@dziomdziorae

This comment has been minimized.

Copy link
Author

commented Aug 6, 2019

Updated in PR 29505

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