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

Matrix: Allow token only client API authorization with token parameter #1236

Merged
merged 5 commits into from
Nov 22, 2024

Conversation

voc0der
Copy link
Contributor

@voc0der voc0der commented Nov 11, 2024

Description:

Related issue (if applicable): #
N/A

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

Description

When a token is passed and the url version is token we force it out of T2/Webhook mode and just pass the token as authentication instead.

Test out the changes with the following command:

#encrypted
apprise -t "Test Title" -b "Test Message" \
  matrixs://mct_xxxxxxxxxxxxxxxx@matrix.yourdomain.com?v=token

#unencrypted
apprise -t "Test Title" -b "Test Message" \
  matrix://mct_xxxxxxxxxxxxxxxx@matrix.yourdomain.com?v=token

where `mct_xxxxxxxxxxxxxxxx` is your header authorization bearer token for matrix.

@voc0der voc0der changed the title Matrix: Allow token only for V2 API with ?token. Matrix: Allow token only client API authorization with with ?token parameter Nov 11, 2024
@voc0der voc0der changed the title Matrix: Allow token only client API authorization with with ?token parameter Matrix: Allow token only client API authorization with token parameter Nov 11, 2024
@@ -1474,6 +1478,9 @@ def url(self, privacy=False, *args, **kwargs):

auth = ''
if self.mode != MatrixWebhookMode.T2BOT:
if self.version == "token":
Copy link
Owner

Choose a reason for hiding this comment

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

Change to if self.version == MatrixVersion.TOKEN:🙏

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.36%. Comparing base (01c1082) to head (ebe7699).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1236   +/-   ##
=======================================
  Coverage   99.36%   99.36%           
=======================================
  Files         147      147           
  Lines       20555    20560    +5     
  Branches     3663     3665    +2     
=======================================
+ Hits        20425    20430    +5     
  Misses        121      121           
  Partials        9        9           

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


🚨 Try these New Features:

@@ -1574,6 +1581,8 @@ def parse_url(url):

elif 'v' in results['qsd'] and len(results['qsd']['v']):
results['version'] = NotifyMatrix.unquote(results['qsd']['v'])
if results['version'] == "token":
Copy link
Owner

Choose a reason for hiding this comment

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

See previous comment

@voc0der
Copy link
Contributor Author

voc0der commented Nov 15, 2024

Thanks for reviewing this and pointing out the change 👍

Edit: Going to test this.

Edit2: Works.

@caronc
Copy link
Owner

caronc commented Nov 20, 2024

1 note and 1 question 😉

📓 : see above as you correctly updated the results['version'] == "token" request in one spot of your code, but not the other.
❓ Your solution appears easier, but i'm not sure where i went wrong here which attempts the same solution (but everyone claiming it didn't work).

For your solution, i'm not sure if the TOKEN belongs in the version variable (just thinking out loud here). Is there a possibility v4 of the API will come along and handle Tokens differently? If this is the case, the current logic does not appear to be forwards compatible.

@voc0der
Copy link
Contributor Author

voc0der commented Nov 20, 2024

  1. Doh... I don't know how I missed that. It's changed, and I tested it. Worked again. Thanks.
  2. If/when Matrix client/server API V4 comes out, I doubt it will handle the endpoint differently. V3 came out, and then allowed token login. Since then, now matrix-authentication-services comes out, and requires a reverse proxy compatibility layer to the login process. https://element-hq.github.io/matrix-authentication-service/setup/reverse-proxy.html#compatibility-layer This is developed by the same team, so I don't think they're trying to change that too much since now they're joint, but you never know.

I just checked the code. So since V3 is the default, this uses V3. Not V2. it could use V2, but that's not how it currently goes. So when V4 comes out, that will become the default, and the endpoint should remain. People using v=token will effectively always be using the highest matrix version and hopefully that always continues working.

As far as #1203 (comment) goes, I can see the payload contains
the right information on the first pass, but obviously the token is obfuscated. I assume the token theyre using is valid. In that case, then the token is maybe not being set.

See https://github.com/caronc/apprise/pull/1236/files#diff-4389ebe118730ba468f8fc5780602cfe968c64076c77db4c89e007e041191eabR1482
https://github.com/caronc/apprise/pull/1203/files#diff-4389ebe118730ba468f8fc5780602cfe968c64076c77db4c89e007e041191eabR892

You're using password, I'm using user as token. It's possible both of those are valid, and this is something else? I didn't change the parsing, maybe that's all that's needed but I havent tried your code? Would you prefer if I tried yours?

My environment seems a bit different, so if you could just link the matrix.py on your branch I could debug it for you?

Maybe the person testing it doesn't have it set up to work?

When I originally built this, I started doing it your way. After reading the documentation, and the code, I decided it was quick to go to T2/ webhook mode, so I figured this was actually less complex, and maybe less confusing too?

@caronc
Copy link
Owner

caronc commented Nov 21, 2024

Personally, I no longer have Matrix so it was difficult for me to test. The fact you're engaged to fix tihs is amazing and I really want you to get all of he credit. I'd much rather close my PR and use yours.

However, I'm a bit picky 😉 .. And i feel your solution is so spot on...

the only thing i'd rather you do is leverage this block of code here vs introducing a new version.

I like the idea of matrix://<token>/ being the trigger vs matrix://<user>:<pass>/

All of the logic is in my PR to figure out if you're rolling with a Token or User/Pass (it's a wee bit more code, but allows us to catch/adjust if the token changes in future versions.

Is there a way you could check out my branch... steal all my code (please)... or whatever is valid within it... and just apply your efforts.

I can also see that i went off to make a call upstream with m.login.token (here which was were i went wrong.

Keep in mind that the url() class function will get a bit if/else like to generate the URL based on it being a token or a user/pass combo.

I'll help you with your unit testing; so don't worry about that. See if you can just make it work (and sorry to give you more work). I'm excited to merge your version vs mine quite honestly. 🙏

@voc0der
Copy link
Contributor Author

voc0der commented Nov 21, 2024

There, that also works. 1cd6f6a The problem is that you were trying to log in, when the token means you've already logged in. You just want to pass the token along with whatever you were going to do after.

The m.login.token spec is for another type of login, such as SSO implementation and QR code sharing from apps. Further, per the doc specs for m.login.token, you must provide a login token in that instance, and you get returned an access token. In the usage here, we are always passing an access token always. Consider that matrix sessions should be few (for sanity), so it seems pointless to add more if you already can obtain infinitely lived access tokens very easily.

1cd6f6a#diff-4389ebe118730ba468f8fc5780602cfe968c64076c77db4c89e007e041191eabR617

@voc0der
Copy link
Contributor Author

voc0der commented Nov 22, 2024

Removed whitespace to pass flake8

@caronc caronc merged commit f17df45 into caronc:master Nov 22, 2024
13 checks passed
@caronc
Copy link
Owner

caronc commented Nov 22, 2024

Merged; thank you 🙏

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.

2 participants