Appservice login #2936

vijfhoek · 2023-01-10T16:50:42Z

This is my first contribution to this code base, so I hope I did everything correctly :)

I did move validateApplicationService (now ValidateApplicationService) from routing to internal to be able to reuse it - is this the right place for it?

Pull Request Checklist

I have added Go unit tests or Complement integration tests for this PR or I have justified why this PR doesn't need tests
Pull request includes a sign off below using a legally identifiable name or I have already signed off privately

Signed-off-by: Sijmen Schoon <me@sijman.nl>

codecov · 2023-01-10T19:00:16Z

Codecov Report

Base: 40.30% // Head: 40.47% // Increases project coverage by +0.17% 🎉

Coverage data is based on head (dc14738) compared to base (2debabf).
Patch coverage: 85.59% of modified lines in pull request are covered.

❗ Current head dc14738 differs from pull request most recent head 902eb1e. Consider uploading reports for the commit 902eb1e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2936      +/-   ##
==========================================
+ Coverage   40.30%   40.47%   +0.17%     
==========================================
  Files         519      520       +1     
  Lines       56436    56457      +21     
==========================================
+ Hits        22745    22851     +106     
+ Misses      30968    30896      -72     
+ Partials     2723     2710      -13

Flag	Coverage Δ
unittests	`40.47% <85.59%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
clientapi/auth/user_interactive.go	`78.62% <ø> (ø)`
clientapi/routing/register.go	`62.18% <0.00%> (+0.41%)`	⬆️
clientapi/auth/login_application_service.go	`61.53% <61.53%> (ø)`
internal/validate.go	`92.18% <87.34%> (-7.82%)`	⬇️
clientapi/auth/login.go	`86.27% <100.00%> (+4.69%)`	⬆️
clientapi/routing/login.go	`69.49% <100.00%> (+21.95%)`	⬆️
roomserver/storage/sqlite3/event_json_table.go	`80.85% <0.00%> (-6.39%)`	⬇️
syncapi/streams/stream_presence.go	`58.26% <0.00%> (-2.37%)`	⬇️
syncapi/notifier/notifier.go	`62.46% <0.00%> (-0.78%)`	⬇️
roomserver/internal/query/query.go	`31.44% <0.00%> (ø)`
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

clientapi/routing/login.go

Signed-off-by: Sijmen <me@sijman.nl> Signed-off-by: Sijmen Schoon <me@sijman.nl>

Signed-off-by: Sijmen Schoon <me@sijman.nl>

…pace

S7evinK

Looks good to me! Thanks for adding tests as well :)

clientapi/auth/login_test.go

kegsay

Otherwise this looks great, thanks!

internal/validate_test.go

kegsay · 2023-01-27T13:55:45Z

internal/validate_test.go

+	// Access token is correct, acting as user_id that matches but is invalid
+	asID, resp = ValidateApplicationService(&fakeConfig.ClientAPI, "@_appservice_bob::", fakeApplicationService.ASToken)
+	if resp == nil || asID == fakeApplicationService.ID {
+		t.Errorf("user_id should not have been valid: @_appservice_bob::")


Good tests overall, but please make them table-driven as it's significantly easier to see the inputs / expected outputs https://dave.cheney.net/2019/05/07/prefer-table-driven-tests

Something like this?

internal/validate.go

kegsay · 2023-01-27T14:06:15Z

clientapi/routing/login.go

+			JSON: flows{
+				Flows: []flow{
+					{Type: authtypes.LoginTypePassword},
+					{Type: authtypes.LoginTypeApplicationService},


This is only true if Dendrite has been configured with appservices surely.

kegsay · 2023-03-06T15:33:23Z

Please fix the linter errors.

sebastian-de · 2023-03-24T08:01:59Z

Hey @vijfhoek, thanks for your work - I'm currently testing your PR on my dendrite instance with mautrix-signal and so far it works great!

I rebased your PR on the current main branch: https://github.com/sebastian-de/dendrite/commits/appservice

Feel free to use this tree, if you'd like to push again.

sebastian-de · 2023-03-24T13:39:59Z

It seems mautrix-signal has problems to connect, when encryption is enabled. This a log from mautrix-signal.service:

[2023-03-24 13:21:20,969] [INFO@mau.user.@user:example.com] Automatically enabling custom puppet
[2023-03-24 13:21:20,979] [INFO@mau.init] Startup actions complete in 0.2 seconds, now running forever
[2023-03-24 13:21:20,983] [WARNING@mau.user.@user:example.com] New IDENTIFIED websocket state from signald CONNECTING
[2023-03-24 13:21:20,987] [WARNING@mau.user.@user:example.com] New UNIDENTIFIED websocket state from signald CONNECTING
[2023-03-24 13:21:21,110] [WARNING@mau.user.@user:example.com] Failed to enable custom puppet: Didn't get an access token: The username or password was incorrect or the account does not exist.

The dendrite logs don't reveal anything regarding that error.
The user does exist and I'm using registration_shared_secret which worked well so far with mautrix-signal.
Are there additional configuration steps neccessary to make appservice login work?

sebastian-de · 2023-03-25T11:11:39Z

It seems mautrix-signal has problems to connect

This was a configuration error in mautrix-signal - the bridge seems to mix things up, when shared secret auth is enabled.
Sorry for the noise.

Fijxu · 2023-04-06T00:59:47Z

I was trying this branch with encryption support but it seems to fails for me using mautrix-whatsapp.

The bot returns this when trying to do a request to _matrix/client/v3/sendToDevice/m.room_key_request/mautrix-go

{
    "errcode": "M_UNKNOWN_TOKEN",
    "error": "Unknown token"
}

and it fails retrieving encryption keys for the encrypted chat. Although it connects good to the Dendrite instance and it detects that the server supports appservice encryption. And in the configuration file of the Bot i need to change from appservice: false to appservice: true or it will fail to connect to the instance.

https://github.com/mautrix/whatsapp/blob/master/example-config.yaml#L353

kegsay · 2023-04-25T10:43:20Z

This PR is at risk of being closed. Unfortunately the lint errors haven't been fixed yet, and now there's a lot of merge conflicts.

vijfhoek · 2023-04-26T20:30:33Z

I haven't had too much time lately, sorry. The merge conflicts seem to be fairly simple to solve at first sight though

kuhnchris · 2023-05-10T05:39:12Z

Hey @vijfhoek - hope you're doing good - I took the liberty to rebase your branch and opened a new PR, if you'd prefer to have it in your repo, let me know and I'll add a PR to that instead. Thanks for your work and cheers ❤️

vijfhoek · 2023-07-03T16:14:48Z

Closing because this one's superseded by #3078

@vijfhoek

Rebase of #2936 as @vijfhoek wrote he got no time to work on this, and I kind of needed it for my experiments. I checked the tests, and it is working with my example code (i.e. impersonating, registering, creating channel, invite people, write messages). I'm not a huge `go` pro, and still learning, but I tried to fix and/or integrate the changes as best as possible with the current `main` branch changes. If there is anything left, let me know and I'll try to figure it out. Signed-off-by: `Kuhn Christopher <kuhnchris+git@kuhnchris.eu>` --------- Signed-off-by: Sijmen <me@sijman.nl> Signed-off-by: Sijmen Schoon <me@sijman.nl> Co-authored-by: Sijmen Schoon <me@sijman.nl> Co-authored-by: Sijmen Schoon <me@vijf.life> Co-authored-by: Till <2353100+S7evinK@users.noreply.github.com>

vijfhoek requested a review from a team as a code owner January 10, 2023 16:50

vijfhoek force-pushed the sijmens/appservice_login branch from b534c90 to 3cd0920 Compare January 10, 2023 18:27

vijfhoek commented Jan 10, 2023

View reviewed changes

clientapi/routing/login.go Outdated Show resolved Hide resolved

vijfhoek force-pushed the sijmens/appservice_login branch 2 times, most recently from c52f212 to 23fa52f Compare January 11, 2023 09:26

vijfhoek added 8 commits January 15, 2023 12:39

Replace "m.login.password" with authtypes constant

c335f6d

Signed-off-by: Sijmen <me@sijman.nl> Signed-off-by: Sijmen Schoon <me@sijman.nl>

Add LoginTypeApplicationService to GET /login

df6cd1a

Signed-off-by: Sijmen <me@sijman.nl> Signed-off-by: Sijmen Schoon <me@sijman.nl>

Move ValidateApplicationService to a separate package for reuse

6874539

Signed-off-by: Sijmen <me@sijman.nl> Signed-off-by: Sijmen Schoon <me@sijman.nl>

Implement POST /login for LoginTypeApplicationService

3ec2c67

Signed-off-by: Sijmen <me@sijman.nl> Signed-off-by: Sijmen Schoon <me@sijman.nl>

Make ValidateApplicationService work with UserIDs and move to internal

2470985

Signed-off-by: Sijmen <me@sijman.nl> Signed-off-by: Sijmen Schoon <me@sijman.nl>

Add tests for m.login.application_service

4f963dd

Signed-off-by: Sijmen Schoon <me@sijman.nl>

Improve ValidateApplicationService test coverage

40938d2

Remove dead, untested code from userIDIsWithinApplicationServiceNames…

11e16a7

…pace

vijfhoek force-pushed the sijmens/appservice_login branch from 23fa52f to 11e16a7 Compare January 15, 2023 11:40

Simplify GET /login and add test for it

0e657c9

S7evinK added C-Client-API spec-compliance Fix something that doesn't comply with the specs T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. F-App-Service labels Jan 16, 2023

S7evinK added 2 commits January 26, 2023 15:27

Merge branch 'main' into sijmens/appservice_login

49eac14

Merge branch 'main' into sijmens/appservice_login

dc14738

S7evinK approved these changes Jan 27, 2023

View reviewed changes

clientapi/auth/login_test.go Outdated Show resolved Hide resolved

clientapi/auth/login_test.go Outdated Show resolved Hide resolved

kegsay requested changes Jan 27, 2023

View reviewed changes

vijfhoek added 6 commits January 27, 2023 15:14

Fix typo "POSTGERS" in CONTRIBUTING.md

2975279

Replace handcrafted genHTTPRequest with httptest.NewRequest

b4e472a

Improve documentation and naming

0b06052

Move UsernameMatchesExclusiveNamespaces back to register.go

cc62f8e

Convert TestValidationOfApplicationServices to be table-driven

23f5d7d

Update TestValidationOfApplicationServices function name

902eb1e

kegsay added the stale This issue or PR is at risk of being closed without further feedback label Apr 25, 2023

kuhnchris mentioned this pull request May 7, 2023

Appservice Login (2nd attempt) #3078

Merged

vijfhoek closed this Jul 3, 2023

jjj333-p mentioned this pull request Jul 23, 2023

Implement appservice login #2723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Appservice login #2936

Appservice login #2936

vijfhoek commented Jan 10, 2023 •

edited

Loading

codecov bot commented Jan 10, 2023 •

edited

Loading

S7evinK left a comment

kegsay left a comment

kegsay Jan 27, 2023

vijfhoek Jan 27, 2023

kegsay Jan 27, 2023

kegsay commented Mar 6, 2023

sebastian-de commented Mar 24, 2023

sebastian-de commented Mar 24, 2023

sebastian-de commented Mar 25, 2023

Fijxu commented Apr 6, 2023

kegsay commented Apr 25, 2023

vijfhoek commented Apr 26, 2023

kuhnchris commented May 10, 2023

vijfhoek commented Jul 3, 2023 •

edited

Loading

Appservice login #2936

Appservice login #2936

Conversation

vijfhoek commented Jan 10, 2023 • edited Loading

Pull Request Checklist

codecov bot commented Jan 10, 2023 • edited Loading

Codecov Report

S7evinK left a comment

Choose a reason for hiding this comment

kegsay left a comment

Choose a reason for hiding this comment

kegsay Jan 27, 2023

Choose a reason for hiding this comment

vijfhoek Jan 27, 2023

Choose a reason for hiding this comment

kegsay Jan 27, 2023

Choose a reason for hiding this comment

kegsay commented Mar 6, 2023

sebastian-de commented Mar 24, 2023

sebastian-de commented Mar 24, 2023

sebastian-de commented Mar 25, 2023

Fijxu commented Apr 6, 2023

kegsay commented Apr 25, 2023

vijfhoek commented Apr 26, 2023

kuhnchris commented May 10, 2023

vijfhoek commented Jul 3, 2023 • edited Loading

vijfhoek commented Jan 10, 2023 •

edited

Loading

codecov bot commented Jan 10, 2023 •

edited

Loading

vijfhoek commented Jul 3, 2023 •

edited

Loading