desktop clients cannot relogin

[butonic]

Starting with v5.0.0 we return the username instead of the userid in the generateToken response #286 (comment)

When the client tries to relogin / get a new access token it fails because is sends the userid as $user and then the comparison in #286 (comment) fails, because we are comparing username to userid. This leads to a weird error:

Fixing the comparison with

diff --git a/lib/Controller/PageController.php b/lib/Controller/PageController.php
index c8bc4f4..028fcef 100755
--- a/lib/Controller/PageController.php
+++ b/lib/Controller/PageController.php
@@ -129,7 +129,7 @@ class PageController extends Controller {
                        );
                }
 
-               if ($user !== null && $user !== $this->userSession->getUser()->getUserName()) {
+               if ($this->isDifferentUser($user)) {
                        $logoutUrl = $this->urlGenerator->linkToRouteAbsolute(
                                'oauth2.page.logout',
                                [
@@ -388,4 +388,19 @@ class PageController extends Controller {
                $escapedDisplayName = Util::sanitizeHTML($displayName);
                return "<span class='hasTooltip' data-original-title='$userId'><strong>$escapedDisplayName</strong></span>";
        }
+
+       /**
+        * @param string $userId
+        * @return bool
+        */
+       private function isDifferentUser($userId) {
+               if (empty($userId)) {
+                       return false;
+               }
+               $userObj = $this->userManager->get($userId);
+               if ($userObj === null) {
+                       return true;
+               }
+               return $userObj->getUID() !== $this->userSession->getUser()->getUID();
+       }
 }

leads to a new problem: The desktop client (who identifies the user by userid) now chokes on the new access token becaues it uses the username instead of the userid:

The reason is the change in #286 (comment)

I don't see a good way to fix this other than reverting #286

@vjt what exactly were you trying to fix. I know it is very bad UX for users to be see the uid... The desktop client uses the userid in the login request: https://cloud.example.org/index.php/apps/oauth2/authorize?response_type=code&client_id=xdXOt13JKxym1B1QcEncf2XDkLAexMBFwiT9j6EfhhHFJhs2KM9jbjTmf8JBXE69&redirect_uri=http://localhost:59937&code_challenge=secret&code_challenge_method=S256&scope=openid%20offline_access%20email%20profile&prompt=select_account%20consent&state=secret&login_hint={userid}&user={userid}

I'll check if we can replace that with the username so users don't get confused.

[butonic]

this might work:

diff --git a/lib/Controller/PageController.php b/lib/Controller/PageController.php
index c8bc4f4..64470f0 100755
--- a/lib/Controller/PageController.php
+++ b/lib/Controller/PageController.php
@@ -98,7 +98,7 @@ class PageController extends Controller {
         * @param string $client_id The client identifier.
         * @param string $redirect_uri The redirection URI.
         * @param string | null $state The state.
-        * @param string | null $user
+        * @param string | null $user The user id
         *
         * @return TemplateResponse | RedirectResponse The authorize view or the
         * authorize-error view with a redirection to the
@@ -129,11 +129,19 @@ class PageController extends Controller {
                        );
                }
 
-               if ($user !== null && $user !== $this->userSession->getUser()->getUserName()) {
+               // we need to translate the user id to username so we can show the username to the end user
+               $userObj = $this->userManager->get($user);
+               if ($userObj !== null) {
+                       $userName = $userObj->getUserName();
+               } else {
+                       $userName = $user;
+               }
+
+               if ($this->isDifferentUser($user)) {
                        $logoutUrl = $this->urlGenerator->linkToRouteAbsolute(
                                'oauth2.page.logout',
                                [
-                                       'user' => $user,
+                                       'user' => $userName,
                                        'requesttoken' => Util::callRegister(),
                                        'response_type' => $response_type,
                                        'client_id' => $client_id,
@@ -143,7 +151,7 @@ class PageController extends Controller {
                        );
                        $currentUser = $this->userSession->getUser();
                        $currentUser = $this->buildDisplayForUser($currentUser);
-                       $requestedUser = $this->buildDisplayForUser($user);
+                       $requestedUser = $this->buildDisplayForUser($userObj);
                        return new TemplateResponse(
                                $this->appName,
                                'switch-user',
                                @@ -195,7 +203,7 @@ class PageController extends Controller {
                $logoutUrl = $this->urlGenerator->linkToRouteAbsolute(
                        'oauth2.page.logout',
                        [
-                               'user' => $user,
+                               'user' => $userName,
                                'requesttoken' => Util::callRegister(),
                                'response_type' => $response_type,
                                'client_id' => $client_id,
@@ -347,12 +355,20 @@ class PageController extends Controller {
                // logout the current user
                $this->userSession->logout();
 
+               // we need to translate the user id to username so we can show the username to the end user
+               $userObj = $this->userManager->get($user);
+               if ($userObj !== null) {
+                       $userName = $userObj->getUserName();
+               } else {
+                       $userName = $user;
+               }
+
                $redirectUrl = $this->urlGenerator->linkToRoute('oauth2.page.authorize', [
                        'response_type' => $response_type,
                        'client_id' => $client_id,
                        'redirect_uri' => $redirect_uri,
                        'state' => $state,
-                       'user' => $user
+                       'user' => $userName
                ]);
 
                // redirect the browser to the login page and set the redirect_url to the authorize page of oauth2
                @@ -388,4 +404,19 @@ class PageController extends Controller {
                $escapedDisplayName = Util::sanitizeHTML($displayName);
                return "<span class='hasTooltip' data-original-title='$userId'><strong>$escapedDisplayName</strong></span>";
        }
+
+       /**
+        * @param string $userId
+        * @return bool
+        */
+       private function isDifferentUser($userId) {
+               if (empty($userId)) {
+                       return false;
+               }
+               $userObj = $this->userManager->get($userId);
+               if ($userObj === null) {
+                       return true;
+               }
+               return $userObj->getUID() !== $this->userSession->getUser()->getUID();
+       }
 }

This change would put the username back into the logout urls that will ultimately end up as the login_hint / in the username field.

[butonic]

The getUsername was originally introduced with owncloud/core#32587 to make windows network drive pick up the username from LDAP, even if the owncloud internal username was using a UUID / GUID or the UPN.

[butonic]

The correct 'fix' is to use userid in the apis, but teach the clients about the user name ... which they currently cannot get, because there is no api that would allow them to get the username.

[butonic]

we might be able to expose the username in the ocs api https://demo.owncloud.com/ocs/v1.php/cloud/user:

<?xml version="1.0"?>
<ocs>
 <meta>
  <status>ok</status>
  <statuscode>100</statuscode>
  <message>OK</message>
  <totalitems></totalitems>
  <itemsperpage></itemsperpage>
 </meta>
 <data>
  <id>demo</id>
  <display-name>demo</display-name>
  <email/>
 </data>
</ocs>

we could add a <username> there ...

thatd would allow all clients to send a proper login_hint, eg: owncloud/client#9506

[butonic]

related: owncloud/ocis#3196

[butonic]

@vjt could you check if this PR and #328 work for you? Together, users should see their login when reauthenticating in the web ui.

[vjt]

Hi @butonic! I have left the company in which this is currently hsex, but I’ll ping an ex colleague of mine (@Scuotivento) asking whether he can test. Could a branch be prepared to aid him in checking out the right code version?

Thanks!