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

add BIMI as source for avatars #5784

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from
Draft

add BIMI as source for avatars #5784

wants to merge 26 commits into from

Conversation

MeiKatz
Copy link

@MeiKatz MeiKatz commented Dec 6, 2021

RFC: https://www.ietf.org/archive/id/draft-blank-ietf-bimi-02.txt

@welcome
Copy link

welcome bot commented Dec 6, 2021

Thanks for opening your first pull request in this repository! ✌️

- @todo: currently only supports "default" as selector ( e.g. default._bimi.example.org )

Signed-off-by: MeiKatz <[email protected]>
@miaulalala
Copy link
Contributor

While I personally love the code style with whitespace between brackets and variables, CI isn't going to let it pass ;) can you run the cs fixer? Should all be there for you to be able to run it via composer with composer run cs:fix.

@MeiKatz
Copy link
Author

MeiKatz commented Dec 6, 2021

@miaulalala okay, I will try. This is my first pull request here therefore I have to learn this step by step :)

@miaulalala
Copy link
Contributor

miaulalala commented Dec 6, 2021

Cool, if you have any questions, feel free to post here or you could join the public dev chat if you'd like!

@MeiKatz
Copy link
Author

MeiKatz commented Dec 6, 2021

Okay, currently I am struggling with the message The PEAR repository has been removed from Composer 2.x. I tried to find a solution but as far as I searched I could not find a solution. How do you solve this issue?

@miaulalala
Copy link
Contributor

Does that happen when you try to run composer install?

@MeiKatz
Copy link
Author

MeiKatz commented Dec 6, 2021

Right. I also cannot run any of the scripts. I guess that is because composer install does not succeed beforehand.

Signed-off-by: MeiKatz <[email protected]>
@MeiKatz
Copy link
Author

MeiKatz commented Dec 6, 2021

Okay, I could fix the problem by removing the pear package from the repositories list in the composer.json.

@ChristophWurst
Copy link
Member

Okay, currently I am struggling with the message The PEAR repository has been removed from Composer 2.x. I tried to find a solution but as far as I searched I could not find a solution. How do you solve this issue?

You will have to use composer v1. I keep a local copy of it as alias of composer1, then I run composer1 i and the like for this app.

Okay, I could fix the problem by removing the pear package from the repositories list in the composer.json.

Hmm okay. But don't you miss the Horde libraries then?

@MeiKatz
Copy link
Author

MeiKatz commented Dec 7, 2021

Hmm okay. But don't you miss the Horde libraries then?

Maybe, but I could run the fixer. If this is enough then we should try the workflow tasks on the GitHub servers.

@MeiKatz
Copy link
Author

MeiKatz commented Dec 9, 2021

@ChristophWurst is there any blocking issue in my pull request? Just curious about the workflow here and if there is anything that I can do to speed up the process.

…as wrapper for dns_record_get()

Signed-off-by: MeiKatz <[email protected]>
Signed-off-by: MeiKatz <[email protected]>
Signed-off-by: MeiKatz <[email protected]>
@ChristophWurst
Copy link
Member

@ChristophWurst is there any blocking issue in my pull request? Just curious about the workflow here and if there is anything that I can do to speed up the process.

We'll test/review soon. Do you have any examples that we could test this with?

@MeiKatz MeiKatz requested a review from miaulalala December 23, 2021 09:18
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

Right, by signing up with wordpress I could test this. The avatar is returned as a data:image string but it never reaches the frontend

@miaulalala
Copy link
Contributor

For testing purposes: pinterest also has a BIMI record.

@MeiKatz
Copy link
Author

MeiKatz commented Dec 23, 2021

Right, by signing up with wordpress I could test this. The avatar is returned as a data:image string but it never reaches the frontend

What do you mean with "it never reaches the frontend"? Is something else shown? Or is it a display error? Or something totally different?

@MeiKatz MeiKatz force-pushed the master branch 2 times, most recently from 232a167 to d52eed6 Compare December 23, 2021 19:51
@miaulalala
Copy link
Contributor

Not an error, but the data format doesn't work with the html from the frontend. I signed up for a wordpress account, which will send you an email so you can test this.

@MeiKatz
Copy link
Author

MeiKatz commented Jan 3, 2022

Not an error, but the data format doesn't work with the html from the frontend. I signed up for a wordpress account, which will send you an email so you can test this.

I currently try to get a full NextCloud instance running on my local machine with Podman, but I am facing some problems with this. So it's not done yet. Nevertheless, can you send me the received data from the api, that the frontend cannot handle?

@miaulalala
Copy link
Contributor

miaulalala commented Feb 28, 2022

Hi, finally got you the data:

so the Avatar object looks like this for me:

