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(ocs): send a message via api #9672

Merged
merged 1 commit into from
Jul 18, 2024
Merged

Conversation

miaulalala
Copy link
Contributor

@miaulalala miaulalala commented May 27, 2024

@ChristophWurst
Copy link
Member

ChristophWurst commented May 28, 2024

PRs don't go onto the board if they have a ticket. Else you are tracking two items for one task ;)

Edit: seems github doesn't show my action in the history. I removed the PR from https://github.com/orgs/nextcloud/projects/61

@miaulalala miaulalala marked this pull request as draft June 18, 2024 09:40
@miaulalala miaulalala marked this pull request as ready for review June 19, 2024 15:03
lib/Controller/MessageApiController.php Outdated Show resolved Hide resolved
lib/Controller/MessageApiController.php Outdated Show resolved Hide resolved
lib/Service/AliasesService.php Outdated Show resolved Hide resolved
lib/Service/AliasesService.php Outdated Show resolved Hide resolved
@kesselb
Copy link
Contributor

kesselb commented Jun 25, 2024

This was for sending an email by another Nextcloud app?

@miaulalala
Copy link
Contributor Author

This was for sending an email by another Nextcloud app?

The workflow engine

@kesselb
Copy link
Contributor

kesselb commented Jun 28, 2024

Response (with debug = false, loglevel = 2) when an account does not exist.

<?xml version="1.0"?>
<ocs>
    <meta>
        <status>failure</status>
        <statuscode>404</statuscode>
        <message></message>
    </meta>
    <data>OCA\Mail\Exception\ClientException: Account 12 does not exist or you don\'t have permission to access it in
        /var/www/html/apps-extra/mail/lib/Service/AccountService.php:101
        Stack trace:
        #0 /var/www/html/apps-extra/mail/lib/Controller/MessageApiController.php(69): OCA\Mail\Service\AccountService-&gt;find('admin',
        12)
        #1 /var/www/html/lib/private/AppFramework/Http/Dispatcher.php(208): OCA\Mail\Controller\MessageApiController-&gt;send(12,
        'blub@blub.com', 'Hello Hello', '&lt;html&gt;&lt;body&gt;&lt;h1...', true, Array, Array, Array, Array)
        #2 /var/www/html/lib/private/AppFramework/Http/Dispatcher.php(114): OC\AppFramework\Http\Dispatcher-&gt;executeController(Object(OCA\Mail\Controller\MessageApiController),
        'send')
        #3 /var/www/html/lib/private/AppFramework/App.php(161): OC\AppFramework\Http\Dispatcher-&gt;dispatch(Object(OCA\Mail\Controller\MessageApiController),
        'send')
        #4 /var/www/html/lib/private/Route/Router.php(309): OC\AppFramework\App::main('OCA\\Mail\\Contro...', 'send',
        Object(OC\AppFramework\DependencyInjection\DIContainer), Array)
        #5 /var/www/html/ocs/v1.php(43): OC\Route\Router-&gt;match('/ocsapp/apps/ma...')
        #6 /var/www/html/ocs/v2.php(7): require_once('/var/www/html/o...')
        #7 {main}
    </data>
</ocs>

Response (debug = false, loglevel = 2) when alias does not exist

