Fix #2427 by converting $remember to integer #2428

justin-sleep · 2016-11-30T21:30:34Z

mention-bot · 2016-11-30T21:30:37Z

@justin-sleep, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ChristophWurst, @icewind1991 and @blizzz to be potential reviewers.

Signed-off-by: justin-sleep <justin@quarterfull.com>

codecov-io · 2016-12-01T00:22:05Z

Current coverage is 57.11% (diff: 100%)

Merging #2428 into master will increase coverage by 0.05%

@@             master      #2428   diff @@
==========================================
  Files          1199       1198     -1   
  Lines         72262      72254     -8   
  Methods        7347       7347          
  Messages          0          0          
  Branches       1214       1216     +2   
==========================================
+ Hits          41228      41265    +37   
+ Misses        31034      30989    -45   
  Partials          0          0

Powered by Codecov. Last update d7dd399...25a5c65

rullzer · 2016-12-01T08:25:04Z

@justin-sleep thanks a lot for reporting the bug and fixing it as well!

@ChristophWurst please have a look :)

LukasReschke · 2016-12-01T08:58:15Z

lib/private/User/Session.php

@@ -558,7 +558,7 @@ public function createSessionToken(IRequest $request, $uid, $loginName, $passwor
 		try {
 			$sessionId = $this->session->getId();
 			$pwd = $this->getPassword($password);
-			$this->tokenProvider->generateToken($sessionId, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, $remember);
+			$this->tokenProvider->generateToken($sessionId, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, +$remember);


(int)$remember maybe?

As I understand it from PHP's page on arithmetic operators isn't +$remember essentially the same? Or is it better to explicitly cast as int here?

It's not the same. +$var also would convert to float, while (int) explicitly converts to int.

php > $s3 = '12.3'; php > $a = +$s3; php > var_dump($a); php shell code:1: double(12.3) php > $a = (int)$s3; php > var_dump($a); php shell code:1: int(12)

This should not apply here, and nevertheless it's better to be crisp and clear :) Also, imho (int) has a better readability.

While they might be semanticly the same. I think what @LukasReschke means more is that explicitly casting with (int) makes it more readable.

Signed-off-by: justin-sleep <justin@quarterfull.com>

nickvergessen · 2016-12-01T14:51:23Z

lib/private/User/Session.php

@@ -558,7 +558,7 @@ public function createSessionToken(IRequest $request, $uid, $loginName, $passwor
 		try {
 			$sessionId = $this->session->getId();
 			$pwd = $this->getPassword($password);
-			$this->tokenProvider->generateToken($sessionId, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, $remember);
+			$this->tokenProvider->generateToken($sessionId, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, (int)$remember);


I think this should either be at the top of the chain (LoginController:

server/core/Controller/LoginController.php

Line 243 in d75e35b

$this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password, $remember_login);

)
or the bottom (DefaultTokenProvider:

server/lib/private/Authentication/Token/DefaultTokenProvider.php

Line 94 in 2183a1f

$dbToken->setRemember($remember);

)

but not in the middle?

And I think top is the way to go, because also this method states that it should be called with an integer only.

Yup sounds good!

MorrisJobke · 2016-12-02T11:47:35Z

@justin-sleep Could I ask you to do the changes as requested above? Then this is good to get in 🚀

Signed-off-by: justin-sleep <justin@quarterfull.com>

justin-sleep · 2016-12-02T22:25:42Z

Look good?

nickvergessen · 2016-12-03T07:36:23Z

👍

MorrisJobke · 2016-12-03T10:10:02Z

Tested and works 👍

MorrisJobke · 2016-12-03T10:11:39Z

Thanks a lot @justin-sleep - feel free to join our IRC channel #nextcloud-dev on freenode or the forums :) Maybe also check out some other starter issues in https://github.com/nextcloud/server/issues?q=is%3Aissue+is%3Aopen+label%3A%22starter+issue%22 😉 Have a nice weekend :)

Fix #2427 by converting $remember to integer

9ee9d21

Signed-off-by: justin-sleep <justin@quarterfull.com>

rullzer added 3. to review Waiting for reviews bug labels Dec 1, 2016

rullzer added this to the Nextcloud 11.0 milestone Dec 1, 2016

LukasReschke reviewed Dec 1, 2016

View reviewed changes

Explicitly cast $remember to int rather than using identity operator

bcadd22

Signed-off-by: justin-sleep <justin@quarterfull.com>

nickvergessen reviewed Dec 1, 2016

View reviewed changes

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 2, 2016

Move integer casting to the top of the chain

25a5c65

Signed-off-by: justin-sleep <justin@quarterfull.com>

ChristophWurst self-assigned this Dec 3, 2016

MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 3, 2016

MorrisJobke merged commit 9eb1ea4 into nextcloud:master Dec 3, 2016

LukasReschke mentioned this pull request Dec 3, 2016

Unable to log in after upgrade from 10 to 11 beta2 #2495

Closed

MorrisJobke mentioned this pull request Dec 5, 2016

Upgrade NC 10.0.1 to 11 beta 2: Incorrect schema migration on Postgres (9.6) #2510

Closed

nickvergessen mentioned this pull request Dec 8, 2016

PostgreSQL Database error with v11Beta2 #2555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2427 by converting $remember to integer #2428

Fix #2427 by converting $remember to integer #2428

justin-sleep commented Nov 30, 2016 •

edited by MorrisJobke

Loading

mention-bot commented Nov 30, 2016

codecov-io commented Dec 1, 2016 •

edited

Loading

rullzer commented Dec 1, 2016

LukasReschke Dec 1, 2016

justin-sleep Dec 1, 2016

blizzz Dec 1, 2016

rullzer Dec 1, 2016

nickvergessen Dec 1, 2016

nickvergessen Dec 1, 2016

ChristophWurst Dec 1, 2016

MorrisJobke commented Dec 2, 2016

justin-sleep commented Dec 2, 2016

nickvergessen commented Dec 3, 2016

MorrisJobke commented Dec 3, 2016 •

edited

Loading

MorrisJobke commented Dec 3, 2016

Fix #2427 by converting $remember to integer #2428

Fix #2427 by converting $remember to integer #2428

Conversation

justin-sleep commented Nov 30, 2016 • edited by MorrisJobke Loading

mention-bot commented Nov 30, 2016

codecov-io commented Dec 1, 2016 • edited Loading

Current coverage is 57.11% (diff: 100%)

rullzer commented Dec 1, 2016

LukasReschke Dec 1, 2016

Choose a reason for hiding this comment

justin-sleep Dec 1, 2016

Choose a reason for hiding this comment

blizzz Dec 1, 2016

Choose a reason for hiding this comment

rullzer Dec 1, 2016

Choose a reason for hiding this comment

nickvergessen Dec 1, 2016

Choose a reason for hiding this comment

nickvergessen Dec 1, 2016

Choose a reason for hiding this comment

ChristophWurst Dec 1, 2016

Choose a reason for hiding this comment

MorrisJobke commented Dec 2, 2016

justin-sleep commented Dec 2, 2016

nickvergessen commented Dec 3, 2016

MorrisJobke commented Dec 3, 2016 • edited Loading

MorrisJobke commented Dec 3, 2016

justin-sleep commented Nov 30, 2016 •

edited by MorrisJobke

Loading

codecov-io commented Dec 1, 2016 •

edited

Loading

MorrisJobke commented Dec 3, 2016 •

edited

Loading