Strictly forbid FS access or any I/O operations during app load process

[PVince81]

In app.php from some apps, objects are created that already access the filesystem through the FS API (aka Node API) of ownCloud. This triggers an early initialization while some other plugins might not have been registered yet.

We should strictly forbid such FS access or any other I/O operations while app code is still being loaded.

Some idea how to do it: have AppManager store a loading state:

• STATE_NOT_LOADED: app loading process has not started
• STATE_LOADING: apps are being loaded (when running loadApp for every app)
• STATE_LOADED: when all apps were loaded

Then in the FS classes / API, check said flag. If the flag is different than "STATE_LOADED", throw an exception.

It is likely that many apps will not be able to function properly without this. So in cases where FS access is needed during initialization, we need to find another solution.

@butonic @patrickjahns @DeepDiver1975 @jvillafanez

[PVince81]

Ideally we should also add a hook "onload" at the time where all apps were loaded.

Then apps can start their initial operations in exactly that hook.

[ownclouders]

GitMate.io thinks possibly related issues are #8451 (files_versions app has problems if two FS operations happen on the same second), #14305 (App access to certain groups fails), #26226 ([master] LDAP app not loaded during upgrade), #17245 (Prevent version app operations when file is locked), and #515 (cannot load app - app is null).

[butonic]

The app loading is currently a little messy an mostly in old static legacy code. I would prefer to enforce the order of loading app types and throw an exception when loading the apps of one type has not finished but a new load app or load apps call is made. That is more general and works without adding more state handling to the already complicated storage layer.

The current order of app types seems to be

• always
  • session init() in https://github.com/owncloud/core/blob/master/lib/base.php#L575
  • theme init() in https://github.com/owncloud/core/blob/master/lib/base.php#L576
  • prelogin setupBackends() in https://github.com/owncloud/core/blob/master/lib/private/legacy/user.php#L128
• public.php & remote.php
  • authentcation https://github.com/owncloud/core/blob/master/public.php#L75-L76
  • filesystem https://github.com/owncloud/core/blob/master/public.php#L75-L76
  • logging https://github.com/owncloud/core/blob/master/public.php#L75-L76
• router
  • authentcation handleRequest() in https://github.com/owncloud/core/blob/master/lib/base.php#L878
• finally
  • all apps

Unfortunately, sometimes an array of app types is given. It is pure luck that the apps are then loaded in the right order. The code literally iterates over the apps and checks if their type matches an element of the array. So if loadApps(['filesystem', 'authentication', 'logging']) is called (as in https://github.com/owncloud/core/blob/master/apps/dav/appinfo/v1/publicwebdav.php#L30-L32)
that is not the order in which they are loaded. In fact all the files_* apps are loaded before eg user_ldap. 🙄

Documented are the following types https://doc.owncloud.org/server/latest/developer_manual/app/fundamentals/info.html#types

• prelogin: applications which need to load on the login page
• filesystem: applications which provide filesystem functionality (e.g., file-sharing applications)
• authentication: applications which provide authentication backends
• logging: applications which implement a logging system
• prevent_group_restriction: applications which can not be enabled for specific groups (e.g., notifications app). will be ignored

I cannot find type session in any app. It was introuced with #8557 and there seem to be apps at customers: #21578 (comment) maybe @DeepDiver1975 can elaborate? The code seems to have been maintained.

Then there is type dav which is loaded in https://github.com/owncloud/core/blob/master/apps/dav/lib/AppInfo/PluginManager.php#L106

Finally, we have several places where just loadApp is used. Sometimes by iterating over all enabled apps, then loading them. Eg the repair steps: https://github.com/owncloud/core/blob/master/core/Command/Maintenance/Repair.php#L80-L98 or the already mentioned davplugin loading: https://github.com/owncloud/core/blob/master/apps/dav/lib/AppInfo/PluginManager.php#L98-L109

In my opinion if you request to load an app, all previous app types (aka all enabled apps of previous types) need to be loaded before the requested app can be loaded.

No idea why logging is only loaded last. The admin_audit uses it. I bet that already causes bugs where we don't log when a user from a non database backend logs in.

I propose this new order of apps:

• always, in this order
  • logging
  • session
  • prelogin
  • authentication
• on demand, not necessarily in this order.
  • filesystem should only be loaded on demand
  • dav
  • are apps that provide functionality to the appframework?
• finally
  • the rest
  • are apps that consume functionality of the appframework?

What a mess ...

[DeepDiver1975]

I cannot find type session in any app. It was introuced with #8557 and there seem to be apps at customers: #21578 (comment) maybe @DeepDiver1975 can elaborate? The code seems to have been maintained.

only use once in one special environment --> we could kill that

[DeepDiver1975]

in an ideal world the loading of apps is a mutli phase process:

1. load all apps and allow them to register to any event bus/dispatcher
2. fire any event to allow all apps to consume them

[DeepDiver1975]

always, in this order

logging
session
prelogin
authentication

no idea if we should load such apps for e.g. unautenticated requests?

[jvillafanez]

Forcing a loading order won't really solve the issue: a logging app might need to store the log file somewhere in the ownCloud filesystem, or log additional actions depending if the user is an admin or not. Similarly, a session app could need to load certificates from the user's data folder.

What I see is a dependency problem due to cycles in the dependeny tree. From my point of view, the specific issue we have is that the filesystem requires user management during the loading phase. You have user_management -> encryption -> filesystem -> back to user_management. This is the problem: the filesystem requires user_management even though it's being loaded.

Although we won't be able to avoid component dependency (components will need to interact with eachother anyway), we MUST limit the interaction outside of the component initialization. This means that, during the component creation we must just inject the dependencies without interact with them. If there is any initialization required for a component, that should be delayed until it's going to be used (classic lazy initialization - as long as it's properly documented this shouldn't be an easy)

Once everything is properly wired, it shouldn't matter where the call is made.

As for how we can limit this interaction, I don't think it's really possible, or at least not easy. Most of the things that can be done in the app.php file can be done anywhere. If we remove the app.php to rely on the Application.php file for initialization, everything can be moved to the new place, so it won't solve the problem by itself.

[butonic]

so we seem to agree that app.php should go away and info.xml should be used to register to any event bus/dispatcher and after all apps have been setup the routing will consume what is necessary?

What does that mean? order of 'loading' apps no longer matters? but the order might still determine if password is checked against ldap or the db first. I guess if we compile the classloading a la symphony, we might be able to gain a huge performance improvement.

Then how do we get there? fix all the apps? add hooks/whatever is needed to core if it is missing? encryption will be a pain. how long do we want to maintain backward compatability?