Set default port for Mattermost notifications

[sochotnicky]

---

Description:

When we generate a MM notification URL with:

>>> NotifyMattermost(token="secret", host="chat.example.com", port=443, secure=True).url()
'mmosts://chat.example.com/secret/?image=no&format=text&overflow=upstream'

We need to be able to also parse it out with same defaults. However:

>>> from apprise import Apprise
>>> ac=Apprise()
>>> ac.add(NotifyMattermost(token="secret", host="chat.example.com", port=443, secure=True).u
rl())
>>> ac.urls()
['mmosts://chat.example.com:None/secret/?image=no&format=text&overflow=upstream']

This causes failure of notifications due to Port being set to None instead of parsing default port 443or 80 (depending whether secure is True). This PR sets the default port if not provided

New Service Completion Status

• apprise/plugins/.py
• KEYWORDS
  • add new service into this file (alphabetically).
• README.md
  • add entry for new service to table (as a quick reference)
• packaging/redhat/python-apprise.spec
  • add new service into the %global common_description

Checklist

• The code change is tested and works locally.
• There is no commented out code in this PR.
• No lint errors (use flake8)
• 100% test coverage

Testing

Anyone can help test this source code as follows:

# Create a virtual environment to work in as follows:
python3 -m venv apprise

# Change into our new directory
cd apprise

# Activate our virtual environment
source bin/activate

# Install the branch
pip install git+https://github.com/sochotnicky/apprise.git@fix-mm-port-parsing

# Test out the changes with the following command:
apprise -t "Test Title" -b "Test Message" \
   'mmosts://chat.example.com/secret/?image=no&format=text&overflow=upstream'

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.36%. Comparing base (9bd20a2) to head (786f8a8).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1293   +/-   ##
=======================================
  Coverage   99.36%   99.36%           
=======================================
  Files         159      159           
  Lines       20724    20730    +6     
  Branches     3725     3728    +3     
=======================================
+ Hits        20593    20599    +6     
  Misses        121      121           
  Partials       10       10           

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

[caronc]

This change is no required; i'm not sure why you would want to force a port when it is already known. https always means 443 and http always means 80 if not otherwise specified.

Why would you want to exclusively set the port. This would especially break some SSL issues if there are keys involved and a request is made to https://my.site.name:443 where the CA keys would probably be set up for only my.site.name (not my.site.name:443 - uncommon)

[sochotnicky]

This change is no required; i'm not sure why you would want to force a port when it is already known. https always means 443 and http always means 80 if not otherwise specified.

The trouble is that the original code does not parse results of NotifyMattermost(token="secret", host="chat.example.com", port=443, secure=True).url() correctly.

Shouldn't result of url() of be parseable by parse_url() and get back the same object?

Why would you want to exclusively set the port. This would especially break some SSL issues if there are keys involved and a request is made to https://my.site.name:443 where the CA keys would probably be set up for only my.site.name (not my.site.name:443 - uncommon)

The trouble is Port gets parsed to "None". So then, continuing the description if we try to actually send the notification we get:

>>> ac.notify(body="test")
DEBUG:apprise:Mattermost POST URL: https://chat.example.com:None/hooks/secret (cert_verify=True)
DEBUG:apprise:Mattermost Payload: {'text': 'test', 'icon_url': None, 'username': 'Apprise'}
WARNING:apprise:A Connection error occurred sending Mattermost notification.
DEBUG:apprise:Socket Exception: Failed to parse: https://chat.example.com:None/hooks/secret

With the patch it becomes DEBUG - Mattermost POST URL: https://chat.example.com:443/hooks/secret (cert_verify=True) and works. I don't think port will matter for TLS/SSL cert validation (didn't in my case/test at least even though the cert does not specify the port). Reading though RFC I don't think port will matter.

It's possible the bug/fix is elsewhere though. Would appreciate pointers where/how to fix the :None in the Mattermost POST URL to be at least empty string if not actual port.

[sochotnicky]

Ah, perhaps the fix is in:

apprise/apprise/plugins/mattermost.py

Lines 218 to 220 in ece4920

	url = '{}://{}:{}{}/hooks/{}'.format(
	self.schema, self.host, self.port,
	self.fullpath.rstrip('/'), self.token)

Basically - don't include the port in URL if it matches default. Would that be preferred fix from your end I suppose?

[sochotnicky]

@caronc Adjusted the PR using the other approach.

Forgot to mention! Thanks for quick feedback/review. Much appreciated

[caronc]

Sorry for taking so long to review; just traveling right now. I'll try to review soon.

If you explicitly specify a port (https and 443 combined as you identified), then I absolutely agree that Apprise should explicitly make use of it. If it's not, then this is definitely a bug. If its setting a None value on the URL, then this is definitely also a bug.

Can you have a look at tests/test_plugin_mattermost.py and add test cases that exploit this issue with your PR proving it's fixed.

Have a look at other test cases, even ones in other plugin files for examples if you need too. It's a bit of extra work, but worth it in the end

[sochotnicky]

Can you have a look at tests/test_plugin_mattermost.py and add test cases that exploit this issue with your PR proving it's fixed.

Have a look at other test cases, even ones in other plugin files for examples if you need too. It's a bit of extra work, but worth it in the end

Had a quick glance at AppriseURLTester but it didn't seem I could test which URL was actually used for POST so I've added a dedicated test. Test case succeeds on this PR, fails on master.