<?xml version="1.0"?>
<ocs>
    <meta>
        <status>failure</status>
        <statuscode>404</statuscode>
        <message></message>
    </meta>
    <data>OCP\AppFramework\Db\DoesNotExistException: Did expect one result but found none when executing: query &quot;SELECT
        `aliases`.*, `accounts`.`provisioning_id` FROM `*PREFIX*mail_aliases` `aliases` INNER JOIN
        `*PREFIX*mail_accounts` `accounts` ON `aliases`.`account_id` = `accounts`.`id` WHERE (`accounts`.`user_id` =
        :dcValue1) AND (`aliases`.`alias` = :dcValue2)&quot;; in
        /var/www/html/lib/public/AppFramework/Db/QBMapper.php:260
        Stack trace:
        #0 /var/www/html/lib/public/AppFramework/Db/QBMapper.php(338): OCP\AppFramework\Db\QBMapper-&gt;findOneQuery(Object(OC\DB\QueryBuilder\QueryBuilder))
        #1 /var/www/html/apps-extra/mail/lib/Db/AliasMapper.php(67): OCP\AppFramework\Db\QBMapper-&gt;findEntity(Object(OC\DB\QueryBuilder\QueryBuilder))
        #2 /var/www/html/apps-extra/mail/lib/Service/AliasesService.php(57): OCA\Mail\Db\AliasMapper-&gt;findByAlias('blub@blub.com',
        'admin')
        #3 /var/www/html/apps-extra/mail/lib/Controller/MessageApiController.php(77): OCA\Mail\Service\AliasesService-&gt;findByAliasAndUserId('blub@blub.com',
        'admin')
        #4 /var/www/html/lib/private/AppFramework/Http/Dispatcher.php(208): OCA\Mail\Controller\MessageApiController-&gt;send(10,
        'blub@blub.com', 'Hello Hello', '&lt;html&gt;&lt;body&gt;&lt;h1...', true, Array, Array, Array, Array)
        #5 /var/www/html/lib/private/AppFramework/Http/Dispatcher.php(114): OC\AppFramework\Http\Dispatcher-&gt;executeController(Object(OCA\Mail\Controller\MessageApiController),
        'send')
        #6 /var/www/html/lib/private/AppFramework/App.php(161): OC\AppFramework\Http\Dispatcher-&gt;dispatch(Object(OCA\Mail\Controller\MessageApiController),
        'send')
        #7 /var/www/html/lib/private/Route/Router.php(309): OC\AppFramework\App::main('OCA\\Mail\\Contro...', 'send',
        Object(OC\AppFramework\DependencyInjection\DIContainer), Array)
        #8 /var/www/html/ocs/v1.php(43): OC\Route\Router-&gt;match('/ocsapp/apps/ma...')
        #9 /var/www/html/ocs/v2.php(7): require_once('/var/www/html/o...')
        #10 {main}
    </data>
</ocs>

Unless it's supposed like that in ocs, we should avoid leaking the actual exceptions via ocs.

@kesselb
Copy link
Contributor

kesselb commented Jun 28, 2024

Request with malformed recipients

POST https://server.internal/ocs/v2.php/apps/mail/message/send
Authorization: Basic admin admin
Content-Type: application/json
#Cookie: XDEBUG_SESSION=start

{
  "accountId": 10,
  "fromEmail": "kesselb@e-mail.de",
  "subject": "Hello Hello",
  "body": "<html><body><h1>Hello Hello</h1><p style=\"font-color: red\">Hello Hello</p></body></html>",
  "isHtml": 1,
  "to": [
    "alice@alice.com",
    "bob@bob.com"
  ],
  "cc": [],
  "bcc": []
}

Response

<?xml version="1.0"?>
<ocs>
    <meta>
        <status>failure</status>
        <statuscode>500</statuscode>
        <message>Internal Server Error
        </message>
    </meta>
    <data/>
</ocs>

The response should be more helpful ;)

@kesselb
Copy link
Contributor

kesselb commented Jun 28, 2024

Request as x-www-form-urlencoded:

POST https://server.internal/ocs/v2.php/apps/mail/message/send
Authorization: Basic admin admin
Content-Type: application/x-www-form-urlencoded
Cookie: XDEBUG_SESSION=start

accountId = 10 &
fromEmail = email@example.org &
subject = Hello Hello www-form-urlencoded &
body = <html><body><h1>Hello Hello</h1><p style="font-color: red">Hello Hello</p></body></html> &
isHtml = 1 &
to[0][email] = email@example.org &
to[0][label] = Alice &
to[1][email] = email@example.org &
to[1][label] = Bob

Request as application/json

POST https://server.internal/ocs/v2.php/apps/mail/message/send
Authorization: Basic admin admin
Content-Type: application/json
Cookie: XDEBUG_SESSION=start

{
  "accountId": 10,
  "fromEmail": "email@example.org",
  "subject": "Hello Hello application/json",
  "body": "<html><body><h1>Hello Hello</h1><p style=\"font-color: red\">Hello Hello</p></body></html>",
  "isHtml": 1,
  "to": [
    {
      "email": "email@example.org",
      "label": "Alice"
    },
    {
      "email": "email@example.org",
      "label": "Bob"
    }
  ],
  "cc": [],
  "bcc": []
}

