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

Problem with password recovery functionality when Piwik has no sites defined #7073

Closed
mgazdzik opened this issue Jan 26, 2015 · 6 comments
Closed
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Milestone

Comments

@mgazdzik
Copy link
Contributor

Goal of this ticket is to improve Piwik functionality of password recovery in situation when there are no sites defined. Currently, it is affected by this piece of code https://github.com/piwik/piwik/blob/master/core/Plugin/Controller.php#L850 .
In some edge cases, this can virtually disable Piwik instance if one has forgoten his password, and has all sites deleted.
It is possible that other functionalities can also be affected by this bug.

Maybe this could be a beginning of new big feature, making Piwik independent of any site being present ? This would remove necessity to define site during installation.

@tsteur
Copy link
Member

tsteur commented Jan 26, 2015

Are you sure it is affected by this line? It doesn't do anything with sites. I tried to reproduce without a site but couldn't. Can you maybe describe step by step?

@tsteur
Copy link
Member

tsteur commented Jan 26, 2015

I can reproduce it after clicking on the activation link

@tsteur
Copy link
Member

tsteur commented Jan 26, 2015

Problem is after clicking on the activation link it wants to redirect to another URL here: https://github.com/piwik/piwik/blob/master/plugins/Login/Controller.php#L282

There it wants to create a URL to redirect but as no website is defined and it fails here: https://github.com/piwik/piwik/blob/master/core/Plugin/Menu.php#L191

In this case $websiteId = 0. We could check whether a site exists at all and if not, we ignore the exception but not sure about side effects. Also Piwik is currently designed that there is at least one site. To make the login work we could change the login controller to build the URL ourselves maybe? I don't see why it has to use URL::redirectToUrl in this case as only module & action is needed. (Obviously it is to remove duplicated code but in this case we could maybe redirect to the module directly)

tsteur added a commit that referenced this issue Jan 26, 2015
…ccess directly to prevent an error in case no website is defined
@tsteur
Copy link
Member

tsteur commented Jan 26, 2015

@diosmosis I committed a patch see f1ce174

I'm not sure with the latest login refactoring that was happening a couple of months ago. I presume this code should work as other Login plugins are supposed to extend the Login controller? And if they don't extend it, the change is still ok as it would never go into this method of the original Login controller (instead into the confirmResetPassword method of the custom Login plugin)?

@tsteur tsteur added the Bug For errors / faults / flaws / inconsistencies etc. label Jan 26, 2015
@tsteur tsteur added this to the Piwik 2.11.0 milestone Jan 26, 2015
@tsteur tsteur self-assigned this Jan 26, 2015
@diosmosis
Copy link
Member

@tsteur Login plugins have to extend from the controller as a practical reality, but I don't think they have to (or should). Ideally, the Login controller should be always activated, and other plugins should provide Auth implementations, PasswordResetters, etc. I don't think there are many plugins in the marketplace, though, so maybe we can do a quick check and see if it won't break anything.

@tsteur
Copy link
Member

tsteur commented Jan 26, 2015

True, they should ideally only provide auth implementation. I will have a look at the Marketplace but I can't think of any problem, it should be even better this way as we get rid of the doAsSuperUser(). BTW: We do not only have to care about Marketplace plugins. I presume there are many plugins out there used in companies etc that are not public.

  • If a plugin implements it's own controller with a custom confirmResetPassword method and does not extend Piwik's Login Controller it does not matter anyway as our code will never be executed
  • If a plugin implements it's own controller with a custom resetPasswordSuccess method and does not extend Piwik's Login Controller the reset password would not work anyway as it would never execute confirmResetPassword of our Login controller. It would require the plugin to also implement confirmResetPassword which makes everything fine again
  • If a plugin extends our Login controller everything is fine anyway and nothing changes. They can overwrite the resetPasswordSuccess method if they want to but they don't have to. In any way it would work.
  • Ideally they only provide a different auth adapter which is officially supported. Our controllers are not a public API so far unless they are tagged with @api. Nonetheless it would still work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Development

No branches or pull requests

3 participants