Fix ajax no access error #23910

chippison · 2025-12-22T02:11:36Z

Description

Dev-19785

This PR is to fix the regression we had when we tried to redirect invalid session errors to login page (https://innocraft.atlassian.net/browse/UX-305).

We only want to force the redirect when we actually detect an invalid session issue and not to redirect ALL NoAccessExceptions. We will retain the original behaviour of NoAccessException for ajax calls to just fail silently and just returning the error message.

Testing:

NOTE: The only way I could test this fix was to try to do some datatable actions that will trigger saving of table preference. This would show errors and will refresh the page with no change in the sorting when the issue is not fixed. This happened when the regression was introduced and was noticeable on our Demo Site.
If the issue is fixed, the datatable should not give any errors and sorting should work (although it will not be saved because it is 'anonymous' user).

This should still error and redirect if a session becomes invalid (session expires or is deleted). I was able to test this by logging in as an admin to my local site. Then deleted my session (or changing the expire date to something in the past), then clicking on a new category/page or even just trying to sort the datatable.

Checklist

[✔] I have understood, reviewed, and tested all AI outputs before use
[✔] All AI instructions respect security, IP, and privacy rules

Review

github-actions · 2026-01-07T02:16:15Z

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

sgiehl

Generally the changes might work, but are far away from "perfect". When working on such legacy parts it's important to also challenge the current implementation and consider fundamental changes instead of just relying on existing behavior.

I've used codex to create some code upon your changes, to implement a proper session timeout handling that is less error prone and improve the code overall a bit:

Relying on 401 status code might still cause problems, as plugins might send 401 status codes, that should not result in reloads. I've update that to use a custom header instead.
Setting a global Access state when the session has a real timeout makes the whole handling a lot more accurate, than trying to detect that based on the referrer.
As the more accurate timeout detection would prevent the session timeout message to be displayed after page reload this state is temporarily stored in a cookie
Properly differ between 401 (fully unauthorized requests) and 403 (authorized requests without permission)

See fix-ajax-no-access-error...fix-session-timeout-handling for the proposed changes.

Note: This was built upon my rough ideas. I haven't done a proper review or deeper testing yet

core/Plugin/Controller.php

core/Access.php

core/NoAccessException.php

sgiehl · 2026-01-15T15:51:30Z

plugins/CoreHome/tests/UI/WidgetLoader_spec.js

+    const loginForm = await page.waitForSelector('#login_form_login', { visible: true });
+    expect(loginForm).to.be.ok;

    const expectedText = 'Error: Your session has expired due to inactivity. Please log in to continue.';


Technically that might already have been incorrect before. Removing cookies isn't really a session timeout. It's just an unauthenticated request, that Matomo incorrectly reports as session timeout.

As written here before. This can't be considered a session timeout. A missing cookie needs to be interpreted as unauthorized.

plugins/Login/Controller.php

core/Access.php

chippison · 2026-01-16T02:42:00Z

Generally the changes might work, but are far away from "perfect". When working on such legacy parts it's important to also challenge the current implementation and consider fundamental changes instead of just relying on existing behavior.

I've used codex to create some code upon your changes, to implement a proper session timeout handling that is less error prone and improve the code overall a bit:

Relying on 401 status code might still cause problems, as plugins might send 401 status codes, that should not result in reloads. I've update that to use a custom header instead.

Setting a global Access state when the session has a real timeout makes the whole handling a lot more accurate, than trying to detect that based on the referrer.

As the more accurate timeout detection would prevent the session timeout message to be displayed after page reload this state is temporarily stored in a cookie

Properly differ between 401 (fully unauthorized requests) and 403 (authorized requests without permission)

See fix-ajax-no-access-error...fix-session-timeout-handling for the proposed changes.

Note: This was built upon my rough ideas. I haven't done a proper review or deeper testing yet

Thanks for your input on this @sgiehl.
It was really helpful to point me in the right direction.
I have merged your changes(fix-session-timeout-handling) to this PR.
I also added some code to make it work if anonymous users are allowed to view the site.
Hopefully my changes are all good. 😁

Cheers

sgiehl

Left some comments. I don't think the changes you made regarding the anonymous user are needed at all. The anonymous user does not need to log in. So even if the session times out, that user would not be redirected to the login form if view access to the website is granted.

sgiehl · 2026-01-16T07:57:08Z

plugins/CoreHome/vue/src/AjaxHelper/AjaxHelper.ts

    const result = new Promise<T | ErrorResponse>((resolve, reject) => {
-      this.requestHandle!.then((data: unknown) => {
+      this.requestHandle!.then((data: unknown, _textStatus: string, xhr: jqXHR) => {
+        const isInApp = !document.querySelector('#login_form');


I can't see a good reason why this would be needed. If the request returns a 200, there was no problem. Even if the session might have timed out - e.g. for anonymous - it was no problem. Otherwise it would result in a 40X.

sgiehl · 2026-01-16T07:58:17Z

plugins/Login/tests/UI/ResetPassword_spec.js

    }

-    it('should display password reset form when forgot password link clicked', async function () {
+    xit('should display password reset form when forgot password link clicked', async function () {


What are those changes good for?

sgiehl · 2026-01-16T08:00:31Z

tests/UI/expected-screenshots/UIIntegrationTest_not_logged_in.png

That change looks incorrect. Why should it show the session message?

sgiehl · 2026-01-16T08:01:27Z

plugins/CoreHome/tests/UI/WidgetLoader_spec.js

+    const loginForm = await page.waitForSelector('#login_form_login', { visible: true });
+    expect(loginForm).to.be.ok;

    const expectedText = 'Error: Your session has expired due to inactivity. Please log in to continue.';


As written here before. This can't be considered a session timeout. A missing cookie needs to be interpreted as unauthorized.

sgiehl · 2026-01-16T08:32:52Z

core/Session/SessionAuth.php

    {
        $expirationTime = $sessionFingerprint->getExpirationTime();
        if (empty($expirationTime)) {
+            $this->sessionExpired = true;


I think this line causes the test changes and should be reverted. From a technical perspective a session without an expiration time should not occur. It happens if there is no session at all. So the very first call to Matomo (without any cookie) is now always marked as timed out. Same happens for the tests using FakeAccess, as they are not using the SessionAuth.

…is not the same as token saved for user

…ession has expired or is invalid

…rue otherwise, just fail silently return error message

… for network to be idle before doing some page actions

…error message since we have put back most ajax calls to fail silently

…ion time is empty/null

…his results in a successful call (since we allow anonymous user to access the api), we just need to check the session timed out header and do a refresh if it exists

…hanges to returned http code and exception thrown.

chippison requested a review from a team December 23, 2025 03:46

chippison marked this pull request as ready for review December 23, 2025 03:46

github-actions bot added the Stale The label used by the Close Stale Issues action label Jan 7, 2026

chippison added the Do not close PRs with this label won't be marked as stale by the Close Stale Issues action label Jan 7, 2026

sgiehl removed Do not close PRs with this label won't be marked as stale by the Close Stale Issues action Stale The label used by the Close Stale Issues action labels Jan 15, 2026

sgiehl force-pushed the fix-ajax-no-access-error branch from b8ea408 to b795c1a Compare January 15, 2026 08:27

sgiehl added the Regression Indicates a feature used to work in a certain way but it no longer does even though it should. label Jan 15, 2026

sgiehl added this to the 5.7.0 milestone Jan 15, 2026

sgiehl requested changes Jan 15, 2026

View reviewed changes

chippison requested a review from sgiehl January 16, 2026 02:58

sgiehl requested changes Jan 16, 2026

View reviewed changes

chippison and others added 16 commits January 19, 2026 21:59

Adding a session expired header to the NoAccessException error

eb57634

put back old version of Access.php

799635f

adding a flag to indicate session has expired

22ca0cb

add indication that session is invalid/expired when token in request …

a7ff017

…is not the same as token saved for user

adding a check that login and token auth is null as indication that s…

d379f03

…ession has expired or is invalid

only enforcing 401 http code in header when session expired flag is t…

ba963d2

…rue otherwise, just fail silently return error message

Making sure we 'await' when saving testEnvironment as well as waiting…

5c08a49

… for network to be idle before doing some page actions

making widgetloader open a blank page first

3ec2253

fixing test

e76a3e0

adding options override

816b0bf

remove commented out code

71c8b20

changing the test so that it now just checks for the session expired …

a61f1c6

…error message since we have put back most ajax calls to fail silently

test using a different test fixture

f1a04ad

adding back the test to check the error message is shown

121b0f1

Improve detection of session timeouts

12a3eaa

use custom header instead of status code

cfc87e6

sgiehl and others added 8 commits January 19, 2026 21:59

Adds UI test

462cbc2

presere timeout state in cookie

d499feb

making sure the setting the session expired flag to true when expirat…

71edc17

…ion time is empty/null

handling the case where anonymous user is allowed to view the site. T…

f0b9b36

…his results in a successful call (since we allow anonymous user to access the api), we just need to check the session timed out header and do a refresh if it exists

adding tests; removed unused space

9a1fdc4

updating systems test and integration tests for Core to reflect our c…

e649089

…hanges to returned http code and exception thrown.

updating UI tests

ed0b24a

fixed missed test after testing

b9ca187

sgiehl force-pushed the fix-ajax-no-access-error branch 3 times, most recently from 345e17b to ebc08c1 Compare January 20, 2026 09:04

apply review feedback

07714c8

sgiehl force-pushed the fix-ajax-no-access-error branch from ebc08c1 to 07714c8 Compare January 20, 2026 09:57

Adds UI test

87a734b

Uh oh!

Fix ajax no access error #23910

Are you sure you want to change the base?

Fix ajax no access error #23910

Conversation

chippison commented Dec 22, 2025

Description

Testing:

Checklist

Review

Uh oh!

github-actions bot commented Jan 7, 2026

Uh oh!

sgiehl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sgiehl Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

sgiehl Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chippison commented Jan 16, 2026

Uh oh!

sgiehl left a comment

Choose a reason for hiding this comment

Uh oh!

sgiehl Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

sgiehl Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

sgiehl Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

sgiehl Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

sgiehl Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants