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

Appservice Login (2nd attempt) #3078

Merged
merged 29 commits into from
Nov 24, 2023
Merged

Conversation

kuhnchris
Copy link
Contributor

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>

@kuhnchris kuhnchris requested a review from a team as a code owner May 7, 2023 22:16
@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (b8f9148) 65.35% compared to head (aa23baa) 65.34%.

Files Patch % Lines
internal/validate.go 89.74% 6 Missing and 2 partials ⚠️
clientapi/auth/login_application_service.go 61.53% 4 Missing and 1 partial ⚠️
clientapi/routing/register.go 0.00% 4 Missing and 1 partial ⚠️
clientapi/routing/receipt.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3078      +/-   ##
==========================================
- Coverage   65.35%   65.34%   -0.01%     
==========================================
  Files         507      508       +1     
  Lines       57285    57388     +103     
==========================================
+ Hits        37440    37502      +62     
- Misses      15969    16003      +34     
- Partials     3876     3883       +7     
Flag Coverage Δ
unittests 49.72% <83.06%> (+0.05%) ⬆️

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

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

@bertybuttface
Copy link

I'd really appreciate being able to use this if it were to be merged. Is there anything specific holding it up?

@mjholub
Copy link
Contributor

mjholub commented Jun 18, 2023

Same. @S7evinK @devonh all checks are passing, could you merge this soon?

@S7evinK
Copy link
Contributor

S7evinK commented Jun 30, 2023

@kuhnchris can you take a look at the linter issues?

vijfhoek and others added 21 commits June 30, 2023 12:28
Signed-off-by: Sijmen <me@sijman.nl>
Signed-off-by: Sijmen Schoon <me@sijman.nl>
Signed-off-by: Sijmen <me@sijman.nl>
Signed-off-by: Sijmen Schoon <me@sijman.nl>
Signed-off-by: Sijmen <me@sijman.nl>
Signed-off-by: Sijmen Schoon <me@sijman.nl>
Signed-off-by: Sijmen <me@sijman.nl>
Signed-off-by: Sijmen Schoon <me@sijman.nl>
Signed-off-by: Sijmen <me@sijman.nl>
Signed-off-by: Sijmen Schoon <me@sijman.nl>
Signed-off-by: Sijmen Schoon <me@sijman.nl>
@kuhnchris
Copy link
Contributor Author

Done, now the linter is... broken?

internal/validate_test.go Outdated Show resolved Hide resolved
@kuhnchris
Copy link
Contributor Author

I'll try fixing it locally and let you know when I get it ready... this seems to take a bit more work

@kuhnchris
Copy link
Contributor Author

No idea why this fixes the issue, it just did...


kuhnchris@insanity ~/dendrite/internal $ ~/go/bin/golangci-lint -v -j 2 run
INFO [config_reader] Config search paths: [./ /home/kuhnchris/dendrite/internal /home/kuhnchris/dendrite /home/kuhnchris /home /]
INFO [config_reader] Used config file ../.golangci.yml
INFO [lintersdb] Active 12 linters: [errcheck goconst gocyclo goimports gosimple govet ineffassign misspell nakedret staticcheck unparam unused]
INFO [loader] Go packages loading at mode 575 (name|exports_file|files|imports|types_sizes|compiled_files|deps) took 4.500304439s
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 57.30882ms
INFO [linters_context/goanalysis] analyzers took 5.300965006s with top 10 stages: buildssa: 801.739929ms, buildir: 800.635345ms, goimports: 621.570239ms, S1038: 451.369521ms, misspell: 379.10888ms, unused: 275.253275ms, S1039: 204.310459ms, printf: 167.90403ms, SA4030: 160.287719ms, SA1012: 150.981751ms
INFO [runner] Issues before processing: 6, after processing: 0
INFO [runner] Processors filtering stat (out/in): skip_files: 6/6, skip_dirs: 6/6, exclude-rules: 5/6, cgo: 6/6, filename_unadjuster: 6/6, autogenerated_exclude: 6/6, identifier_marker: 6/6, path_prettifier: 6/6, exclude: 6/6, nolint: 0/5
INFO [runner] processing took 31.838392ms with stages: nolint: 28.749287ms, identifier_marker: 873.377µs, autogenerated_exclude: 851.216µs, path_prettifier: 697.486µs, skip_dirs: 253.173µs, exclude-rules: 224.261µs, skip_files: 131.782µs, fixer: 19.425µs, cgo: 12.656µs, filename_unadjuster: 12.419µs, max_same_issues: 3.446µs, uniq_by_line: 1.86µs, max_from_linter: 1.249µs, diff: 1.182µs, source_code: 1.122µs, path_shortener: 963ns, sort_results: 855ns, exclude: 851ns, severity-rules: 796ns, max_per_file_from_linter: 521ns, path_prefixer: 465ns
INFO [runner] linters took 3.524843857s with stages: goanalysis_metalinter: 3.492193385s
INFO File cache stats: 10 entries of total size 72.2KiB
INFO Memory: 83 samples, avg is 58.5MB, max is 148.5MB
INFO Execution took 8.11431403s

