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

cleanup password after each login attempt #29

Closed
wants to merge 5 commits into from

Conversation

gdlx
Copy link

@gdlx gdlx commented Nov 5, 2013

I've noticed that after a logout or unsuccessful login attempt, the password field wasn't empty when going back to the login page.

I think it is unsecure to keep the password in memory after a login attempt so I submit this PR that just cleanup the password variable (and bound form input).

@@ -58,6 +58,7 @@ Ember.SimpleAuth.LoginControllerMixin = Ember.Mixin.create({
login: function() {
var self = this;
var data = this.getProperties('identification', 'password');
self.set('password', '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better set to undefined

@marcoow
Copy link
Member

marcoow commented Nov 5, 2013

sounds good; can you fix the build and add a test case?

@gdlx
Copy link
Author

gdlx commented Nov 6, 2013

I'm not comfortable with PhantomJS testing yet and I can't run tests locally.

bundle exec rake test hangs here:

# bundle exec rake test
Building Ember.SimpleAuth...
Done
which phantomjs > /dev/null 2>&1
Git Support Debugging Info:
  commit_range: c0efa9f0d52985060508b1fd3aa05ee5ee05b349...c0efa9f0d52985060508b1fd3aa05ee5ee05b349
  current_tag:
  current_branch: clean-password
  current_revision: c0efa9f0d52985060508b1fd3aa05ee5ee05b349
  commits: {}
Running tests on master
Puma 2.6.0 starting...
* Min threads: 0, max threads: 16
* Environment: development
* Listening on tcp://0.0.0.0:60099

Running: {"package":"all"}

Any idea ?

@marcoow
Copy link
Member

marcoow commented Nov 6, 2013

Hm, do you have PhantomJS installed? Also you could try running

bundle exec rackup

from the repository root and then open the URL it prints out in the browser and run the specs there.

@gdlx
Copy link
Author

gdlx commented Nov 6, 2013

That's ok. I was using phantomjs installed by npm. Using the website bin works fine.

@gdlx
Copy link
Author

gdlx commented Nov 8, 2013

About the broken test, I really don't understand why the overriden tokenRequestOptions isn't called anymore when clearing the password variable.

Could I clear the variable anywhere in the login action, the default tokenRequestOptions will be called instead of the overridden one in the test...

Did you already meet this behavior ?

@marcoow
Copy link
Member

marcoow commented Nov 11, 2013

The problem is that password is set to undefined when login is called the first time in line 54 and then wham it's called the 2nd time in line 67 it's still undefined and nothing actually happens as

if (!Ember.isEmpty(data.identification) && !Ember.isEmpty(data.password)) {
  ...

is false. You'd need a

testController.set('password', 'password');

before the 2nd invocation.

Also, it would be better to only unset password inside of the if block.

marcoow pushed a commit that referenced this pull request Nov 14, 2013
@marcoow marcoow closed this Nov 14, 2013
@gdlx
Copy link
Author

gdlx commented Nov 15, 2013

I've seen you've made the change.

Thank you ! (I'll delete the branch).

@gdlx gdlx deleted the clean-password branch November 15, 2013 10:42
marcoow pushed a commit that referenced this pull request Feb 8, 2018
* update libraries

* Remove unused "ember-welcome-page" addon

* package.json: Add "mocha" script

... to fix CI builds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants