Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for webhook listeners #45475

Merged
merged 32 commits into from
Jun 11, 2024
Merged

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented May 23, 2024

Summary

Add support for registering webhooks as listeners of events.

Checklist

@come-nc come-nc added the 2. developing Work in progress label May 23, 2024
@come-nc come-nc self-assigned this May 23, 2024
@come-nc come-nc force-pushed the feat/add-support-for-webhooks branch from 6af595d to 3b790df Compare May 23, 2024 10:00
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 23, 2024
@come-nc come-nc added this to the Nextcloud 30 milestone May 23, 2024
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good! How will this be used? Can we assume that apps know their webhook listeners when the app is just registered and the system isn't fully booted yet?

@come-nc
Copy link
Contributor Author

come-nc commented May 23, 2024

Code looks good! How will this be used?

Plan is to use that for integration of an external workflow engine, but architecture is not final yet.
One problem is we’d prefer if the webhook gets called only on certains conditions, but then we end up with apps/workflowengine complexity.

Can we assume that apps know their webhook listeners when the app is just registered and the system isn't fully booted yet?

Hum, what is not accessible at that point?
If I want to have an app which search in its database and user settings to know which webhooks to register, will that work?

@ChristophWurst
Copy link
Member

Can we assume that apps know their webhook listeners when the app is just registered and the system isn't fully booted yet?

Hum, what is not accessible at that point? If I want to have an app which search in its database and user settings to know which webhooks to register, will that work?

That works. Anything that is provided by other apps might be missing. Say information from a user backend.

If you always want to wire a specific event to webhook listeners it's fine. But the lack of this information could be critical when you need to conditionally register webhooks. I haven't yet understood the full picture of the new automations so this might not be necessary.

https://docs.nextcloud.com/server/latest/developer_manual/app_development/bootstrap.html#nextcloud-20-and-later for my original ideas and thoughts about the two step register/boot process.

@come-nc come-nc added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 28, 2024
@come-nc come-nc removed the request for review from nickvergessen May 28, 2024 10:02
@come-nc
Copy link
Contributor Author

come-nc commented May 28, 2024

Webhooks are moved to an application with registrations in DB instead. They will also need to support filtering of events triggerring the webhook.

  • Skeleton app
  • API to register webhooks
  • occ command to list them
  • Actually call the webhooks, through background jobs
  • Serialize the event to the webhook
  • Add a filtering system like https://github.com/Akkroo/PHPMongoQuery/blob/master/src/Akkroo/PHPMongoQuery.php
  • Switch to SPDX headers
  • Add auth methods
  • Encrypt auth info
  • Remove core webhook registration
  • Validate data passed to the API for webhook registration

@come-nc
Copy link
Contributor Author

come-nc commented May 28, 2024

  • Store app id from appapi header if available

@come-nc come-nc force-pushed the feat/add-support-for-webhooks branch 3 times, most recently from 7c37986 to 7898ea4 Compare June 3, 2024 15:13
@come-nc
Copy link
Contributor Author

come-nc commented Jun 3, 2024

To test current version:
1 - register a webhook, for instance:

<?php
function callNextcloudApi(string $endpoint, array $postfields): void {
		$uri = 'http://nextcloud.local/';

		$ch = curl_init();

		curl_setopt($ch, CURLOPT_URL, "$uri/$endpoint");
		curl_setopt($ch, CURLOPT_POST, 1);
		curl_setopt($ch, CURLOPT_POSTFIELDS, json_encode($postfields));
		curl_setopt($ch, CURLOPT_HTTPHEADER,
			[
				'Accept:application/json',
				'Content-Type:application/json',
				'OCS-APIRequest: true',
				'Authorization: Basic '. base64_encode('admin:admin')
			]);
		curl_setopt($ch, CURLOPT_HTTPAUTH, CURLAUTH_BASIC);

		// Receive server response ...
		curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);

		$server_output = curl_exec($ch);
		curl_close($ch);

		var_dump($server_output);
}

callNextcloudApi(
	'/ocs/v2.php/apps/webhooks/api/v1/webhooks',
	[
        'httpMethod' => 'POST',
        'uri' => 'http://127.0.0.1/hook.php',
        'event' => 'OCP\\Files\\Events\\Node\\NodeWrittenEvent',
        'eventFilter' => [
          'user.uid' => 'admin',
        ],
        'headers'		=> ['Header1' => 'value1'],
        'authMethod'	=> 'header',
        'authData'		=> ['Header2' => 'value2'],
	]
);

2 - react to webhook calls in your hook.php:

<?php

$filename = '/tmp/record.txt';

function callNextcloudApi($fp, string $endpoint, array $postfields): void {
	try {
		$uri = 'http://nextcloud.local/index.php/';

		$ch = curl_init();

		curl_setopt($ch, CURLOPT_URL, "$uri/$endpoint");
		curl_setopt($ch, CURLOPT_POST, 1);
		curl_setopt($ch, CURLOPT_POSTFIELDS, http_build_query($postfields));
		curl_setopt($ch, CURLOPT_HTTPHEADER, ['Authorization: Basic '. base64_encode('admin:admin')]);
		curl_setopt($ch, CURLOPT_HTTPAUTH, CURLAUTH_BASIC);

		// Receive server response ...
		curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);

		$server_output = curl_exec($ch);
		curl_close($ch);

		var_dump($server_output);

		if (fwrite($fp, $server_output."\n") === false) {
			die("Cannot write in ($filename)");
		}
	} catch (\Throwable $t) {
		if (fwrite($fp, $t."\n") === false) {
			die("Cannot write in ($filename)");
		}
	}
}

if (is_writable($filename)) {
	if (!$fp = fopen($filename, 'a')) {
		die("Cannot open ($filename)");
	}

	$event = json_decode(file_get_contents('php://input'), associative:true);

	if (fwrite($fp, file_get_contents('php://input').' - '.json_encode($_POST)."\n") === false) {
		die("Cannot write in ($filename)");
	}

	if (isset($event['event']['node']['id']) && isset($event['event']['node']['path']) && isset($event['user']['uid'])) {
		callNextcloudApi(
			$fp,
			'/apps/tables/api/1/tables/2/rows',
			[
				'data' => json_encode(
					[
						5 => $event['event']['node']['id'],
						6 => $event['event']['node']['path'],
						7 => $event['user']['uid'].' - '.($_SERVER['HTTP_HEADER1'] ?? '').' - '.($_SERVER['HTTP_HEADER2'] ?? ''),
						8 => time(),
					]
				)
			]
		);
	}

	fclose($fp);
} else {
	die("Cannot open ($filename) - no permission");
}

(this one needs a /tmp/records.txt writable file, and tables application with a table with id 2 and columns with id 5/6/7/8, adapt to your needs)
3 - You can check registered webhooks with occ webhooks:list

$ occ webhooks:list
+----+-------+--------+------------+---------------------------+----------------------------------------+----------------------+----------------------+------------+----------+
| id | appId | userId | httpMethod | uri                       | event                                  | eventFilter          | headers              | authMethod | authData |
+----+-------+--------+------------+---------------------------+----------------------------------------+----------------------+----------------------+------------+----------+
| 1  |       | admin  | POST       | http://127.0.0.1/hook.php | OCP\Files\Events\Node\NodeWrittenEvent | {"user.uid":"admin"} |                      | none       |          |
| 2  |       | admin  | POST       | http://127.0.0.1/hook.php | OCP\Files\Events\Node\NodeWrittenEvent | {"user.uid":"admin"} | {"Header1":"value1"} | header     |          |
+----+-------+--------+------------+---------------------------+----------------------------------------+----------------------+----------------------+------------+----------+

4 - You can check scheduled jobs with occ background-job:list --class="OCA\Webhooks\BackgroundJobs\WebhookCall"

$ occ background-job:list --class="OCA\Webhooks\BackgroundJobs\WebhookCall"
+-----+-----------------------------------------+---------------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| id  | class                                   | last_run                  | argument                                                                                                                                                                                        |
+-----+-----------------------------------------+---------------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| 104 | OCA\Webhooks\BackgroundJobs\WebhookCall | 1970-01-01T00:00:00+00:00 | [{"event":{"class":"OCP\\Files\\Events\\Node\\NodeWrittenEvent","node":{"id":185,"path":"\/admin\/files\/New text file.md"}},"user":{"uid":"admin","displayName":"admin"},"time":1717425671},1] |
+-----+-----------------------------------------+---------------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

@come-nc come-nc requested a review from julien-nc June 4, 2024 12:10
@come-nc come-nc marked this pull request as ready for review June 4, 2024 12:15
@come-nc
Copy link
Contributor Author

come-nc commented Jun 4, 2024

Follow-up:

  • Extract user filter from eventFilter so that the listener is not even registered depending on logged in user
  • Same for group membership

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great! A few minor comments and questions.

apps/webhooks/lib/Controller/WebhooksController.php Outdated Show resolved Hide resolved
apps/webhooks/lib/Listener/WebhooksEventListener.php Outdated Show resolved Hide resolved
apps/webhooks/appinfo/info.xml Outdated Show resolved Hide resolved
apps/webhooks/lib/Controller/WebhooksController.php Outdated Show resolved Hide resolved
apps/webhooks/lib/Db/WebhookListener.php Outdated Show resolved Hide resolved
apps/webhooks/lib/Db/WebhookListenerMapper.php Outdated Show resolved Hide resolved
apps/webhooks/lib/Listener/WebhooksEventListener.php Outdated Show resolved Hide resolved
apps/webhooks/lib/Service/PHPMongoQuery.php Outdated Show resolved Hide resolved
apps/webhooks/lib/AppInfo/Application.php Outdated Show resolved Hide resolved
apps/webhooks/lib/BackgroundJobs/WebhookCall.php Outdated Show resolved Hide resolved
apps/webhooks/lib/Controller/WebhooksController.php Outdated Show resolved Hide resolved
apps/webhooks/lib/AppInfo/Application.php Outdated Show resolved Hide resolved
@provokateurin
Copy link
Member

I'm unsubscribing from this PR for now, please ping me if you need a review of the (Open)API.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Application class cannot use DI, and having the cache in the mapper
 allows for invalidating it when inserting or updating a webhook
 registration.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
…thout auth data

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
This way importing a certificate with occ security:certificate:import
 will allow to use it for webhooks.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
…n all classes

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
There is already a webhooks application in the appstore

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the feat/add-support-for-webhooks branch from c6630d8 to bff7d3c Compare June 11, 2024 12:10
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc merged commit 9fbf208 into master Jun 11, 2024
164 checks passed
@come-nc come-nc deleted the feat/add-support-for-webhooks branch June 11, 2024 14:31
@come-nc come-nc added 4. to release Ready to be released and/or waiting for tests to finish pending documentation This pull request needs an associated documentation update and removed 2. developing Work in progress labels Jun 11, 2024
*
* @since 30.0.0
*/
interface IWebhookCompatibleEvent {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blizzz blizzz mentioned this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish pending documentation This pull request needs an associated documentation update
Projects
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

6 participants