Skip to content

Commit

Permalink
ExApp proxy adjustments and fixes (#296)
Browse files Browse the repository at this point in the history 
This PR made a few changes to ExApp proxy:

1. Fix Get requests proxy to preserve the url params
2. Add pass of cookies to the ExApp with request
3. Fixes Proxy work with multipart requests and files using separate
internal version of requestToExApp with slightly adjusted handling of
query and body params
4. Remove default caching for json requests

---------

Signed-off-by: Andrey Borysenko <[email protected]>
Signed-off-by: Alexander Piskun <[email protected]>
Co-authored-by: Alexander Piskun <[email protected]>
  • Loading branch information
@andrey18106 @bigcat88
andrey18106 and bigcat88 authored Jun 10, 2024
1 parent 8b26399 commit 0a80c68
Show file tree
Hide file tree
Showing 5 changed files with 199 additions and 19 deletions.
2 changes: 1 addition & 1 deletion appinfo/routes.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// Proxy
['name' => 'ExAppProxy#ExAppGet',
'url' => '/proxy/{appId}/{other}', 'verb' => 'GET' , 'root' => '/proxy',
'requirements' => ['other' => '.+'], 'defaults' => ['other' => '']],
'requirements' => ['other' => '.*'], 'defaults' => ['other' => '']],
['name' => 'ExAppProxy#ExAppPost',
'url' => '/proxy/{appId}/{other}', 'verb' => 'POST' , 'root' => '/proxy',
'requirements' => ['other' => '.+'], 'defaults' => ['other' => '']],
Expand Down
96 changes: 87 additions & 9 deletions lib/Controller/ExAppProxyController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

namespace OCA\AppAPI\Controller;

use GuzzleHttp\Cookie\CookieJar;
use GuzzleHttp\Cookie\SetCookie;
use GuzzleHttp\RequestOptions;
use OC\Security\CSP\ContentSecurityPolicyNonceManager;
use OCA\AppAPI\AppInfo\Application;
use OCA\AppAPI\ProxyResponse;
Expand Down Expand Up @@ -62,7 +65,9 @@ private function createProxyResponse(string $path, IResponse $response, $cache =
}

$proxyResponse = new ProxyResponse($response->getStatusCode(), $responseHeaders, $content);
if ($cache && !$isHTML && empty($response->getHeader('cache-control'))) {
if ($cache && !$isHTML && empty($response->getHeader('cache-control'))
&& $response->getHeader('Content-Type') !== 'application/json'
&& $response->getHeader('Content-Type') !== 'application/x-tar') {
$proxyResponse->cacheFor(3600);
}
return $proxyResponse;
Expand All @@ -76,8 +81,11 @@ public function ExAppGet(string $appId, string $other): Response {
return new NotFoundResponse();
}

$response = $this->service->requestToExApp(
$exApp, '/' . $other, $this->userId, 'GET', request: $this->request
$response = $this->service->aeRequestToExApp2(
$exApp, '/' . $other, $this->userId, 'GET', queryParams: $_GET, options: [
RequestOptions::COOKIES => $this->buildProxyCookiesJar($_COOKIE, $exApp->getHost()),
],
request: $this->request,
);
if (is_array($response)) {
return (new Response())->setStatus(500);
Expand All @@ -93,9 +101,19 @@ public function ExAppPost(string $appId, string $other): Response {
return new NotFoundResponse();
}

$response = $this->service->aeRequestToExApp(
$options = [
RequestOptions::COOKIES => $this->buildProxyCookiesJar($_COOKIE, $exApp->getHost()),
];
if (str_starts_with($this->request->getHeader('Content-Type'), 'multipart/form-data') || count($_FILES) > 0) {
$multipart = $this->buildMultipartFormData($this->prepareBodyParams($this->request->getParams()), $_FILES);
$options[RequestOptions::MULTIPART] = $multipart;
}
$bodyParams = $this->prepareBodyParams($this->request->getParams());

$response = $this->service->aeRequestToExApp2(
$exApp, '/' . $other, $this->userId,
params: $this->request->getParams(), request: $this->request
queryParams: $_GET, bodyParams: $bodyParams, options: $options,
request: $this->request,
);
if (is_array($response)) {
return (new Response())->setStatus(500);
Expand All @@ -111,8 +129,19 @@ public function ExAppPut(string $appId, string $other): Response {
return new NotFoundResponse();
}

$response = $this->service->aeRequestToExApp(
$exApp, '/' . $other, $this->userId, 'PUT', $this->request->getParams(), request: $this->request
$options = [
RequestOptions::COOKIES => $this->buildProxyCookiesJar($_COOKIE, $exApp->getHost()),
];
if (str_starts_with($this->request->getHeader('Content-Type'), 'multipart/form-data') || count($_FILES) > 0) {
$multipart = $this->buildMultipartFormData($this->prepareBodyParams($this->request->getParams()), $_FILES);
$options[RequestOptions::MULTIPART] = $multipart;
}
$bodyParams = $this->prepareBodyParams($this->request->getParams());

$response = $this->service->aeRequestToExApp2(
$exApp, '/' . $other, $this->userId, 'PUT', queryParams: $_GET, bodyParams: $bodyParams,
options: $options,
request: $this->request,
);
if (is_array($response)) {
return (new Response())->setStatus(500);
Expand All @@ -128,12 +157,61 @@ public function ExAppDelete(string $appId, string $other): Response {
return new NotFoundResponse();
}

$response = $this->service->aeRequestToExApp(
$exApp, '/' . $other, $this->userId, 'DELETE', $this->request->getParams(), request: $this->request
$bodyParams = $this->prepareBodyParams($this->request->getParams());
$response = $this->service->aeRequestToExApp2(
$exApp, '/' . $other, $this->userId, 'DELETE', queryParams: $_GET, bodyParams: $bodyParams,
options: [
RequestOptions::COOKIES => $this->buildProxyCookiesJar($_COOKIE, $exApp->getHost()),
],
request: $this->request,
);
if (is_array($response)) {
return (new Response())->setStatus(500);
}
return $this->createProxyResponse($other, $response);
}

private function buildProxyCookiesJar(array $cookies, string $domain): CookieJar {
$cookieJar = new CookieJar();
foreach ($cookies as $name => $value) {
$cookieJar->setCookie(new SetCookie([
'Domain' => $domain,
'Name' => $name,
'Value' => $value,
'Discard' => true,
'Secure' => false,
'HttpOnly' => true,
]));
}
return $cookieJar;
}

private function prepareBodyParams(array $params): array {
unset($params['appId'], $params['other'], $params['_route']);
foreach ($_GET as $key => $value) {
unset($params[$key]);
}
return $params;
}

/**
* Build the multipart form data from input parameters and files
*/
private function buildMultipartFormData(array $bodyParams, array $files): array {
$multipart = [];
foreach ($bodyParams as $key => $value) {
$multipart[] = [
'name' => $key,
'contents' => $value,
];
}
foreach ($files as $key => $file) {
$multipart[] = [
'name' => $key,
'contents' => fopen($file['tmp_name'], 'r'),
'filename' => $file['name'],
];
}
return $multipart;
}
}
89 changes: 89 additions & 0 deletions lib/Service/AppAPIService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,29 @@ public function aeRequestToExApp(
return $this->requestToExApp($exApp, $route, $userId, $method, $params, $options, $request);
}

/**
* Request to ExApp with AppAPI auth headers and ExApp user initialization
* with more correct query/body params handling
*/
public function aeRequestToExApp2(
ExApp $exApp,
string $route,
?string $userId = null,
string $method = 'POST',
array $queryParams = [],
array $bodyParams = [],
array $options = [],
?IRequest $request = null,
): array|IResponse {
try {
$this->exAppUsersService->setupExAppUser($exApp->getAppid(), $userId);
} catch (\Exception $e) {
$this->logger->error(sprintf('Error while inserting ExApp %s user. Error: %s', $exApp->getAppid(), $e->getMessage()), ['exception' => $e]);
return ['error' => 'Error while inserting ExApp user: ' . $e->getMessage()];
}
return $this->requestToExApp2($exApp, $route, $userId, $method, $queryParams, $bodyParams, $options, $request);
}

/**
* Request to ExApp with AppAPI auth headers
*/
Expand All @@ -89,6 +112,23 @@ public function requestToExApp(
return $this->requestToExAppInternal($exApp, $method, $requestData['url'], $requestData['options']);
}

/**
* Request to ExApp with AppAPI auth headers with proper query/body params handling
*/
public function requestToExApp2(
ExApp $exApp,
string $route,
?string $userId = null,
string $method = 'POST',
array $queryParams = [],
array $bodyParams = [],
array $options = [],
?IRequest $request = null,
): array|IResponse {
$requestData = $this->prepareRequestToExApp2($exApp, $route, $userId, $method, $queryParams, $bodyParams, $options, $request);
return $this->requestToExAppInternal($exApp, $method, $requestData['url'], $requestData['options']);
}

private function requestToExAppInternal(
ExApp $exApp,
string $method,
Expand Down Expand Up @@ -197,6 +237,7 @@ private function prepareRequestToExApp(
$options['nextcloud'] = [
'allow_local_address' => true, // it's required as we are using ExApp appid as hostname (usually local)
];
$options['http_errors'] = false; // do not throw exceptions on 4xx and 5xx responses
if (!empty($auth)) {
$options['auth'] = $auth;
}
Expand All @@ -211,6 +252,54 @@ private function prepareRequestToExApp(
return ['url' => $url, 'options' => $options];
}

private function prepareRequestToExApp2(
ExApp $exApp,
string $route,
?string $userId,
string $method,
array $queryParams,
array $bodyParams,
#[\SensitiveParameter]
array $options,
?IRequest $request,
): array {
$this->handleExAppDebug($exApp->getAppid(), $request, true);
$auth = [];
$url = $this->getExAppUrl($exApp, $exApp->getPort(), $auth);
if (str_starts_with($route, '/')) {
$url = $url.$route;
} else {
$url = $url.'/'.$route;
}

if (isset($options['headers']) && is_array($options['headers'])) {
$options['headers'] = [...$options['headers'], ...$this->commonService->buildAppAPIAuthHeaders($request, $userId, $exApp->getAppid(), $exApp->getVersion(), $exApp->getSecret())];
} else {
$options['headers'] = $this->commonService->buildAppAPIAuthHeaders($request, $userId, $exApp->getAppid(), $exApp->getVersion(), $exApp->getSecret());
}
$lang = $this->l10nFactory->findLanguage($exApp->getAppid());
if (!isset($options['headers']['Accept-Language'])) {
$options['headers']['Accept-Language'] = $lang;
}
$options['nextcloud'] = [
'allow_local_address' => true, // it's required as we are using ExApp appid as hostname (usually local)
];
$options['http_errors'] = false; // do not throw exceptions on 4xx and 5xx responses
if (!empty($auth)) {
$options['auth'] = $auth;
}

if ((!array_key_exists('multipart', $options))) {
if (count($queryParams) > 0) {
$url .= '?' . $this->getUriEncodedParams($queryParams);
}
if ($method !== 'GET' && count($bodyParams) > 0) {
$options['json'] = $bodyParams;
}
}
return ['url' => $url, 'options' => $options];
}

private function getUriEncodedParams(array $params): string {
$paramsContent = '';
foreach ($params as $key => $value) {
Expand Down
13 changes: 13 additions & 0 deletions tests/psalm-baseline.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,19 @@
<code>DavPlugin</code>
</MissingDependency>
</file>
<file src="lib/Controller/ExAppProxyController.php">
<UndefinedClass>
<code>CookieJar</code>
<code>CookieJar</code>
<code>RequestOptions</code>
<code>RequestOptions</code>
<code>RequestOptions</code>
<code>RequestOptions</code>
<code>RequestOptions</code>
<code>RequestOptions</code>
<code>SetCookie</code>
</UndefinedClass>
</file>
<file src="lib/Controller/ExAppsPageController.php">
<UndefinedClass>
<code><![CDATA[$this->categoryFetcher]]></code>
Expand Down
18 changes: 9 additions & 9 deletions tests/test_occ_commands_docker.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,43 +43,43 @@ def deploy_register():
if __name__ == "__main__":
# nextcloud_url should be without slash at the end
register_daemon("http://127.0.0.1:8080/")
r = run("php occ app_api:daemon:list".split(), stdout=PIPE, stderr=PIPE, check=True)
r = run("php occ --no-warnings app_api:daemon:list".split(), stdout=PIPE, stderr=PIPE, check=True)
assert not r.stderr.decode("UTF-8")
r_output = r.stdout.decode("UTF-8")
assert r_output.find("http://127.0.0.1:8080/") == -1
assert r_output.find("http://127.0.0.1:8080") != -1
unregister_daemon()
# nextcloud_url should be without slash at the end but with "index.php"
register_daemon("http://127.0.0.1:8080/index.php/")
r = run("php occ app_api:daemon:list".split(), stdout=PIPE, stderr=PIPE, check=True)
r = run("php occ --no-warnings app_api:daemon:list".split(), stdout=PIPE, stderr=PIPE, check=True)
assert not r.stderr.decode("UTF-8")
r_output = r.stdout.decode("UTF-8")
assert r_output.find("http://127.0.0.1:8080/index.php/") == -1
assert r_output.find("http://127.0.0.1:8080/index.php") != -1
# silent should not fail, as there are not such ExApp
r = run("php occ app_api:app:unregister skeleton --silent".split(), stdout=PIPE, stderr=PIPE, check=True)
r = run("php occ --no-warnings app_api:app:unregister skeleton --silent".split(), stdout=PIPE, stderr=PIPE, check=True)
assert not r.stderr.decode("UTF-8")
r_output = r.stdout.decode("UTF-8")
assert not r_output, f"Output should be empty: {r_output}"
# without "--silent" it should fail, as there are not such ExApp
r = run("php occ app_api:app:unregister skeleton".split(), stdout=PIPE)
r = run("php occ --no-warnings app_api:app:unregister skeleton".split(), stdout=PIPE)
assert r.returncode
assert r.stdout.decode("UTF-8"), "Output should be non empty"
# testing if "--keep-data" works.
deploy_register()
r = run("php occ app_api:app:unregister skeleton --keep-data".split(), stdout=PIPE, check=True)
r = run("php occ --no-warnings app_api:app:unregister skeleton --keep-data".split(), stdout=PIPE, check=True)
assert r.stdout.decode("UTF-8"), "Output should be non empty"
run("docker volume inspect nc_app_skeleton_data".split(), check=True)
# test if volume will be removed without "--keep-data"
deploy_register()
run("php occ app_api:app:unregister skeleton".split(), check=True)
run("php occ --no-warnings app_api:app:unregister skeleton".split(), check=True)
r = run("docker volume inspect nc_app_skeleton_data".split())
assert r.returncode
# test "--force" option
deploy_register()
run("docker container rm --force nc_app_skeleton".split(), check=True)
r = run("php occ app_api:app:unregister skeleton".split())
r = run("php occ --no-warnings app_api:app:unregister skeleton".split())
assert r.returncode
r = run("php occ app_api:app:unregister skeleton --silent".split())
r = run("php occ --no-warnings app_api:app:unregister skeleton --silent".split())
assert r.returncode
run("php occ app_api:app:unregister skeleton --force".split(), check=True)
run("php occ --no-warnings app_api:app:unregister skeleton --force".split(), check=True)

0 comments on commit 0a80c68

Please sign in to comment.