Skip to content

[4.0] Redirect to com_admin when forcing TFA#30152

Merged
wilsonge merged 6 commits intojoomla:4.0-devfrom
SharkyKZ:j4/fix/twofactor-redirect
Jul 25, 2020
Merged

[4.0] Redirect to com_admin when forcing TFA#30152
wilsonge merged 6 commits intojoomla:4.0-devfrom
SharkyKZ:j4/fix/twofactor-redirect

Conversation

@SharkyKZ
Copy link
Copy Markdown
Contributor

@SharkyKZ SharkyKZ commented Jul 21, 2020

Fixes #30147.

Summary of Changes

Redirect to com_admin instead of com_users when forced TFA is enabled in backend.

Testing Instructions

Enable some Two Factor Authentication plugins.
Enable "Enforce Two Factor Authentication" for Backend
Create a new user with Access Level "Manager"
Login to backend with new user, setup two factor authentication

Actual result BEFORE applying this Pull Request

Message: "You don't have permission to access this. Please contact a website administrator if this is incorrect."

Expected result AFTER applying this Pull Request

Get redirected to user profile page where TFA can be set up properly.

Documentation Changes Required

No.

@Harmageddon
Copy link
Copy Markdown
Contributor

Just a short note for the testing instructions: Might be worth to mention there that one needs to enable one of the TFA plugins in order to run into this situation. ;-)

@ChristineWk
Copy link
Copy Markdown

Manager Level, before Patch, I got:
Notice
You were redirected because you are required to set up Two Factor Authentication to continue.
An error has occurred.
403 You don't have permission to access this. Please contact a website administrator if this is incorrect.

With Patch:
tfa_manager_only

Then I can't close/leave the session. Had to clear Browser Cache to be able to start again with /administrator entry.

@SharkyKZ
Copy link
Copy Markdown
Contributor Author

@Harmageddon Thanks, test instructions updated.

@le-jou @ChristineWk please test again.

@le-jou
Copy link
Copy Markdown

le-jou commented Jul 23, 2020

I have tested this item ✅ successfully on ba7c763

Works now, but finishes with warning "Warning
COM_USERS_USERS_ERROR_CANNOT_EDIT_OWN_GROUP"


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

@SharkyKZ
Copy link
Copy Markdown
Contributor Author

That's not intended.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@ChristineWk
Copy link
Copy Markdown

I have tested this item ✅ successfully on ba7c763

With Patch (Manager):
Notice
You were redirected because you are required to set up Two Factor Authentication to continue.
FYI: But I couldn't set up TFA (no experience with this)
@Harmageddon Thks for your assistance to be able to test this PR.


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

@SharkyKZ
Copy link
Copy Markdown
Contributor Author

PR updated. Please test again.

@le-jou
Copy link
Copy Markdown

le-jou commented Jul 24, 2020

I have tested this item ✅ successfully on 30103dd


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

@Harmageddon
Copy link
Copy Markdown
Contributor

This PR does solve the bug. But I'm not sure whether it is the best solution. Is there any particular reason why the save method of ProfileModel has to be independent of its parent? This implementation leads to much duplicated code and I can see issues similar to this one here arising when someone changes UserModel::save without remembering that the same changes have to be included in ProfileModel.

In my tests, reducing the ProfileModel::save method to the following seemed to work:

	public function save($data)
	{
		$user = Factory::getUser();
		$pk   = $user->id;
		$data['id'] = $pk;
		$data['block'] = $user->block;
		return parent::save($data);
	}

Or am I missing something? What do you think about it?

@toivo
Copy link
Copy Markdown
Contributor

toivo commented Jul 25, 2020

I have tested this item ✅ successfully on 30103dd

Tested successfully in Beta3-dev of 25 July.


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

@SharkyKZ
Copy link
Copy Markdown
Contributor Author

@Harmageddon I tried not to touch existing logic. With your code I get this when using super user account:

Save failed with the following error: You can't save a user account without selecting at least one user group.

@Harmageddon
Copy link
Copy Markdown
Contributor

@SharkyKZ Oh, I see. Okay, best then to proceed merging this PR here as-is, to fix the described bug.

I'm going to write a new PR for the reduction of duplicated code, so we can test it more in detail there.

@Harmageddon
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on 30103dd


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

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Jul 25, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 25, 2020
@wilsonge wilsonge merged commit 7064c48 into joomla:4.0-dev Jul 25, 2020
@wilsonge
Copy link
Copy Markdown
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 25, 2020
@wilsonge wilsonge added this to the Joomla 4.0 milestone Jul 25, 2020
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
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.

9 participants