Skip to content

Allow the statistics plugin to run on the captive page#38436

Merged
fancyFranci merged 2 commits intojoomla:4.2-devfrom
roland-d:feature/allow-statistics-plugin-to-run-on-mfa
Aug 13, 2022
Merged

Allow the statistics plugin to run on the captive page#38436
fancyFranci merged 2 commits intojoomla:4.2-devfrom
roland-d:feature/allow-statistics-plugin-to-run-on-mfa

Conversation

@roland-d
Copy link
Copy Markdown
Contributor

@roland-d roland-d commented Aug 10, 2022

Summary of Changes

When a user is using multi-factor authentication to login it may show some HTML in the message box. This is the result of a failed check on the multi-factor authentication because the Statistics plugin is trying to send data to the Joomla server On the captive page a user is already logged-in but it has not yet completed the multi-factor authentication. Duh, we are trying to complete that page when the "message" shows up. This is part of what a user will see:
image

The page can still be used but it does not look pretty.

Testing Instructions

This assumes you have setup one or more multi-factor authtentications

  1. Enable the Statistics plugin
  2. Open the file plugins/system/stats/stats.php
  3. Uncomment line 24 so it says define('PLG_SYSTEM_STATS_DEBUG', 1);
  4. Log out of the website
  5. Login by entering a username and password
  6. You now get to the captive page and soon after the image shown in the summary will popup
  7. You can click on Select a different method in the toolbar and you go to the page to select which multi-factor authentication you want to use but again the message popup will show
  8. Apply the patch
  9. Refresh or repeat steps 5 - 7. You should no longer see the popups

Actual result BEFORE applying this Pull Request

You get some HTML output in the message box

Expected result AFTER applying this Pull Request

No message box is shown

Documentation Changes Required

None

@roland-d
Allow the statistics plugin to run on the captive page
d3b2eea 
Signed-off-by: Roland Dalmulder <contact@rolandd.com>
@roland-d roland-d requested a review from HLeithner August 10, 2022 17:22
@HLeithner
Copy link
Copy Markdown
Member

HLeithner commented Aug 10, 2022

@nikosdion can you have a look please

@nikosdion
Copy link
Copy Markdown
Contributor

nikosdion commented Aug 10, 2022

I'm on vacation until the 22nd BUT I can tell you that how @roland-d did it is exactly what I'd do. So it's okay as far as I am concerned.

Another way to do it — best practice for 3PDs actually — is for the plugin to check if we're in a captive page and not execute. I am leaving this note not as something applicable to this PR but as something that a 3PD might stumble on. The rabbit hole will lead them to this comment here and they will figure out what to do next ;)

@HLeithner
Copy link
Copy Markdown
Member

HLeithner commented Aug 10, 2022

I'm on vacation until the 22nd BUT I can tell you that how @roland-d did it is exactly what I'd do. So it's okay as far as I am concerned.

Another way to do it — best practice for 3PDs actually — is for the plugin to check if we're in a captive page and not execute. I am leaving this note not as something applicable to this PR but as something that a 3PD might stumble on. The rabbit hole will lead them to this comment here and they will figure out what to do next ;)

thx

@N6REJ
Copy link
Copy Markdown
Contributor

N6REJ commented Aug 11, 2022

I'm unable to test this as the mult-factor authentication setup is failing. I tried turning off EVERY plugin except "fixed" but that still didn't change anything
image

@nikosdion
Copy link
Copy Markdown
Contributor

nikosdion commented Aug 11, 2022

@N6REJ It looks like your site's database schema is out of date or otherwise borked. Try installing a fresh copy of Joomla 4.2 from the nightlies and apply this patch.

@bayareajenn
Copy link
Copy Markdown

bayareajenn commented Aug 11, 2022

@N6REJ It looks like your site's database schema is out of date or otherwise borked. Try installing a fresh copy of Joomla 4.2 from the nightlies and apply this patch.

I get the same error in a fresh 4.2.0 build, no extensions installed, patch installed. It appears to work until I go back into to user profile and Save or Save & Close. Then I get the same error shown in @N6REJ 's post.

@brianteeman
Copy link
Copy Markdown
Contributor

brianteeman commented Aug 11, 2022 •
edited
Loading

its the user actions log setting and nothing to do with the authentication plugins.

@bayareajenn
Copy link
Copy Markdown

bayareajenn commented Aug 11, 2022

its the user actions log setting and nothing to do with the authentication plugins.

But I didn't change any settings. The default settings on a fresh build make this error happen. Regardless, it seems like we need to tell people this will happen and what to do about it. I'll take it up with the release team.

@roland-d
Copy link
Copy Markdown
Contributor Author

roland-d commented Aug 11, 2022

So I had a look at this finding and it is nothing related with this PR, so we should move it to it's own. It is indeed because of the actionlogs plugin which acts on the trigger onUserAfterSave and expects the $user array that is supplied to contain $user['actionlogs']['actionlogsExtensions'], to set that as extension, which it doesn't. So extension remains empty and this throws the error we see here. Going to see what I can do as a PR.

@brianteeman
Copy link
Copy Markdown
Contributor

brianteeman commented Aug 11, 2022

Regardless, it seems like we need to tell people this will happen and what to do about it.

No. its a bug that needs fixing

@bayareajenn
Copy link
Copy Markdown

bayareajenn commented Aug 11, 2022

Regardless, it seems like we need to tell people this will happen and what to do about it.

No. its a bug that needs fixing

Yup. Roland found it (as seen in a comment). Thanks.

@roland-d
Copy link
Copy Markdown
Contributor Author

roland-d commented Aug 11, 2022

PR created: #38439

@N6REJ
Copy link
Copy Markdown
Contributor

N6REJ commented Aug 12, 2022

@N6REJ It looks like your site's database schema is out of date or otherwise borked. Try installing a fresh copy of Joomla 4.2 from the nightlies and apply this patch.

that was a clean 4.2-dev build.

@brianteeman
Copy link
Copy Markdown
Contributor

brianteeman commented Aug 12, 2022

PR created: #38439

That is not a correct fix

@bayareajenn
Copy link
Copy Markdown

bayareajenn commented Aug 12, 2022

I have tested this item ✅ successfully on d3b2eea

Tests great. Thanks, Roland.

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

@webgras
Copy link
Copy Markdown
Contributor

webgras commented Aug 12, 2022

I have tested this item ✅ successfully on d3b2eea

Tested successfully on localhost/php 8.1

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

@richard67
Copy link
Copy Markdown
Member

richard67 commented Aug 13, 2022

RTC as it has 2 successful human tests which were invalidated by a clean branch update only.

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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 13, 2022
@fancyFranci fancyFranci merged commit 819f18b into joomla:4.2-dev Aug 13, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 13, 2022
@fancyFranci
Copy link
Copy Markdown
Contributor

fancyFranci commented Aug 13, 2022

Merged, thank you!

@fancyFranci fancyFranci added this to the Joomla! 4.2.0 milestone Aug 13, 2022
@heelc29 heelc29 mentioned this pull request Aug 16, 2022
Closed
@Fedik Fedik mentioned this pull request Aug 17, 2022
Merged
@roland-d roland-d deleted the feature/allow-statistics-plugin-to-run-on-mfa branch August 17, 2022 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HLeithner HLeithner Awaiting requested review from HLeithner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Joomla! 4.2.0

Development

Successfully merging this pull request may close these issues.

10 participants

@roland-d @HLeithner @nikosdion @N6REJ @bayareajenn @brianteeman @webgras @richard67 @fancyFranci @joomla-cms-bot