Skip to content

Member-Password is encrypted twice #17

Closed
nebucaz opened this Issue Jan 26, 2013 · 9 comments

3 participants

@nebucaz
nebucaz commented Jan 26, 2013

Dashboard Module seems to encrypt Member-Password so that member is not able to login. Also, unit-tests using basic-auth do not work, after dashboard module was installed.
Explanation and stack-trace: http://www.silverstripe.org/general-questions/show/22132#post319518

@unclecheese
Owner

What do you mean by "a member is not able to login"? How can I replicate this issue? I have the module installed on countless sites, and I've never heard of anyone not being able to login.

@nebucaz
nebucaz commented Jan 27, 2013

@unclecheese I wrote a simple test you can find here: https://github.com/nebucaz/ssdashboardtest
The test runs successfully as long as dashboard is not installed. To reproduce: 1) create new silverstripe-project and install ssdashboardtest 2) /dev/build?flush=1 3) run /dev/tests/ServiceTest --> Success 4) install Dashboard 5) /dev/build?flush=1 6) run /dev/tests/ServiceTest --> Failed asserting that 401 matches expected 200.

@unclecheese
Owner

Ah, so the issue is just that it fails a test. It's not actually affecting any real-life use cases?

@nebucaz
nebucaz commented Jan 27, 2013

Yeah right, this is a test. It helps to demonstrate and reproduce the issue. But actually, there is a real-life use case - at least in my project: The "TestService" is kind of Restful Server which "basic authenticates" the clients whose connection-attempt is simulated by the test. Do you think, there is a better way to track-down and solve the issue?
I also am happy to learn how and where to correct my code. Did you already have a look at the forum-post with the stack-trace?

@unclecheese
Owner

OK, I just wanted to make sure, because I've seen cases before where the test throws false positives.

Just a hunch, here -- the only intersection I can see between the Dashbaord module and authentication is in the onAfterWrite function of the DashboardMember class. Try the following:

Change

<?php
$this->owner->write();

To

<?php
Member::get()->byID($this->owner->ID)->write();

Wondering if the password won't be encrypted in a separate write execution, as instantiated by a fresh query.

If that doesn't work, try changing the onAfterWrite() function to onBeforeWrite(), and remove the line:

<?php
$this->owner->write();
@nebucaz
nebucaz commented Jan 27, 2013

Changing line 49 of DashboardMember.php as described above makes the test pass. However, I did not (yet) test if the Dashboard still works as expected. Could you explain the difference between

$this->owner->write(); and Member::get()->byID($this->owner->ID)->write();?

@dospuntocero
Collaborator

ohh... so this was the module that was changing passwords of my users... xd!! i was blaming ingo and the complete silverstripe team!!!

remember aaron that i told you about the password problem some weeks ago?, well when you create a user, the password its encoded twice and the user wont be able to access to the cms until you change the password in the security tab, after that change you will be able to access with that user. you can test this problem in any of our dcclab projects.

create an user in the security tab and log out / try to access with him.

@unclecheese
Owner

Kind of surprised it works, actually. The thinking was that onAfterWrite() is calling write() itself, which is restarting the write process. If there is a flag in the Member class telling it to encrypt a password on $this->isChanged("Password"), for instance, that will fire a second time, because the write technically never completed.

By getting a fresh member record, it knows nothing about the previous activity. The pasword field isn't dirtied, so the write function works normally.

As a matter of best practice, calling $this->write() in onAfterWrite() is a bad idea, because it leads to recursive weirdness. You can open up a separate write execution by just instantiating a new record in the manner above.

@unclecheese
Owner

Resolved in 45cab75

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.