Hotfix: Change http status code for client 2.4.2 #145

micbar · 2018-08-08T09:55:00Z

Hotfix for #144

Problem

The 2.4.2 desktop client goes into an infinite loop when he gets a "invalid_grant" while refreshing his access code with response code 400. This puts very high loads on the webserver.

Hotfix

If he would get the same response with http status code 200, he would stop his infinite loop and open the webUI to re authorize.

Changes

I changed the http status code to 200 after checking for the 2.4.2 client in the request header.

All other clients get the 400 status code, as the Oauth Specs demand it. https://tools.ietf.org/html/rfc6749#section-5.2

CLAassistant · 2018-08-08T09:55:07Z

All committers have signed the CLA.

patrickjahns · 2018-08-08T10:58:09Z

lib/Controller/OAuthApiController.php

+					$statusCode = Http::STATUS_OK;
+				}
+				else {
+					$statusCode = Http::STATUS_BAD_REQUEST;


How about moving this before the if condition?

$statusCode = Http::STATUS_BAD_REQUEST; if (preg_match('/\bmirall\b.+2\.4\.2/i', $this->request->getHeader('User-Agent'))) { $statusCode = Http::STATUS_OK; }

This eases the flow of reading - and you could add a comment on top outlining that this is a fix

Good point!

can you tell my, how to set a http user agent header in the unit test case?

currently the request mock is created here:

oauth2/tests/Unit/Controller/OAuthApiControllerTest.php

Line 148 in 61a71b4

$this->getMockBuilder('OCP\IRequest')->getMock(),

make it a class field and you can work on it in the individual tests

micbar · 2018-08-08T11:20:14Z

@patrickjahns @phil-davis @individual-it can anyone tell me why acceptance tests are failing?

codecov · 2018-08-08T13:07:16Z

Codecov Report

Merging #145 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master    #145      +/-   ##
===========================================
+ Coverage     82.12%   82.2%   +0.07%     
- Complexity      199     200       +1     
===========================================
  Files            21      21              
  Lines           705     708       +3     
===========================================
+ Hits            579     582       +3     
  Misses          126     126

Impacted Files	Coverage Δ	Complexity Δ
lib/Controller/OAuthApiController.php	`100% <100%> (ø)`	`19 <0> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89c17f0...b0cbaaa. Read the comment docs.

micbar · 2018-08-09T11:40:13Z

Output from travis/ci


File tests/.travis/custom.ini added to /home/travis/.phpenv/versions/nightly/etc/conf.d.
Using database oc_autotest
<?php
$AUTOCONFIG = array (
  'installed' => false,
  'dbtype' => 'oci',
  'dbtableprefix' => 'oc_',
  'adminlogin' => 'admin',
  'adminpass' => 'admin',
  'directory' => '/home/travis/build/owncloud/core/data-autotest',
  'dbuser' => 'autotest',
  'dbname' => 'XE',
  'dbhost' =>'',
  'dbpass' => 'owncloud',
  'loglevel' => 0,
);
Setup environment for sqlite testing ...
INDEX
This version of ownCloud is not compatible with PHP 7.3<br/>You are currently running PHP 7.4.0-dev.END INDEX
owncloud configuration:
<?php
$CONFIG = [
	'apps_paths' => [
		[
			'path'		=> OC::$SERVERROOT . '/apps',
			'url'		=> '/apps',
			'writable'	=> true,
		],
	],
	'default_language' => 'en',
];
if (\is_dir(OC::$SERVERROOT.'/apps2')) {
	$CONFIG['apps_paths'][] = [
		'path' => OC::$SERVERROOT . '/apps2',
		'url' => '/apps2',
		'writable' => false,
	];
}
if (\getenv("TC") === "selenium") {
	$CONFIG['skeletondirectory'] = OC::$SERVERROOT . '/tests/ui/skeleton';
}
data directory:
total 0
owncloud.log:
before_install.8
0.00s$ cd ../core
0.06s$ ./occ app:enable $APP_NAME
This version of ownCloud is not compatible with PHP 7.3
You are currently running PHP 7.4.0-dev.
The command "./occ app:enable $APP_NAME" failed and exited with 1 during .
Your build has been stopped.

phil-davis · 2018-08-09T11:44:01Z

I restarted the first Travis job 974.1 for PHP 5.6 - that had hung for some reason and got the "no output for 10 minutes" message.

phil-davis · 2018-08-09T11:46:15Z

Where did you see stuff about PHP 7.3 and PHP 7.4 ???

micbar · 2018-08-09T11:47:50Z

I saw that at the travis build log.

phil-davis · 2018-08-09T11:56:18Z

Something went crazy in Travis! PHP 7.3beta1 came out last week, and so I guess PHP 7.4.0-dev does exist. But goodness knows what happened for Travis to use those.

Anyway, CI passes.

DeepDiver1975 · 2018-08-09T12:00:00Z

Will review soon.

DeepDiver1975 · 2018-08-09T13:14:55Z

tests/Unit/Controller/OAuthApiControllerTest.php

+		$_SERVER['PHP_AUTH_PW'] = null;
+
+		$result = $this->controller->generateToken('refresh_token', null, null, $this->refreshToken->getToken());
+		$this->assertTrue($result instanceof JSONResponse);


assertInstanceOf

DeepDiver1975 · 2018-08-09T13:15:37Z

tests/Unit/Controller/OAuthApiControllerTest.php

+		$this->assertNotEmpty($json->message_url);
+		$this->assertEquals($this->authorizationSuccessfulMessageUrl, $json->message_url);
+		$this->assertEquals(200, $result->getStatus());
+		$this->assertEquals(1, count($this->accessTokenMapper->findAll()));


assertCount

DeepDiver1975 · 2018-08-09T13:16:48Z

tests/Unit/Controller/OAuthApiControllerTest.php

+
+		$result = $this->controller->generateToken('refresh_token', null, null, $this->refreshToken->getToken());
+		$this->assertTrue($result instanceof JSONResponse);
+		$json = json_decode($result->render());


just use getData - this gives you the array

micbar self-assigned this Aug 8, 2018

micbar added blue-ticket p2-high labels Aug 8, 2018

micbar requested a review from DeepDiver1975 August 8, 2018 09:57

pmaier1 mentioned this pull request Aug 8, 2018

OAuth2 hotfix 0.2.3 #144

Closed

21 tasks

patrickjahns reviewed Aug 8, 2018

View reviewed changes

micbar force-pushed the response-codes branch from 1e34b7f to 60a1d0c Compare August 8, 2018 13:07

micbar force-pushed the response-codes branch from 60a1d0c to 5bdd80a Compare August 8, 2018 14:25

micbar requested a review from PVince81 August 9, 2018 09:36

Hotfix: Change http status code for client 2.4.2

b0cbaaa

micbar force-pushed the response-codes branch from 5bdd80a to b0cbaaa Compare August 9, 2018 11:22

DeepDiver1975 reviewed Aug 9, 2018

View reviewed changes

DeepDiver1975 approved these changes Aug 9, 2018

View reviewed changes

DeepDiver1975 merged commit 9e1991e into master Aug 9, 2018

DeepDiver1975 deleted the response-codes branch August 9, 2018 13:22

patrickjahns modified the milestones: development, 0.2.3 Aug 9, 2018

T0mWz mentioned this pull request Aug 10, 2018

Owncloud deletes files & tries to sync again (with little success) owncloud/client#6677

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hotfix: Change http status code for client 2.4.2 #145

Hotfix: Change http status code for client 2.4.2 #145

micbar commented Aug 8, 2018 •

edited

Loading

CLAassistant commented Aug 8, 2018 •

edited

Loading

patrickjahns Aug 8, 2018

micbar Aug 8, 2018

micbar Aug 8, 2018

DeepDiver1975 Aug 8, 2018

micbar commented Aug 8, 2018

codecov bot commented Aug 8, 2018 •

edited

Loading

micbar commented Aug 9, 2018 •

edited

Loading

phil-davis commented Aug 9, 2018

phil-davis commented Aug 9, 2018

micbar commented Aug 9, 2018

phil-davis commented Aug 9, 2018

DeepDiver1975 commented Aug 9, 2018

DeepDiver1975 Aug 9, 2018

DeepDiver1975 Aug 9, 2018

DeepDiver1975 Aug 9, 2018

Hotfix: Change http status code for client 2.4.2 #145

Hotfix: Change http status code for client 2.4.2 #145

Conversation

micbar commented Aug 8, 2018 • edited Loading

Problem

Hotfix

Changes

CLAassistant commented Aug 8, 2018 • edited Loading

patrickjahns Aug 8, 2018

Choose a reason for hiding this comment

micbar Aug 8, 2018

Choose a reason for hiding this comment

micbar Aug 8, 2018

Choose a reason for hiding this comment

DeepDiver1975 Aug 8, 2018

Choose a reason for hiding this comment

micbar commented Aug 8, 2018

codecov bot commented Aug 8, 2018 • edited Loading

Codecov Report

micbar commented Aug 9, 2018 • edited Loading

phil-davis commented Aug 9, 2018

phil-davis commented Aug 9, 2018

micbar commented Aug 9, 2018

phil-davis commented Aug 9, 2018

DeepDiver1975 commented Aug 9, 2018

DeepDiver1975 Aug 9, 2018

Choose a reason for hiding this comment

DeepDiver1975 Aug 9, 2018

Choose a reason for hiding this comment

DeepDiver1975 Aug 9, 2018

Choose a reason for hiding this comment

micbar commented Aug 8, 2018 •

edited

Loading

CLAassistant commented Aug 8, 2018 •

edited

Loading

codecov bot commented Aug 8, 2018 •

edited

Loading

micbar commented Aug 9, 2018 •

edited

Loading