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

enhance: introduce testCaptcha #14745

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

enhance: introduce testCaptcha #14745

wants to merge 1 commit into from

Conversation

syuilo
Copy link
Member

@syuilo syuilo commented Oct 10, 2024

What

Resolve #14740

Why

Additional info (optional)

Animation3

無理やりsubmitしてもサーバーサイドのtestCaptchaの検証で弾かれてエラーになる
Animation3

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 Oct 10, 2024
Copy link
Contributor

このPRによるapi.jsonの差分

差分はこちら
--- base
+++ head
@@ -90,6 +90,9 @@
                         "null"
                       ]
                     },
+                    "enableTestcaptcha": {
+                      "type": "boolean"
+                    },
                     "swPublickey": {
                       "type": [
                         "string",
@@ -582,6 +585,7 @@
                     "recaptchaSiteKey",
                     "enableTurnstile",
                     "turnstileSiteKey",
+                    "enableTestcaptcha",
                     "swPublickey",
                     "mascotImageUrl",
                     "bannerUrl",
@@ -13961,6 +13965,9 @@
                       "null"
                     ]
                   },
+                  "enableTestcaptcha": {
+                    "type": "boolean"
+                  },
                   "sensitiveMediaDetection": {
                     "type": "string",
                     "enum": [
@@ -81952,6 +81959,9 @@
               "null"
             ]
           },
+          "enableTestcaptcha": {
+            "type": "boolean"
+          },
           "swPublickey": {
             "type": [
               "string",
@@ -82136,6 +82146,7 @@
           "recaptchaSiteKey",
           "enableTurnstile",
           "turnstileSiteKey",
+          "enableTestcaptcha",
           "swPublickey",
           "mascotImageUrl",
           "bannerUrl",

Get diff files from Workflow Page

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 15.09434% with 90 lines in your changes missing coverage. Please review.

Project coverage is 41.31%. Comparing base (d376aab) to head (f6eb49e).

Files with missing lines Patch % Lines
packages/frontend/src/components/MkCaptcha.vue 0.00% 26 Missing ⚠️
...ckages/frontend/src/pages/admin/bot-protection.vue 0.00% 14 Missing ⚠️
packages/backend/src/core/CaptchaService.ts 15.38% 11 Missing ⚠️
...ages/frontend/src/components/MkSignin.password.vue 0.00% 8 Missing ⚠️
...ackages/backend/src/server/api/SigninApiService.ts 0.00% 7 Missing ⚠️
...ackages/backend/src/server/api/SignupApiService.ts 0.00% 7 Missing ⚠️
...es/frontend/src/components/MkSignupDialog.form.vue 0.00% 6 Missing ⚠️
...kend/src/server/api/endpoints/admin/update-meta.ts 20.00% 4 Missing ⚠️
packages/frontend/src/components/MkSignin.vue 0.00% 3 Missing ⚠️
...ackages/backend/src/server/api/ApiServerService.ts 0.00% 2 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #14745      +/-   ##
===========================================
+ Coverage    39.63%   41.31%   +1.68%     
===========================================
  Files         1553     1557       +4     
  Lines       195038   200901    +5863     
  Branches      3609     3656      +47     
===========================================
+ Hits         77300    83010    +5710     
- Misses      117136   117289     +153     
  Partials       602      602              

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

@syuilo syuilo marked this pull request as ready for review October 10, 2024 09:52
@syuilo
Copy link
Member Author

syuilo commented Oct 10, 2024

レビュワーを募集しております

Copy link
Member

@anatawa12 anatawa12 left a comment

Choose a reason for hiding this comment

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

動作未確認です

Copy link
Member

Choose a reason for hiding this comment

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

このコンポーネントはCaptchaの抽象層だと思うのでTestCaptchaの実装は別コンポーネントにしたいきがするけどいい切り分け単位も思いつかない

@@ -113,6 +119,7 @@ function resetCaptcha() {
mCaptcha.value?.reset();
reCaptcha.value?.reset();
turnstile.value?.reset();
testcaptcha.value?.reset();
Copy link
Member

Choose a reason for hiding this comment

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

testcaptcha.value?.reset がundefinedになる可能性あると思うけどここでは?.()になってないのが気になった。

packages/frontend/src/components/MkSignupDialog.form.vue では reset?.()となってる

@@ -29,7 +40,7 @@ export type Captcha = {
getResponse(id: string): string;
};
Copy link
Member

Choose a reason for hiding this comment

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

元をたどるとCaptcha.resetメソッドがoptionalと定義されてないのが悪そうかも

Copy link
Contributor

Choose a reason for hiding this comment

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

vueのexposeのほうのresetを発火しているはずなので、外部から参照されるresetがundefinedになる心配はなさそう

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

Successfully merging this pull request may close these issues.

CAPTHCAのモック実装を使えるようにする
3 participants