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

fix: use namespace for separator #552

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

gaige
Copy link
Contributor

@gaige gaige commented Sep 2, 2024

Use the IMAP namespace separator based on the information retrieved from the IMAP command to retrieve namespaces, falling back to '/' if the command isn't available or the results don't include the private namespace.

Resolves #551

Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.

Project coverage is 61.39%. Comparing base (d6128ea) to head (23996fc).
Report is 40 commits behind head on master.

Files with missing lines Patch % Lines
parsedmarc/mail/imap.py 14.28% 6 Missing ⚠️
parsedmarc/__init__.py 0.00% 5 Missing ⚠️
parsedmarc/mail/mailbox_connection.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
+ Coverage   59.88%   61.39%   +1.50%     
==========================================
  Files          12       12              
  Lines        1578     1650      +72     
==========================================
+ Hits          945     1013      +68     
- Misses        633      637       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seanthegeek
Copy link
Contributor

A couple of things:

  • I don't see get_folder_separator() defined anywhere when a do a quick search of the repo. Where is it defined at?
  • Let's set the folder separator once instead of calling get_folder_separator() for each path.

@gaige
Copy link
Contributor Author

gaige commented Sep 2, 2024

@seanthegeek Yeah, somehow I missed the other files, they're in the mail classes, they should be there now. I've updated to get the separator once.

@gaige
Copy link
Contributor Author

gaige commented Sep 2, 2024

I have verified that this works in my configuration with Dovecot. The IMAPClient namespace method is used for determining the separator uses the IMAP standard call for determining the separator, so I don't expect any problems with other servers, however I've put in fallbacks for both IMAP and the base class to use the / separator which was used originally. In the case of the IMAP class, / will be returned if the call doesn't contain the expected parameters.

@seanthegeek seanthegeek merged commit 06d8578 into domainaware:master Sep 2, 2024
6 of 7 checks passed
seanthegeek added a commit that referenced this pull request Sep 3, 2024
- Skip invalid aggregate report rows without calling the whole report invalid
  - Some providers such as GoDaddy will send reports with some rows missing a source IP address, while other rows are fine
- Fix Dovecot support by using the seperator provided by the IPMAP namespace when possible (PR #552 closes #551)
- Only download `base_reverse_dns_map.csv` once (fixes #542)
- Update included `base_reverse_dns_map.csv`
  - Replace University category with Education to be more inclusive
- Update included `dbip-country-lite.mmdb`
seanthegeek added a commit that referenced this pull request Sep 3, 2024
- Fix processing of SMTP-TLS reports (#549)
- Skip invalid aggregate report rows without calling the whole report invalid
  - Some providers such as GoDaddy will send reports with some rows missing a source IP address, while other rows are fine
- Fix Dovecot support by using the seperator provided by the IPMAP namespace when possible (PR #552 closes #551)
- Only download `base_reverse_dns_map.csv` once (fixes #542)
- Update included `base_reverse_dns_map.csv`
  - Replace University category with Education to be more inclusive
- Update included `dbip-country-lite.mmdb`
@StarkZarn StarkZarn mentioned this pull request Sep 5, 2024
seanthegeek added a commit that referenced this pull request Oct 3, 2024
- Proper IMAP namespace fix (Closes issue #557 and issue #563)
  - Require `mailsuite>=1.9.17`
  - Revert PR #552
- Add pre-flight check for nameservers (PR #562 closes issue #543)
- Reformat code with `ruff`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IMAP: Character not allowed in mailbox name: '/'
2 participants