[WIP] Migrate incognito mode to IUserSession

[LukasReschke]

Fixes #12891 together with #12899

@schiesbn @PVince81

[LukasReschke]

Also @Raydiation

[BernhardPosselt]

Looks good but I wouldnt put the icognito mode setter on the public interface. We dont want that app devs use this setting (since its already deprecated). So I'd just add it to the user session class and remove it from the public IUserSession interface

[schiessle]

Looks good but I wouldnt put the icognito mode setter on the public interface. We dont want that app devs use this setting (since its already deprecated).

We shouldn't deprecate it before we have a replacement. At the moment that's what the app developer need to use if he provides a public view on files.

example: https://github.com/owncloud/gallery/blob/master/ajax/image.php#L26

[BernhardPosselt]

@schiesbn app developers that dont want to migrate off incognito mode can still continue to use the public interface they're used to (OCP\User that is). App developers should in fact migrate away from incognito mode and the solution is to not rely on it. Therefore this is IMHO perfectly fine ;)

[schiessle]

As I tried to explain in the other issue many Apps and many places in core relay on getUser() to return the correct user, which is the user performing the current operation. Getting back a random user or false, depending if and who is logged in at a different browser window is not good can trigger many nasty bugs.

If we want to find a way to remove the incognito mode, we need to do it in the right order:

1. research to find out what is effected by the incognito mode
2. decide if it is a good idea or not to return random users for public operations
   1. If the conclusion to 2 is "yes", we need to find a way to handle this case and teach app developers about it
   2. if the conclusion to 2 is "no" and we still want to get rid of the incognito mode we need to implement a replacement
3. Now we can deprecate methods which are no longer needed
4. After some time (I would say at least one year) we can remove the deprecated methods

Starting with 3+4 in parallel doesn't sounds like a good idea to me

[BernhardPosselt]

Deprecating methods should be done as soon as possible since it does no harm and tells developers that they need to migrate their code to a model without icognito mode. This is the first thing that should be done. Then you can start looking what is still dependant on it and fix it.

[schiessle]

Deprecating methods should be done as soon as possible since it does no harm and tells developers that they need to migrate their code to a model without icognito mode.

Not if we don't know yet how the new model looks like and if it really works 😉

[BernhardPosselt]

The new model is to check for public sharing in the encryption app and gallery ;)

[BernhardPosselt]

And fixing the path issues in the filesystem stuff that you talked about

[scrutinizer-notifier]

The inspection completed: 262 new issues, 6 updated code elements

[DeepDiver1975]

00:02:55.955 ................PHP Fatal error:  Call to a member function executeQuery() on null in /var/jenkins/workspace/pull-request-analyser-ng-simple@22/label/SLAVE/lib/private/appconfig.php on line 109

[LukasReschke]

Yes - I know. Same on the other PR - looking at it tomorrow. - But thankfully the tests are even locally failing for me on master… => Need a new testing VM.

[ghost]

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/4816/

Build result: FAILURE

[...truncated 17 lines...]using GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git6992423690025864952.credentials # timeout=10 > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse origin/pr/12912/merge^{commit} # timeout=10 > git branch -a --contains cef2f8f6bd8da4004a5db250fb966ffe04e0e9ca # timeout=10 > git rev-parse remotes/origin/pr/12912/merge^{commit} # timeout=10Checking out Revision cef2f8f6bd8da4004a5db250fb966ffe04e0e9ca (origin/pr/12912/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f cef2f8f6bd8da4004a5db250fb966ffe04e0e9caFirst time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEConfiguration pull-request-analyser-ng-simple » SLAVE is still in the queue: Waiting for next available executor on SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 2 minutes 6 seconds
💣 Test FAILed. 💣

[DeepDiver1975]

is this a burning change for OC8 - which can not wait for OC8.1?

[LukasReschke]

The problem that we face here is #12891

As long as we don't touch any sharing related code for master anymore that uses the new public API instead of OC_User, we're fine. But if we do such a change we might have a problem with public sharing apps.

I'd propose that I open another PR where I add a quick'n dirty hack in IUserSession which checks the singleton from OC_User, that should only be a 2-3 line change and thus much easier to review. - Then we can keep this one open for later. @DeepDiver1975 agreed?

[LukasReschke]

Ultra-slim hack version: #12923

[ghost]

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/12060/
💣 Test FAILed. 💣

[PVince81]

@LukasReschke time to revive the ultra-fat non-hack version ? 😉

[PVince81]

@LukasReschke please rebase

[LukasReschke]

Let's reopen this later at some point.