Skip to content

If plugins return false when login, should mark as failure.#22936

Merged
HLeithner merged 5 commits intojoomla:stagingfrom
asika32764:patch-3
Jun 5, 2019
Merged

If plugins return false when login, should mark as failure.#22936
HLeithner merged 5 commits intojoomla:stagingfrom
asika32764:patch-3

Conversation

@asika32764
Copy link
Copy Markdown
Contributor

@asika32764 asika32764 commented Nov 4, 2018

Move the return line into if block.

I found that if plugin return FALSE from onUserBeforeLogin, Application will still return true. This situation should mark as failure and goto onUserLoginFailure.

SharkyKZ
SharkyKZ reviewed Nov 4, 2018
View reviewed changes
libraries/src/Application/CMSApplication.php Outdated

// The user is successfully logged in. Run the after login events
$this->triggerEvent('onUserAfterLogin', array($options));

Copy link
Copy Markdown
Contributor

@SharkyKZ SharkyKZ Nov 4, 2018

Choose a reason for hiding this comment

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

Suggested change

@bembelimen
Copy link
Copy Markdown
Contributor

bembelimen commented Nov 19, 2018 •
edited
Loading

Did you check what happens if one login works (e.g. the default Joomla! login) and another fails...? Is the user logged in after?

@asika32764
Copy link
Copy Markdown
Contributor Author

asika32764 commented Nov 19, 2018 •
edited
Loading

Must test in a custom plugin or custom login code. This issue will only happened if a developer attempts to use $app->login() to make their own login flow in 3rd party components.

I'm still thinking how to attach simple code to reproduce this issue.

@ghost ghost added J3 Issue and removed J3 Issue labels Apr 5, 2019
@ghost ghost changed the title [Fix] If plugins return false when login, should mark as failure. If plugins return false when login, should mark as failure. Apr 19, 2019
@ghost ghost removed the J3 Issue label Apr 19, 2019
@ghost
Copy link
Copy Markdown

ghost commented Apr 24, 2019

@asika32764 whats the state of this Pull Request?

@SharkyKZ
Copy link
Copy Markdown
Contributor

SharkyKZ commented Apr 25, 2019

To reproduce the issue in core:

Enable System - User Log plugin.
Try to login to administrator area with unauthorized user (should see You do not have access to the Administrator section of this site. message).
Check administrator/logs/error.php log. The login failure should be logged but it's not.

@SharkyKZ
Copy link
Copy Markdown
Contributor

SharkyKZ commented Apr 25, 2019

I have tested this item ✅ successfully on c6fccec

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22936.

@ghost
Copy link
Copy Markdown

ghost commented Apr 25, 2019

@SharkyKZ thanks for the clear Test Instructions. At CloudAccess i have to upgrade to unlock Logs, so can't test.

@asika32764
Copy link
Copy Markdown
Contributor Author

asika32764 commented Apr 25, 2019

Thanks @SharkyKZ

I'm trying to create a plugin to reproduce this issue but your way is more simpler to test.

@Quy
Copy link
Copy Markdown
Contributor

Quy commented May 8, 2019

I have tested this item ✅ successfully on e5a7c2d

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22936.

@Quy
Copy link
Copy Markdown
Contributor

Quy commented May 8, 2019

RTC

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22936.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 8, 2019
@HLeithner HLeithner merged commit f60f63a into joomla:staging Jun 5, 2019
@HLeithner
Copy link
Copy Markdown
Member

HLeithner commented Jun 5, 2019

thx

@HLeithner HLeithner added this to the Joomla 3.9.7 milestone Jun 5, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@SharkyKZ SharkyKZ SharkyKZ left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Joomla 3.9.7

Development

Successfully merging this pull request may close these issues.

8 participants

@asika32764 @bembelimen @SharkyKZ @Quy @HLeithner @joomla-cms-bot @infograf768 @SniperSister