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: ユーザー登録を承認制にできるように #12852

Draft
wants to merge 48 commits into
base: develop
Choose a base branch
from

Conversation

kakkokari-gtyih
Copy link
Contributor

@kakkokari-gtyih kakkokari-gtyih commented Dec 29, 2023

What

Cherry-picked from https://activitypub.software/TransFem-org/Sharkey

Why

モデレーション強化
Fix #5957 / Fix #10334

Additional info (optional)

Sharkeyからの改良点

  • 日本語訳を修正
  • 紛らわしいキー名を一部変更
  • ログイン時に未承認ユーザーは弾くようになっていたが、その際のエラーコードが重複している問題を修正
  • マイグレーションの際に既存のユーザーは全員承認済みとしてマークするように
  • メッセージを丁寧にした
  • 通常のAPI呼び出し時にも承認状態を確認するようにした

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added packages/frontend Client side specific issue/PR packages/backend Server side specific issue/PR packages/misskey-js labels Dec 29, 2023
Copy link

codecov bot commented Dec 29, 2023

Codecov Report

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

Comparison is base (0f7918c) 64.38% compared to head (0e2e4c3) 65.73%.
Report is 9 commits behind head on develop.

Files Patch % Lines
...ackages/backend/src/server/api/SignupApiService.ts 2.81% 68 Missing and 1 partial ⚠️
...end/src/server/api/endpoints/admin/approve-user.ts 40.32% 33 Missing and 4 partials ⚠️
...ackages/backend/src/server/api/SigninApiService.ts 5.00% 18 Missing and 1 partial ⚠️
packages/backend/src/server/api/ApiCallService.ts 0.00% 10 Missing ⚠️
packages/backend/src/core/SignupService.ts 0.00% 8 Missing ⚠️
...kend/src/server/api/endpoints/admin/update-meta.ts 20.00% 4 Missing ⚠️
...ackend/src/server/api/endpoints/admin/show-user.ts 0.00% 2 Missing ⚠️
...end/src/core/activitypub/models/ApPersonService.ts 50.00% 1 Missing ⚠️
...ges/backend/src/core/entities/UserEntityService.ts 50.00% 1 Missing ⚠️
...ges/backend/src/server/api/endpoints/admin/meta.ts 80.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12852      +/-   ##
===========================================
+ Coverage    64.38%   65.73%   +1.34%     
===========================================
  Files          979      984       +5     
  Lines       109398   114294    +4896     
  Branches      5600     5638      +38     
===========================================
+ Hits         70439    75129    +4690     
- Misses       37525    37725     +200     
- Partials      1434     1440       +6     

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

@kakkokari-gtyih
Copy link
Contributor Author

これ既存ユーザーはどうなるんだろう

@anatawa12
Copy link
Member

を書き換えることができる(**)に関しては

email: user.email,
signupReason: user.signupReason,追加すれば良さそう

@kakkokari-gtyih
Copy link
Contributor Author

テ ス ト 通 過

@kakkokari-gtyih

This comment was marked as resolved.

@kakkokari-gtyih kakkokari-gtyih marked this pull request as ready for review December 30, 2023 02:46
@kakkokari-gtyih
Copy link
Contributor Author

この変更が適用されてないプレーンなMisskeyでの動作確認が必要

@kakkokari-gtyih
Copy link
Contributor Author

kakkokari-gtyih commented Dec 30, 2023

これログイン処理では承認済みかどうかのチェック入ってるけどその他の認証処理には入ってないので、何らかの方法でAPIトークンを取得できれば承認をバイパスできるな
(ただログインできなければそもそもAPIトークンの発行もできないので、そこを考慮する必要はないのかも…?)

@kakkokari-gtyih
Copy link
Contributor Author

これログイン処理では承認済みかどうかのチェック入ってるけどその他の認証処理には入ってないので、何らかの方法でAPIトークンを取得できれば承認をバイパスできるな (ただログインできなければそもそもAPIトークンの発行もできないので、そこを考慮する必要はないのかも…?)

通常のAPI呼び出し時にも承認状態を確認するようにした

@syuilo
Copy link
Member

syuilo commented Feb 8, 2024

テストあると有難いわね

@kakkokari-gtyih
Copy link
Contributor Author

kakkokari-gtyih commented Feb 8, 2024

@Mar0xy Have you coded (or do you plan to write) a test for this?
(あるならもらいたい)

@Mar0xy
Copy link
Contributor

Mar0xy commented Feb 8, 2024

@Mar0xy Have you coded (or do you plan to write) a test for this?
(あるならもらいたい)

We don't have a test for it nor is one being written/planned currently.

@kakkokari-gtyih
Copy link
Contributor Author

kakkokari-gtyih commented Feb 8, 2024

あんまり意味ないとは思うけど、同一IPからの複数申請は弾く(一つ承認されないと申請できない)ようにしても良いかも

@kakkokari-gtyih
Copy link
Contributor Author

承認制と招待コード両方が有効になってるときの処理が入ってなかった

@kakkokari-gtyih
Copy link
Contributor Author

基本的に招待コード持ってたら審査バイパスでいいと思っている

@kakkokari-gtyih kakkokari-gtyih marked this pull request as draft February 9, 2024 08:59
Copy link
Contributor

github-actions bot commented Feb 9, 2024

Thank you for sending us a great Pull Request! 👍
Please regenerate misskey-js type definitions! 🙏

example:

pnpm run build-misskey-js-with-types

@MomentQYC
Copy link
Contributor

Is the development of this feature stalled? I believe it would be more practical to implement a registration reason feature rather than switching to an invite-only system after a period of administrator inactivity.

@MomentQYC
Copy link
Contributor

Is the development of this feature stalled? I believe it would be more practical to implement a registration reason feature rather than switching to an invite-only system after a period of administrator inactivity.

@kakkokari-gtyih

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend:test packages/backend Server side specific issue/PR packages/frontend Client side specific issue/PR packages/misskey-js
Projects
Development

Successfully merging this pull request may close these issues.

Manual registration confirmation Any plans on Signup Modes? (User generated invite vs. Admin invite)
5 participants