Copy link
Contributor

@S7evinK S7evinK left a comment

Choose a reason for hiding this comment

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

A few minor things, but aside from that looks good.

Also, thanks for adding tests! :)

clientapi/routing/login.go Outdated Show resolved Hide resolved
clientapi/auth/login.go Show resolved Hide resolved
@kuhnchris
Copy link
Contributor Author

It's not my code, it's all thanks to @vijfhoek s work, I only did the ff/rebase work, hence I only have limited knowledge of the change, plus, my go know-how is still very limited, but I try my best to fix whatever comes up. :-)

clientapi/routing/login.go Outdated Show resolved Hide resolved
Co-authored-by: Till <2353100+S7evinK@users.noreply.github.com>
@vijfhoek vijfhoek mentioned this pull request Jul 3, 2023
2 tasks
Copy link
Contributor

@mjholub mjholub left a comment

Choose a reason for hiding this comment

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

A bit late for a review, but the notification about the merge of main made me notice you can just do

if len(cfg.Derived.ApplicationServices) > 0 {
		loginFlows = append(loginFlows, flow{Type: authtypes.LoginTypeApplicationService})
	}

and skip the zero length check since you're already declaring the loginFlows slice with authtypes.LoginTypePassword before the conditional statements

@S7evinK
Copy link
Contributor

S7evinK commented Sep 13, 2023

@devonh I was basically happy with this, but maybe you can also have a look?

@S7evinK S7evinK mentioned this pull request Sep 29, 2023
16 tasks
@S7evinK
Copy link
Contributor

S7evinK commented Nov 24, 2023

Let's finally land this..

Issue for the flake in Complement: #3273

Copy link
Contributor

@S7evinK S7evinK left a comment

Choose a reason for hiding this comment

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

Edit: Comment was not intentional. :D

Copy link
Collaborator

@devonh devonh left a comment

Choose a reason for hiding this comment

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

Changes look thought out and have decent test coverage.

@S7evinK S7evinK merged commit 4f94377 into matrix-org:main Nov 24, 2023
20 checks passed
@S7evinK
Copy link
Contributor

S7evinK commented Nov 24, 2023

Thank you very much @kuhnchris and @vijfhoek for this!

@kuhnchris
Copy link
Contributor Author

Nice, thanks for the merge, in the meantime I'll try to get the mautrix bridges working, which indirectly also is affected by appservices, but I'll create a separate PR for that - again thanks for pushing through with it. :-) 👍

@necropola
Copy link

Does that mean that End-to-bridge encryption is now working with mautrix bridges and dendrite (master)? I think, I won't need this, because I'm planning to run both the mautrix bridges as well as the dendrite homeserver on the same machine, but it would be nice for those who don't.

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.

None yet

7 participants