Allow public routes for group-enabled apps

[tomneedham]

Fixes #20304.

Would appreciate feedback on the testing side of this. I'm not fully aware of the implications of the change.

@LukasReschke @PVince81 @nickvergessen @schiesbn

[tomneedham]

@LukasReschke Can you shed some light on the first condition here?

[LukasReschke]

It's a huge hack. Basically we use the AppFramework also for setting. $this->appName is then settings. However, the app settings does technically not exists at all and is thus also not enabled for the user. Thus it would always fail.

We thus do check if the app actually does exist, and if not we assume the request should just pass. The app settings would not exist and thus not go into that loop.

… yes … hacky … improvements welcome 😉

[nickvergessen]

I always missed functionality to check if an app is enabled for any user, on the app manager.
Maybe it's worth adding that instead of doing it in several places manually via app config?

Other then that, we need to make sure that classes are loaded, triggered hooks are working correctly etc.

[tomneedham]

I always missed functionality to check if an app is enabled for any user, on the app manager.
Maybe it's worth adding that instead of doing it in several places manually via app config?

Agreed. Especially since this must be not an uncommon requirement. I can do a separate PR.

Other then that, we need to make sure that classes are loaded, triggered hooks are working correctly etc.

@nickvergessen Can I maybe chat to you on IRC about how to write some tests for this?

[nickvergessen]

@DeepDiver1975 your architectural input needed here.
What is supposed to happen with code of an app, that is disabled for a user?
I guess it should not be loaded at all. So we can't make public routes available.

This sounds like a huge hack to me.

[DeepDiver1975]

@DeepDiver1975 your architectural input needed here.
What is supposed to happen with code of an app, that is disabled for a user?
I guess it should not be loaded at all. So we can't make public routes available.

This sounds like a huge hack to me.

In an ideal world public access would be mapped to an special case user (e.g. system/public ❓ ) and it'll remain to the admin to enable an app for this special case user or not.

[DeepDiver1975]

For the time being: public pages should be accessible no matter which groups the app is enabled for - public is a use case of it's own.

[tomneedham]

@PVince81 and myself had a chat about this earlier today to discuss situations where apps would not be around to listen to triggered hooks. This will not be an issue for public pages since all apps that are 'installed' are enabled while the page is loaded (see https://github.com/owncloud/core/blob/master/lib/private/app.php#L241 which is called with no arguments).

What is still a separate issue is apps such as Activity that listen for hooks do not have a define behaviour for when it is only enabled for some users. For example if a user performs and action which triggers a hook activity listens for, but isn't in a group with activity enabled, the app wont have been loaded. In this case the hooks wouldn't be triggered but that would leave a gap in the activity feed for a user with that app enabled. Apps would need some sort of way to enable backends for everyone regardless of whether the user-visible section of the app is available for them

[nickvergessen]

The actual problem is
https://github.com/owncloud/core/blob/master/lib/private/app.php#L257 where you end up, when a user is still logged in and uses the public page?

[bboule]

@tomneedham it looks like this has failed some tests? my assumption is that this will make 9.0 is that indeed correct?

[PVince81]

Any update on this ?
(seems the tests passed now)

What's missing ?

[tomneedham]

The actual problem is
https://github.com/owncloud/core/blob/master/lib/private/app.php#L257 where you end up, when a user is still logged in and uses the public page?

@PVince81 This is still the problem from an architectural point of view.

[MorrisJobke]

@PVince81 This is still the problem from an architectural point of view.

@cmonteroluque @PVince81 I guess this will then be a hard way to still accomplish the change with 9.0.1 😞 Any idea where this PR should go?

[PVince81]

I'd say 9.1 if that's ok and also have this scheduled official.
We need to take time to look into that architectural problem

[ghost]

moved to 9.1, also the corresponding issue.

[PVince81]

We're past feature freeze, move to 9.2 ? @cmonteroluque

[ghost]

I guess so...

[DeepDiver1975]

Fixes #20304.

the bug is blue labeled, sev2 and assigned to 9.1

[PVince81]

The fix is incomplete and had some architectural issues, so is likely to be a risky merge

[PVince81]

@DeepDiver1975 also see this comment in the ticket #20304 (comment)

[PVince81]

We'll need to get to this eventually... tricky stuff

[tomneedham]

Closing for now, until reborn

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.