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

Bonfire password #427

Merged
merged 8 commits into from May 17, 2012

Conversation

Projects
None yet
3 participants
@jfox015
Copy link
Contributor

commented May 10, 2012

Revamped Password Strength addition. Contains

  • Modified password_strength JS lib with Bonfire custom options applied
  • Custom Twitter bootstrap labels
  • Settings option to use or not use labels/front end validation
@seandowney

This comment has been minimized.

Copy link
Member

commented May 10, 2012

Hey Jeff

That looks excellent but if you could change the migration stuff to a new migration file that would be best.

This will ensure that people on the latest will upgrade safely.

I haven't tried it out yet but it looks excellent and simple to understand which is always good.

Thanks for this

@jfox015

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2012

@seandowney
I had a feeling you guys would want that but thought I'd trying grouping it with the other options just in case. Will commit a new migration later today. Thanks for the feedback. Hope people find this useful.

@seandowney

This comment has been minimized.

Copy link
Member

commented May 10, 2012

He he no worries. Yeah I think people will love it.

@seandowney

This comment has been minimized.

Copy link
Member

commented May 10, 2012

@jfox015 Just to make you aware I pushed a new migration file now so your file will need to be 028_xxxx.php

Thanks

@jfox015

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2012

@seandowney
Updated and named 028.

@seandowney

This comment has been minimized.

Copy link
Member

commented May 10, 2012

@jfox015 It's looking good (during testing I found a different bug !)

I did notice that when I had min length 8 and selected:

  • to force Numbers - it still said Weak even when the text I entered should have satisfied it
  • to force Symbols - it still said Weak even when the text I entered should have satisfied it

Forcing Mixed Case worked as expected

Maybe there are settings that need tweaking?

@jfox015

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2012

Will investigate further. Stay tuned.

@seandowney

This comment has been minimized.

Copy link
Member

commented May 10, 2012

@jfox015 cool thanks

//cc @svizion //cc @lonnieezell Could one of ye take this over to test as I won't be available for the rest of the weekend and this is really close to being ready to merge.

@svizion

This comment has been minimized.

Copy link
Contributor

commented May 11, 2012

I'll test it out this weekend looks like a nice addition

Added email validation
Fixed optional tests to be less negative
Only execute username or email validation if values are found
@jfox015

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2012

@seandowney @svizion
I tweaked the testing code to reduce the negative impacts of the test a bit. I also hooked up email validation which tests the password against the email field if the username setting is set to false. Tests look good and the password comes back strong when they follow the rules of each option setting. Try it out and let me know if you run into any further issues.

@svizion

This comment has been minimized.

Copy link
Contributor

commented May 15, 2012

@jfox015 I tried this out and couldn't get the java-script validation to display anything other then "weak" on either a password or the password confirm prompts. There were no error's shown in Chrome's console and I didn't really have a lot time this weekend to fully go through the code and see what could have caused the issue.

I used a blank development branch of Bonfire, but I used git to merge it so that could have caused a issue I didn't see.

I've re-try it out on a different install by hand merging it later this week,

I really like the idea of it and good work on it, I think my issues might have been either merge related or possibly some other issue on my test server.

@jfox015

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2012

@svizion That's strange. The tests should't change because of any browser or OS. If you do a manual merge and still find it being weak, try dropping a console.log on line 38 of Password_Strength.js and let me know if you have any weird negative scores past 300. Also, do you have use_username selected? I tried both ways myself but that one test could cause some wonkiness if it's testing both username and password for some reason.

@svizion

This comment has been minimized.

Copy link
Contributor

commented May 16, 2012

Ok after manually merging the changes in, and using the views/users/user_js view the Passwords Match, and will change. Apparently the way I pulled the pull-request and merged it only pulled the first commit, not the latest commit to the pull-request. Seems to work fairly well, not sure the rules on there validation strength and haven't tested the email/use_username yet, but I got things working, I'll have to finish up the rest of the manual merging tomorrow as it's getting late here, not enough time in a day.

@jfox015

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2012

@svizion Cool. Let me know if anything breaks or fails. So far so good on my end.

@svizion

This comment has been minimized.

Copy link
Contributor

commented May 17, 2012

/cc @jfox015 @seandowney @lonnieezell Everything is working solid now, it's a good addition

@jfox015 I had to add 2 new migrations so I think you might need to rename your migration file to 30_ now to match up with the newer migrations.

@jfox015

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2012

WOOT! I will update the migration # and commit later today. No more incrementing it until you guys merge this in though. :P

@svizion

This comment has been minimized.

Copy link
Contributor

commented May 17, 2012

Sorry about the migration #'s , I've had to hand merge it into my Blank Development site and a Live site and everything is functioning well and good work on creating this addition. Thanks for all the contributions you have made :)

@jfox015

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2012

@seandowney @svizion
Push a new update and renamed the migration to 30.

seandowney added a commit that referenced this pull request May 17, 2012

Merge pull request #427 from jfox015/bonfire_password
Validate password strength - added by @jfox015

@seandowney seandowney merged commit cccffe9 into ci-bonfire:develop May 17, 2012

@seandowney

This comment has been minimized.

Copy link
Member

commented May 17, 2012

@jfox015 nice work it's great to have it integrated

@svizion thanks for the extra testing

@jfox015

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2012

/cc @seandowney @svizion @lonnieezell Thanks guys. Excited to be contributing!

@seandowney

This comment has been minimized.

Copy link
Member

commented May 17, 2012

@jfox015 actually I just noticed that the strength is giving different results on the frontend from the backend.

I tried without any of the number, special char or case settings and put in a basic 5 letter and 4 digit password.

On the front end - 5 letters + 3 digits was good with 5 letter + 4 digit strong

On the back end - 5 letters + 3 digits was good with 5 letter + 4 digit good, 5 letter + 5 digit was strong

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.