Do not try to login if the user authenticated via other means #69
Conversation
Codecov Report
@@ Coverage Diff @@
## master #69 +/- ##
==========================================
+ Coverage 27.10% 27.11% +0.01%
==========================================
Files 10 10
Lines 2118 2124 +6
==========================================
+ Hits 574 576 +2
- Misses 1544 1548 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Could you expand a bit on how you are doing your log in. Will this be once per bridge or once per user. I think this is changing the way pan works since it might not have its own separate device now. There were reasons for having a separate pan device per user but I can't recall them from the top of my head right now.
A changelog entry would be nice as well.
| if password is "": | ||
| # If password is blank, we cannot login normally and must | ||
| # fall back to using the provided device_id. | ||
| pan_client.restore_login(user_id, device_id, access_token) |
There was a problem hiding this comment.
device_id can be None here. We should handle the None case either here or when getting it out of the response.
There was a problem hiding this comment.
I've added an exception, though feel free to suggest a better type / message for the exception.
There was a problem hiding this comment.
I don't think an exception is the right thing to do here since nobody will catch it up the stack either.
Returning and adding a log line here should be enough.
Certainly. Each bridge "ghost" user will need to login and sync seperately, like real users and provide their own devices. Since bridge can't usually use /login at all, I've written matrix-org/synapse#8320 as a special login We could extend pan to support different types of authentication, if you think we need to have a special pan device. |
Half-Shot
force-pushed
the
hs/bridges-pan
branch
from
fc1cd6e
to
587af85
Compare
Half-Shot
force-pushed
the
hs/bridges-pan
branch
from
587af85
to
bb9408c
Compare
There was a problem hiding this comment.
I read a bit through the code and I think it's possible that we handle the case where we don't have a separate device correctly, even if we don't since this is a special case for bridges we can take this and tests things out without having a bad conscience.
Please just fix the raise -> return and we can merge this.
| if password is "": | ||
| # If password is blank, we cannot login normally and must | ||
| # fall back to using the provided device_id. | ||
| pan_client.restore_login(user_id, device_id, access_token) |
There was a problem hiding this comment.
I don't think an exception is the right thing to do here since nobody will catch it up the stack either.
Returning and adding a log line here should be enough.
Co-authored-by: poljar <poljar@termina.org.uk>
The plan for encrypted bridges is for them to authenticate using access tokens rather than a password, which means that when Pan attempts to login with a provided password, it falls over. I'm not terribly happy with this solution, but it's does make things work again.