modoboa / modoboa-dmarc

A set of tools to use DMARC through Modoboa.
MIT License
15 stars 11 forks source link

Add support for Content-Type application/octet-stream (#52) #53

Closed tschuettler closed 3 years ago

tschuettler commented 3 years ago

Since the RFC only mentions GZIP as compression type, i have assumed that MIME parts with the content type application/octet-stream will always be an gzip archive. I'm not sure if that is a correct assumption.

Fixes #52

tonioo commented 3 years ago

@tschuettler Not sure too... Have you checked what happens if you send something that is not a gzip file using this mime type? Maybe a better approach would be to not rely on the mime type and try to guess the file type?

tschuettler commented 3 years ago

@tonioo Thanks for your feedback. I tried to guess the file type with the help of https://pypi.org/project/python-magic/. I also added a test for a successfull report and an ignored report due to non-matching file type. Arguably the to be igored report may deserve its own handling. But it should not count as failed report.

If this PR is going to be merged the documentation needs to make sure that the required c library is installed: sudo apt-get install libmagic1

tonioo commented 3 years ago

@tschuettler Could you update the README to include the library installation please?

codecov[bot] commented 3 years ago

Codecov Report

Merging #53 (1b6afd3) into master (e1b272b) will increase coverage by 0.12%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   87.45%   87.57%   +0.12%     
==========================================
  Files          19       19              
  Lines         502      507       +5     
==========================================
+ Hits          439      444       +5     
  Misses         63       63              
Impacted Files Coverage Δ
modoboa_dmarc/lib.py 82.08% <100.00%> (+0.69%) :arrow_up:

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 e1b272b...1b6afd3. Read the comment docs.

tschuettler commented 3 years ago

@tonioo Thanks for your feedback!

I had to modify the date of the report to be successfully processed, so that it wont conflict with merged test.

I do not have access to CentOS, so I could not verify that this package actually provides the required library, but searching for it brought this up. I have no idea about GHA, but assumed it would be reasonable to add the package to those stages.

Feel free to squash those commits or request for that.

tschuettler commented 3 years ago

@tonioo Hi there, can we do something to get this merged? Is there a timeframe for a new release containing this and #50? We would love to see this in production :)

tonioo commented 3 years ago

@tschuettler Sorry, I've been pretty busy. I'll merge it now and try to release a new version this week.