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

feat: admin APIs for token authenticated registration #3101

Merged
merged 28 commits into from
Jun 22, 2023

Conversation

santhoshivan23
Copy link
Contributor

@santhoshivan23 santhoshivan23 commented Jun 3, 2023

Pull Request Checklist

Signed-off-by: Santhoshivan Amudhan santhoshivan23@gmail.com

@santhoshivan23 santhoshivan23 requested a review from a team as a code owner June 3, 2023 16:21
@santhoshivan23 santhoshivan23 marked this pull request as draft June 3, 2023 16:49
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.

As there are no tests in Sytest, would be awesome if you could add tests for this in Dendrite. :)

setup/config/config_clientapi.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 4, 2023

Codecov Report

Patch coverage: 75.75% and project coverage change: +0.11 🎉

Comparison is base (a734b11) 65.54% compared to head (04ca62a) 65.65%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3101      +/-   ##
==========================================
+ Coverage   65.54%   65.65%   +0.11%     
==========================================
  Files         502      504       +2     
  Lines       55192    55753     +561     
==========================================
+ Hits        36174    36604     +430     
- Misses      15256    15356     +100     
- Partials     3762     3793      +31     
Flag Coverage Δ
unittests 49.96% <75.75%> (+0.32%) ⬆️

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

Impacted Files Coverage Δ
userapi/api/api.go 70.90% <ø> (ø)
userapi/storage/postgres/storage.go 50.35% <40.00%> (-0.39%) ⬇️
userapi/storage/sqlite3/storage.go 50.35% <40.00%> (-0.39%) ⬇️
clientapi/routing/admin.go 62.97% <68.22%> (+5.35%) ⬆️
userapi/internal/user_api.go 64.80% <71.42%> (+0.19%) ⬆️
clientapi/routing/routing.go 64.66% <73.91%> (+0.20%) ⬆️
...rapi/storage/postgres/registration_tokens_table.go 81.34% <81.34%> (ø)
...erapi/storage/sqlite3/registration_tokens_table.go 81.34% <81.34%> (ø)
setup/config/config_clientapi.go 73.68% <100.00%> (+0.46%) ⬆️
userapi/storage/shared/storage.go 78.22% <100.00%> (+0.87%) ⬆️

... and 7 files with indirect coverage changes

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

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.

Needs a few changes, but this is promising :)

Comments for Postgres should also apply to SQLite.
As for the database functions, there is a bit of inconsistency here: Sometimes you're using sqlutil.TxStmt() and sometimes you're using the "raw" statement.

clientapi/routing/routing.go Outdated Show resolved Hide resolved
clientapi/routing/routing.go Outdated Show resolved Hide resolved
userapi/internal/user_api.go Outdated Show resolved Hide resolved
userapi/storage/postgres/registration_tokens_table.go Outdated Show resolved Hide resolved
userapi/storage/postgres/registration_tokens_table.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
userapi/storage/postgres/registration_tokens_table.go Outdated Show resolved Hide resolved
userapi/storage/postgres/registration_tokens_table.go Outdated Show resolved Hide resolved
@santhoshivan23
Copy link
Contributor Author

Needs a few changes, but this is promising :)

Comments for Postgres should also apply to SQLite. As for the database functions, there is a bit of inconsistency here: Sometimes you're using sqlutil.TxStmt() and sometimes you're using the "raw" statement.

Thanks @S7evinK - let me work on the comments

@santhoshivan23
Copy link
Contributor Author

@S7evinK Thanks for your valuable comments. I've addressed all of them

@santhoshivan23 santhoshivan23 marked this pull request as ready for review June 17, 2023 06:23
@santhoshivan23
Copy link
Contributor Author

@S7evinK PR is ready for review!

@santhoshivan23 santhoshivan23 changed the title feat: token authenticated registration feat: admin APIs for token authenticated registration Jun 17, 2023
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.

Didn't have a look at the tests yet, doing that tomorrow.

clientapi/routing/admin.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
@santhoshivan23
Copy link
Contributor Author

@S7evinK - addressed all the comments.

@devonh
Copy link
Collaborator

devonh commented Jun 21, 2023

I added 2 comments about flipping logic around. Otherwise, once the linter errors are fixed this should be good to merge.

clientapi/routing/admin.go Outdated Show resolved Hide resolved
clientapi/routing/admin.go Outdated Show resolved Hide resolved
@santhoshivan23
Copy link
Contributor Author

thanks @devonh @S7evinK for the review. i've addressed the comments. hope we can merge this now

@devonh devonh merged commit 45082d4 into matrix-org:main Jun 22, 2023
20 checks passed
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.

3 participants