Umbraco-cms: Backoffice nodetree saved in cache even after logout

[liamlaverty]

A user's content node tree is saved in their browser cache. When that user signs out, the tree isn't cleared from cache. If another user with a different account signs in on the same computer, the first user's content tree is shown to the 2nd user - even if the 2nd user doesn't have privileges to view those nodes.

Bug summary

It looks as if this line isn't checking if the contents of the cache belong to the user trying to access it.

https://github.com/umbraco/Umbraco-CMS/blob/acbe82f4c891bc8ae4a7b5568bc949a54e141bde/src/Umbraco.Web.UI.Client/src/common/services/tree.service.js#L566

There's also probably a bug in the sign out code, that's not clearing away the cache.

Specifics

• Checked in Umbraco v8.0.2
• Chrome/FF/Edge latest versions as of 2nd Aug 2019
• No errors in the console

Steps to reproduce

1. Create a few content items in an Umbraco site (make sure to add some child nodes for best reproduction results)
2. Create two users.
   
   
   
   • User 1 (Admin) has full admin privileges over the entire content tree
   
   
   • User 2 (Editor) has restricted editing privileges, and a "content start node" of one of the children
   
   
3. As the admin user, open a node that the Editor doesn't have access to
4. Sign out
5. Sign in as the editor
6. View the node tree of the admin
7. Hard refresh to view the tree of the editor
8. Sign out
9. Sign in as the admin
10. View the node tree of the editor

If you're filing a bug, please describe how to reproduce it. Include as much
relevant information as possible, such as:

Expected result

Expected the node tree for the editor to contain the contents they're authorised to view

Actual result

The editor was able to view the contents of the node tree they're not authorised to view.

[liamlaverty]

https://github.com/liamlaverty/Umbraco-Node-Tree-Bug

You can use this repo with users pre-setup to reproduce. The error occurs in both versions. In v8.0.2 the tree never refreshes, so you always see the previous user's tree. In v8.1.1 the same error occurs, though after about half a second the current user's tree appears.

Credentials for both sites:
admin
[email protected]
AdministrationCustomPass

editor
[email protected]
EditorCustomPass

[kjac]

@liamlaverty thanks for reporting 👍

I can reproduce this. But after debugging a bit, I don't think the tree cache is the culprit here. It looks more as if the tree component isn't destroyed on logout, thus retaining its state for a split second until the tree is reloaded after login.

Could you help me verify this thesis? Try closing the window after logout and see if the issue persists?

[liamlaverty]

Hey @kjac,

If I close the window after logging out, the issue no longer occurs, and the correct user's node tree appears (checked on firefox).

[nul800sebastiaan]

While this certainly seems like a bug, I'm a bit curious as to the requirement for multiple users using the same computer and the same user account on that computer.

Won't you very very quickly run into the issue where the user with more privilege forgets to log out and then the next user taking over that session?

[liamlaverty]

I mainly reported it because it suggests there's some housekeeping tasks that aren't being done on logout, which might be a symptom of a broader issue. It was my assumption that tidying up would happen on logout for everything that's user-specific.

We came across the bug in 8.0 while debugging a permissions issue for one of our sites, a dev got stuck in a loop thinking they'd misconfigured some permissions, and spent some time working out if they were experiencing an issue with Umbraco, or with their own configuration. The process was:

1. Admin creates a new editor account with some restrictive node tree permissions

2. Admin signs out of their own account.

3. Admin signs into the editor account to check the new permissions are set correctly

4. Admin shown a screen that suggests they haven't setup the permissions correctly

5. Admin signs back into the admin account to correct the issues

6. return to step 2

[nul800sebastiaan]

Perfect, that is an absolutely valid use case I didn't think about @liamlaverty !

We should get this fixed up then, while there is no bigger issue (closing browser invalidates whatever cache there is), it certainly makes sense to be able to test permissions properly.

[umbrabot]

Hi @liamlaverty,

We're writing to let you know that we've added the Up For Grabs label to your issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly PR team bot :-)

[Shazwazza]

This is similar to what was fixed in 8.1.0, see rev https://github.com/umbraco/Umbraco-CMS/commit/4ed746e4ef1d01174db393c363ca522db093351c#diff-addf183ea1c959af2523c8ba5db8b3f9

So is this still an issue in 8.1 ? You mention

In v8.1.1 the same error occurs, though after about half a second the current user's tree appears.

It seems the issue now is that there's a flicker/timing issue of some sort. We can't really clear the section/tree state when a user's session times out because if we do that and the user logs back in then their state is lost.

[nul800sebastiaan]

Aah I see.. so this is to make sure when a user gets automatically logged out, their tree state is intact after they log back in 🤔

How about we only clear the section/tree state when someone clicks the logout button on purpose? So an auto-logout just preserves the state, but a deliberate logout just makes you start over.

[Shazwazza]

@nul800sebastiaan In 8.1.0 - if a user gets logged out due to timeout, and a new user logs back in we __do__ refresh the tree/section. But as @liamlaverty mentions it might perhaps mean there is a very quick flicker that the new user might see as the old section/tree is refreshed.

We've always left the tree/section state intact if a user just times out, there is nothing changed there. In v7 the section/tree handling is just different than 8 and it also clears when a new user logs in but there is no flicker.

We can of course fix the flicker - just need to confirm that this is the only real problem now in 8.1.

When a user explicitly logs out, i can't remember what we do but we might already eagerly reset everything but would need to check the code.

[kjac]

@Shazwazza I'm running through the code right now. The caches and states are only cleared upon login and of course only if the current user differs from the last user.

The flicker does seem to be the only real problem in 8.1, and I do believe it's simply the last tree state still being stuck in the DOM before the cache is invalidated and the tree state is reloaded.

I'm running a quick test to see if a cache/state clear upon explicit logout solves anything.

[Shazwazza]

@kjac cool, i'm sure we can fix the flicker. Might be able to tap into the promise that we have in the navigationService. When the back office starts it loads the nav and the editor at the same time and there can be timing issues with syncing/showing the tree depending on when things load so there's a handy promise in the navigation service navReadyPromise and you can wait on the nav with navigationService.navReadyPromise. Perhaps we can 'reset' that promise when a new user logs in or something.

[kjac]

Long story short... the flicker is caused by the previously loaded tree being stuck in the DOM after login. It's not a caching issue.

I have created a fix in #6071 but I'm not really sure it's worth it. I'm sure @Shazwazza has an opinion about it 😄

[Shazwazza]

After reviewing and testing #6071 we have concluded that while the fix addresses the flicker, it has an issue with syncing the tree node back to where it was. To fix the whole thing would then require resyncing the tree, etc... which requires additional work. We'll close this for now since i think we can live with the tiny flicker.