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

Add option to change password #8

Merged
merged 7 commits into from Oct 1, 2018

Conversation

Projects
None yet
3 participants
@leportella
Copy link
Contributor

leportella commented Sep 25, 2018

This implements a new url where the logged user can change the original password to a new one.

This was done based on comments made on this issue.

To test it (manually):

  • install this version
  • login to jupyterhub
  • go to /hub/auth/change-password
  • create a new password
  • logout and login again with the new password

@leportella leportella changed the title Reset password option Add option to change password Sep 25, 2018

@minrk
Copy link
Member

minrk left a comment

This looks great, thanks!

html = self.render_template('reset.html')
self.finish(html)

async def post(self):

This comment has been minimized.

@minrk

minrk Sep 26, 2018

Member

This method will also want the @web.authenticated decorator to ensure that the user is logged in before proceeding

This comment has been minimized.

@leportella

leportella Sep 26, 2018

Contributor

just added it :)

@yuvipanda
Copy link
Member

yuvipanda left a comment

Woohoo, this looks like a great PR! I've suggested some design changes, so take a look and LMK what you think.

I'm gonna set this up and test it locally now!

@web.authenticated
async def post(self):
data = {}
for arg in self.request.arguments:

This comment has been minimized.

@yuvipanda

yuvipanda Sep 26, 2018

Member

We only need the user's new password here. It should also be a body parameter only (comes in the body of the request), rather than a query parameter (comes in URL, like &password=new). This is important to reduce the attack surface for user's passwords being reset without their knowledge.

So, I'd recommend:

  1. Using get_body_argument instead of get_argument
  2. Explicitly only checking for the password field
  3. Change the reset_password method in FirstUseAuthenticator to take a user object & password string, rather than a dictionary 'data'.

This comment has been minimized.

@leportella

leportella Sep 26, 2018

Contributor

I did this, and indeed it got much better.. much cleaner! Thank you for your feedback

install_requires=['bcrypt']
install_requires=['bcrypt'],
package_data={
'': ['*.html'],

This comment has been minimized.

@yuvipanda

yuvipanda Sep 26, 2018

Member

I didn't know you could leave the key empty! I learnt something new today :)

This comment has been minimized.

@leportella

leportella Sep 26, 2018

Contributor

just followed the example that I saw here :)

@yuvipanda
Copy link
Member

yuvipanda left a comment

I left some style change comments, otherwise looks great to me! \o/

{% block main %}

<div class="container">
<form action="{{post_url}}" method="post" role="form">

This comment has been minimized.

@yuvipanda

yuvipanda Sep 26, 2018

Member

We're using Bootstrap 3.3, rather than Bootstrap 4. I think this requires different class annotations for forms to get proper styling. Can you switch it to using horizontal forms, as seen in: https://getbootstrap.com/docs/3.3/css/#forms?


<div class="container">
<form action="{{post_url}}" method="post" role="form">
<h2 class="auth-form-header">

This comment has been minimized.

@yuvipanda

yuvipanda Sep 26, 2018

Member

Since we aren't explicitly styling these, these class attributes can probably be removed.

<h2 class="auth-form-header">
Change Password
</h2>
<div class='auth-form-body'>

This comment has been minimized.

@yuvipanda

yuvipanda Sep 26, 2018

Member

Since we aren't explicitly styling these, these class attributes can probably be removed.

{% block main %}

<div class="container">
<form action="{{post_url}}" method="post" role="form">

This comment has been minimized.

@yuvipanda

yuvipanda Sep 26, 2018

Member

Can you indent this as well, so it's clearer when the container div starts and ends?

html = self.render_template(
'reset.html',
result=True,
result_message='password changed successfully',

This comment has been minimized.

@yuvipanda

yuvipanda Sep 26, 2018

Member

Can you change the message to say `your password has been changed successfully'?

/>
</div>
</form>
{% if result %}

This comment has been minimized.

@yuvipanda

yuvipanda Sep 26, 2018

Member

Can you render this using a 'success' alert, like https://getbootstrap.com/docs/3.3/components/#alerts? That would make it more obvious I think.

@leportella

This comment has been minimized.

Copy link
Contributor

leportella commented Sep 26, 2018

@yuvipanda I think I did all the changes you asked for. Can you check if everything is ok now? :) Thanks!

@leportella

This comment has been minimized.

Copy link
Contributor

leportella commented Sep 30, 2018

@minrk I beliebe that @yuvipanda may be busy. Could you please take a look at this when you can, please?
Thanks!

@yuvipanda

This comment has been minimized.

Copy link
Member

yuvipanda commented Oct 1, 2018

Apologies for the delayed review, @leportella! This looks good to me!

Congratulations on getting the PR merged!

@yuvipanda yuvipanda closed this Oct 1, 2018

@yuvipanda yuvipanda reopened this Oct 1, 2018

@yuvipanda yuvipanda merged commit 70f831e into jupyterhub:master Oct 1, 2018

@leportella leportella deleted the leportella:reset-password-option branch Oct 2, 2018

@leportella

This comment has been minimized.

Copy link
Contributor

leportella commented Oct 2, 2018

Thank you @yuvipanda !! Glad to do it

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