Request as multipart/form-data

POST https://server.internal/ocs/v2.php/apps/mail/message/send
Authorization: Basic admin admin
Content-Type: multipart/form-data; boundary=WebAppBoundary
Cookie: XDEBUG_SESSION=start

--WebAppBoundary
Content-Disposition: form-data; name="accountId"

10
--WebAppBoundary
Content-Disposition: form-data; name="fromEmail"

email@example.org
--WebAppBoundary
Content-Disposition: form-data; name="subject"

Hello Hello multipart/form-data
--WebAppBoundary
Content-Disposition: form-data; name="body"

<html><body><h1>Hello Hello</h1><p style="font-color: red">Hello Hello</p></body></html>
--WebAppBoundary
Content-Disposition: form-data; name="isHtml"

1
--WebAppBoundary
Content-Disposition: form-data; name="to[0][email]"

email@example.org

--WebAppBoundary
Content-Disposition: form-data; name="to[0][label]"

Alice
--WebAppBoundary
Content-Disposition: form-data; name="to[1][email]"

email@example.org

--WebAppBoundary
Content-Disposition: form-data; name="to[1][label]"

Bob

--WebAppBoundary
Content-Disposition: form-data; name="attachments[]"; filename="sample.txt"
Content-Type: text/plain

I'm an attachment, please attach me!

@kesselb
Copy link
Contributor

kesselb commented Jun 28, 2024

I guess it's a side effect of using ocs that three different ways of formatting the body are supported, but it's a bit weird that uploading files only works with one.

Moreover, using multipart/form-data does not feel very "rest-like". I would prefer to only accept a request in JSON and include possible attachments into the JSON.

@miaulalala
Copy link
Contributor Author

I guess it's a side effect of using ocs that three different ways of formatting the body are supported, but it's a bit weird that uploading files only works with one.

Moreover, using multipart/form-data does not feel very "rest-like". I would prefer to only accept a request in JSON and include possible attachments into the JSON.

Christoph and I had this discussion and have decided that that's the best way to do it. Yes, it's not very REST- like, I agree, but it is what it is.

@miaulalala miaulalala requested a review from kesselb July 3, 2024 13:11
@kesselb
Copy link
Contributor

kesselb commented Jul 3, 2024

Mind to change "AGPL-3.0-only" to "AGPL-3.0-or-later"?

@miaulalala miaulalala linked an issue Jul 8, 2024 that may be closed by this pull request
2 tasks
@ChristophWurst
Copy link
Member

Is this ready for merge?

@miaulalala
Copy link
Contributor Author

Is this ready for merge?

Yes - if you can look through it too?

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.

Looks good otherwise

lib/Controller/MessageApiController.php Outdated Show resolved Hide resolved
lib/Controller/MessageApiController.php Outdated Show resolved Hide resolved
lib/Controller/MessageApiController.php Outdated Show resolved Hide resolved
lib/Controller/MessageApiController.php Outdated Show resolved Hide resolved
lib/Controller/MessageApiController.php Outdated Show resolved Hide resolved
@miaulalala miaulalala force-pushed the feat/ocs-api-to-send-mails branch 2 times, most recently from 6636f5c to 048e86d Compare July 10, 2024 13:27
@kesselb
Copy link
Contributor

kesselb commented Jul 11, 2024

I've accidentally run into the following problem while testing:

The recipient "daniel alice@example.com" returns an 500er response. That's correct because the email is wrong, but the error comes from the mail transmission / horde already. We should have some basic validation in the controller for the email address.

For example:

$mightBeValidEmail = filter_var($recipient['email'], FILTER_VALIDATE_EMAIL);
if ($mightBeValidEmail === false) {
	return new DataResponse('Email address looks weird.', Http::STATUS_BAD_REQUEST);
}

Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala miaulalala merged commit d76d7ef into main Jul 18, 2024
28 checks passed
@miaulalala miaulalala deleted the feat/ocs-api-to-send-mails branch July 18, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OCS API to send a message
4 participants