$avatar = {OCA\Mail\Service\Avatar\Avatar} [3]
 url = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAIAAAACACAIAAABMXPacAAAABGdBTUEAALGPC/xhBQAAACBjSFJNAAB6JgAAgIQAAPoAAACA6AAAdTAAAOpgAAA6mAAAF3CculE8AAAABmJLR0QAAAAAAAD5Q7t/AAAAB3RJTUUH5gIcDR4WRIvwpQAAJNtJREFUeNrtfXtcVNUW/zpn3g9mYHgNL2EgQRnRQURTy4jQ1JtpKmaQXc361S8u3atx4Wo+SiwfXdEul1tm/TK7qGlqlIkiEZZgCghKiJowPOX9mDfzOOf8/jh4HHnMmUGQ+ly//8DZs59rnbP23muvtTbAQ4wqEABwWf7P0e7G/y6Y5B+UzR/tnvwvAjfp0dHuw/86HjJglPGQAaOMhwwYZTxkwCjjIQNGGQ8ZMMp4yIBRxkMGjDIeMmCU8ZABowzmaHeAHrhRxzJ28wxtqKre0KJsVl63nV8qG8fzDMTFvgaeu5njjHIEoz0CW/hdMoAgcKNWoGtkNl2tuZxPpikUiqioqLCwpTKZzM3NTSQScblcJpPJYDAwDLNYLD09PWq1ur29XalUlpeX5+fn15aVkWUDJkdZvCbqBD4oRwgIMtrDuwe96ujfiTaU0HeLVDdbLhw3aNUCgSAxMXHu3LlyuVwikaCow9ISx/HOzs6KiorTp0+np6frdDqeUOQ5fbFaHIzwnUd7rAAAuEn/u2AAgZn53dX6kuOdzfUKhSIlJSU6Otrd3R0ZvreVIIi2tra8vLwdO3aUlZVJpH78iMV650CEwRrFgY8+A3CT3rm1VJmXCQDp6emxsbGenp4j3WhLS8vRo0cTExMBQBYd3+0RPorDHzUG4CaDS/PF6vwj48ePz8jImDVrFoPBGDCnxWJpbW29efNmWVlZUVHRpUuXbt26NVi1QUFBkZGRU6dODQ8PDw4Olkqlg8kuDMN++umnhISEysrKwKhlXdJpKJv3wIkwGgwgcM"
 mime = "image/png"
 isExternal = true

It works fine up to this line: https://github.com/MeiKatz/nextcloud-mail/blob/master/lib/Service/AvatarService.php#L144

I think this method needs an additional check on the mime type to cover this case of a base64 encoded image as URL.

@MeiKatz
Copy link
Author

MeiKatz commented Feb 28, 2022

@miaulalala So this is the value of $cachedImage or of $avatar?

@miaulalala
Copy link
Contributor

This is the value of $avatar

@ChristophWurst ChristophWurst marked this pull request as draft June 28, 2023 11:53
@ChristophWurst
Copy link
Member

This would still be a very nice feature. @MeiKatz if you have time to continue/finish the feature let me know. Else I'll try to find someone else to take over, but it won't happen anytime soon.

@MeiKatz
Copy link
Author

MeiKatz commented Sep 29, 2023

@ChristophWurst currently my development of the feature is paused but I never stopped thinking about it. In the last few months I learnt a lot about deployment of NC via Docker and therefore I will try in the next weeks so set up a local instance of NC so I can re-start developing this feature.

@ChristophWurst ChristophWurst mentioned this pull request Nov 29, 2023
24 tasks
@ChristophWurst
Copy link
Member

The SVG handling is a bit scary. I wish that could be offloaded to a library with plenty of tests.

The rest looks alright, so I'm not closing the PR yet.

@MeiKatz
Copy link
Author

MeiKatz commented Dec 6, 2023

The SVG handling is a bit scary. I wish that could be offloaded to a library with plenty of tests.

I would go with a package that does the job but as far as I know there is not for now. But I can create this missing package myself and use it here, so the problem goes into my play field. Would this be better?

@ChristophWurst
Copy link
Member

https://github.com/SRWieZ/php-svg-ps-converter?

@ChristophWurst ChristophWurst mentioned this pull request Mar 11, 2024
17 tasks
@pabzm
Copy link
Contributor

pabzm commented May 3, 2024

As far as I can tell this code doesn't check if the email is valid and legimitate. I'd suggest to at least require a good DMARC-result, else this might even make illegitimate emails look more trustworthy.

This feature is in development for Roundcubemail, too, and over there someone suggested technical details of such a requirement.

@MeiKatz
Copy link
Author

MeiKatz commented May 3, 2024

@pabzm You've got a valid point there. DMARC would be the requirement because paid certificates are for the most companies unavailable because they are way too expensive.

This PR is on my to do list for a long time now and maybe there should be a separate pull request before where checking for DMARC is added and we can call something like $this->isValidDMARC() before fetching the BIMI image.

@Neustradamus
Copy link

@MeiKatz: Can you update your PR with the latest BIMI draft version?

Thanks in advance.

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.

8 participants