From 4c7cbb08dcea5d17b491d0e240c88640019d0832 Mon Sep 17 00:00:00 2001 From: lyan <49744633+zds-s@users.noreply.github.com> Date: Wed, 15 Jan 2025 20:42:15 +0800 Subject: [PATCH] refactor(http): optimize request authorization and validation (#532) * refactor(http): optimize request authorization and validation - Remove redundant authorize() methods from request classes - Add NoAuthorizeTrait for consistent authorization logic - Implement HttpMethodTrait for HTTP method checks - Update RoleRequest with custom validation rules for code - Add token retrieval failure check in GetTokenTrait - Remove unnecessary comments and adjust imports * fix(permission): prevent broken relations after deleting menus or roles - Add deleting event handler in Menu model to detach associated roles - Update Role model to also detach associated menus when deleting * ci: update codecov file path- Change the file path for codecov action from "./tests/coverage/index.xml" to "./tests/coverage.xml/index.xml" * refactor(request): move request traits to Traits directory - Move NoAuthorizeTrait and HttpMethodTrait to Traits directory - Update namespace from Trait to Traits in multiple files - Adjust import statements in various request files to use new namespace * update UserRequest rules * ci: update Codecov and remove CodeQL workflows - Update Codecov workflow to use new file path - Remove CodeQL workflow as it's no longer needed * ci: update code coverage and simplify phone number validation - Update .gitignore to include tests/coverage directory - Modify GitHub Actions workflow to use correct coverage file path - Remove regex validation for phone number in UserRequest --- .github/workflows/code-coverage.yml | 2 +- .github/workflows/codeql.yml | 95 ------------------- .gitignore | 1 + .../Admin/Request/PassportLoginRequest.php | 7 +- .../BatchGrantPermissionsForRoleRequest.php | 6 +- .../BatchGrantRolesForUserRequest.php | 6 +- .../Admin/Request/Permission/MenuRequest.php | 6 +- .../Request/Permission/PermissionRequest.php | 6 +- .../Admin/Request/Permission/RoleRequest.php | 24 +++-- .../Admin/Request/Permission/UserRequest.php | 10 +- app/Http/Admin/Request/UploadRequest.php | 6 +- app/Http/Api/Request/V1/UserRequest.php | 6 +- .../Common/Request/Traits/HttpMethodTrait.php | 41 ++++++++ .../Request/Traits/NoAuthorizeTrait.php | 21 ++++ app/Model/Permission/Menu.php | 6 ++ app/Model/Permission/Role.php | 7 ++ app/Model/Permission/User.php | 1 - tests/Feature/Admin/GetTokenTrait.php | 3 + .../Admin/Permission/RoleControllerTest.php | 7 +- tests/HttpTestCase.php | 2 +- 20 files changed, 123 insertions(+), 140 deletions(-) delete mode 100755 .github/workflows/codeql.yml create mode 100644 app/Http/Common/Request/Traits/HttpMethodTrait.php create mode 100644 app/Http/Common/Request/Traits/NoAuthorizeTrait.php diff --git a/.github/workflows/code-coverage.yml b/.github/workflows/code-coverage.yml index 277f0f4b..79cd0725 100755 --- a/.github/workflows/code-coverage.yml +++ b/.github/workflows/code-coverage.yml @@ -84,4 +84,4 @@ jobs: uses: codecov/codecov-action@v4-beta with: token: ${{ secrets.CODECOV_TOKEN }} - file: "./tests/coverage/index.xml" \ No newline at end of file + file: "tests/coverage/index.xml" \ No newline at end of file diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml deleted file mode 100755 index 0ed953aa..00000000 --- a/.github/workflows/codeql.yml +++ /dev/null @@ -1,95 +0,0 @@ -# For most projects, this workflow file will not need changing; you simply need -# to commit it to your repository. -# -# You may wish to alter this file to override the set of languages analyzed, -# or to provide custom queries or build logic. -# -# ******** NOTE ******** -# We have attempted to detect the languages in your repository. Please check -# the `language` matrix defined below to confirm you have the correct set of -# supported CodeQL languages. -# -name: "CodeQL" - -on: - push: - pull_request: - schedule: - - cron: '0 2 * * *' - -jobs: - analyze: - name: Analyze (${{ matrix.language }}) - # Runner size impacts CodeQL analysis time. To learn more, please see: - # - https://gh.io/recommended-hardware-resources-for-running-codeql - # - https://gh.io/supported-runners-and-hardware-resources - # - https://gh.io/using-larger-runners (GitHub.com only) - # Consider using larger runners or machines with greater resources for possible analysis time improvements. - runs-on: ${{ (matrix.language == 'swift' && 'macos-latest') || 'ubuntu-latest' }} - timeout-minutes: ${{ (matrix.language == 'swift' && 120) || 360 }} - permissions: - # required for all workflows - security-events: write - - # required to fetch internal or private CodeQL packs - packages: read - - # only required for workflows in private repositories - actions: read - contents: read - - strategy: - fail-fast: false - matrix: - include: - - language: javascript-typescript - build-mode: none - # CodeQL supports the following values keywords for 'language': 'c-cpp', 'csharp', 'go', 'java-kotlin', 'javascript-typescript', 'python', 'ruby', 'swift' - # Use `c-cpp` to analyze code written in C, C++ or both - # Use 'java-kotlin' to analyze code written in Java, Kotlin or both - # Use 'javascript-typescript' to analyze code written in JavaScript, TypeScript or both - # To learn more about changing the languages that are analyzed or customizing the build mode for your analysis, - # see https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/customizing-your-advanced-setup-for-code-scanning. - # If you are analyzing a compiled language, you can modify the 'build-mode' for that language to customize how - # your codebase is analyzed, see https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/codeql-code-scanning-for-compiled-languages - steps: - - name: Checkout repository - uses: actions/checkout@v4 - with: - # Fetch just the latest commit so that we can determine which files were changed - fetch-depth: 0 - path: web - - # Initializes the CodeQL tools for scanning. - - name: Initialize CodeQL - uses: github/codeql-action/init@v3 - with: - languages: ${{ matrix.language }} - build-mode: ${{ matrix.build-mode }} - # If you wish to specify custom queries, you can do so here or in a config file. - # By default, queries listed here will override any specified in a config file. - # Prefix the list here with "+" to use these queries and those in the config file. - - # For more details on CodeQL's query packs, refer to: https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning#using-queries-in-ql-packs - # queries: security-extended,security-and-quality - - # If the analyze step fails for one of the languages you are analyzing with - # "We were unable to automatically build your code", modify the matrix above - # to set the build mode to "manual" for that language. Then modify this step - # to build your code. - # ℹī¸ Command-line programs to run using the OS shell. - # 📚 See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsrun - - if: matrix.build-mode == 'manual' - shell: bash - run: | - echo 'If you are using a "manual" build mode for one or more of the' \ - 'languages you are analyzing, replace this with the commands to build' \ - 'your code, for example:' - echo ' make bootstrap' - echo ' make release' - exit 1 - - - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v3 - with: - category: "/language:${{matrix.language}}" \ No newline at end of file diff --git a/.gitignore b/.gitignore index 0df290ad..7178184e 100644 --- a/.gitignore +++ b/.gitignore @@ -14,6 +14,7 @@ vendor/ .vscode/ tests/cover tests/coverage.xml +tests/coverage tests/coding_standard.xml tests/junit.xml public diff --git a/app/Http/Admin/Request/PassportLoginRequest.php b/app/Http/Admin/Request/PassportLoginRequest.php index 461756c6..9a69872e 100644 --- a/app/Http/Admin/Request/PassportLoginRequest.php +++ b/app/Http/Admin/Request/PassportLoginRequest.php @@ -12,6 +12,7 @@ namespace App\Http\Admin\Request; +use App\Http\Common\Request\Traits\NoAuthorizeTrait; use Hyperf\Collection\Arr; use Hyperf\Swagger\Annotation\Property; use Hyperf\Swagger\Annotation\Schema; @@ -27,11 +28,7 @@ class PassportLoginRequest extends FormRequest { use ClientIpRequestTrait; use ClientOsTrait; - - public function authorize(): bool - { - return true; - } + use NoAuthorizeTrait; public function rules(): array { diff --git a/app/Http/Admin/Request/Permission/BatchGrantPermissionsForRoleRequest.php b/app/Http/Admin/Request/Permission/BatchGrantPermissionsForRoleRequest.php index 4905f7f4..7b479f95 100644 --- a/app/Http/Admin/Request/Permission/BatchGrantPermissionsForRoleRequest.php +++ b/app/Http/Admin/Request/Permission/BatchGrantPermissionsForRoleRequest.php @@ -12,6 +12,7 @@ namespace App\Http\Admin\Request\Permission; +use App\Http\Common\Request\Traits\NoAuthorizeTrait; use Hyperf\Swagger\Annotation\Property; use Hyperf\Swagger\Annotation\Schema; use Hyperf\Validation\Request\FormRequest; @@ -24,10 +25,7 @@ )] class BatchGrantPermissionsForRoleRequest extends FormRequest { - public function authorize(): bool - { - return true; - } + use NoAuthorizeTrait; public function rules(): array { diff --git a/app/Http/Admin/Request/Permission/BatchGrantRolesForUserRequest.php b/app/Http/Admin/Request/Permission/BatchGrantRolesForUserRequest.php index e9602f0b..de009c1b 100644 --- a/app/Http/Admin/Request/Permission/BatchGrantRolesForUserRequest.php +++ b/app/Http/Admin/Request/Permission/BatchGrantRolesForUserRequest.php @@ -12,6 +12,7 @@ namespace App\Http\Admin\Request\Permission; +use App\Http\Common\Request\Traits\NoAuthorizeTrait; use Hyperf\Swagger\Annotation\Property; use Hyperf\Swagger\Annotation\Schema; use Hyperf\Validation\Request\FormRequest; @@ -24,10 +25,7 @@ )] class BatchGrantRolesForUserRequest extends FormRequest { - public function authorize(): bool - { - return true; - } + use NoAuthorizeTrait; public function rules(): array { diff --git a/app/Http/Admin/Request/Permission/MenuRequest.php b/app/Http/Admin/Request/Permission/MenuRequest.php index b6c3478f..3fa2e162 100644 --- a/app/Http/Admin/Request/Permission/MenuRequest.php +++ b/app/Http/Admin/Request/Permission/MenuRequest.php @@ -12,6 +12,7 @@ namespace App\Http\Admin\Request\Permission; +use App\Http\Common\Request\Traits\NoAuthorizeTrait; use App\Schema\MenuSchema; use Hyperf\Validation\Request\FormRequest; @@ -24,10 +25,7 @@ )] class MenuRequest extends FormRequest { - public function authorize() - { - return true; - } + use NoAuthorizeTrait; public function rules(): array { diff --git a/app/Http/Admin/Request/Permission/PermissionRequest.php b/app/Http/Admin/Request/Permission/PermissionRequest.php index f76cc775..767e8497 100644 --- a/app/Http/Admin/Request/Permission/PermissionRequest.php +++ b/app/Http/Admin/Request/Permission/PermissionRequest.php @@ -12,6 +12,7 @@ namespace App\Http\Admin\Request\Permission; +use App\Http\Common\Request\Traits\NoAuthorizeTrait; use App\Schema\UserSchema; use Hyperf\Validation\Request\FormRequest; @@ -23,10 +24,7 @@ )] class PermissionRequest extends FormRequest { - public function authorize(): bool - { - return true; - } + use NoAuthorizeTrait; public function rules(): array { diff --git a/app/Http/Admin/Request/Permission/RoleRequest.php b/app/Http/Admin/Request/Permission/RoleRequest.php index 0b44a624..ab9d96fa 100644 --- a/app/Http/Admin/Request/Permission/RoleRequest.php +++ b/app/Http/Admin/Request/Permission/RoleRequest.php @@ -12,6 +12,8 @@ namespace App\Http\Admin\Request\Permission; +use App\Http\Common\Request\Traits\HttpMethodTrait; +use App\Http\Common\Request\Traits\NoAuthorizeTrait; use App\Schema\RoleSchema; use Hyperf\Validation\Request\FormRequest; @@ -23,20 +25,30 @@ )] class RoleRequest extends FormRequest { - public function authorize(): bool - { - return true; - } + use HttpMethodTrait; + use NoAuthorizeTrait; public function rules(): array { - return [ + $rules = [ 'name' => 'required|string|max:60', - 'code' => 'required|string|max:60', + 'code' => [ + 'required', + 'string', + 'max:60', + 'regex:/^[a-zA-Z0-9_]+$/', + ], 'status' => 'sometimes|integer|in:1,2', 'sort' => 'required|integer', 'remark' => 'nullable|string|max:255', ]; + if ($this->isCreate()) { + $rules['code'][] = 'unique:role,code'; + } + if ($this->isUpdate()) { + $rules['code'][] = 'unique:role,code,' . $this->route('id'); + } + return $rules; } public function attributes(): array diff --git a/app/Http/Admin/Request/Permission/UserRequest.php b/app/Http/Admin/Request/Permission/UserRequest.php index 8fa8139a..33c1af07 100644 --- a/app/Http/Admin/Request/Permission/UserRequest.php +++ b/app/Http/Admin/Request/Permission/UserRequest.php @@ -12,6 +12,7 @@ namespace App\Http\Admin\Request\Permission; +use App\Http\Common\Request\Traits\NoAuthorizeTrait; use App\Schema\UserSchema; use Hyperf\Validation\Request\FormRequest; use Mine\Swagger\Attributes\FormRequest as FormRequestAnnotation; @@ -46,10 +47,7 @@ )] class UserRequest extends FormRequest { - public function authorize(): bool - { - return true; - } + use NoAuthorizeTrait; public function rules(): array { @@ -58,8 +56,8 @@ public function rules(): array 'user_type' => 'required|integer', 'nickname' => ['required', 'string', 'max:60', 'regex:/^[^\s]+$/'], 'phone' => 'sometimes|string|max:12', - 'email' => 'sometimes|string|max:60', - 'avatar' => 'sometimes|string|max:255', + 'email' => 'sometimes|string|max:60|email:rfc,dns', + 'avatar' => 'sometimes|string|max:255|url', 'signed' => 'sometimes|string|max:255', 'status' => 'sometimes|integer', 'backend_setting' => 'sometimes|array|max:255', diff --git a/app/Http/Admin/Request/UploadRequest.php b/app/Http/Admin/Request/UploadRequest.php index 53084982..8782f483 100644 --- a/app/Http/Admin/Request/UploadRequest.php +++ b/app/Http/Admin/Request/UploadRequest.php @@ -12,6 +12,7 @@ namespace App\Http\Admin\Request; +use App\Http\Common\Request\Traits\NoAuthorizeTrait; use Hyperf\Swagger\Annotation\Property; use Hyperf\Swagger\Annotation\Schema; use Hyperf\Validation\Request\FormRequest; @@ -24,10 +25,7 @@ )] class UploadRequest extends FormRequest { - public function authorize(): bool - { - return true; - } + use NoAuthorizeTrait; public function rules(): array { diff --git a/app/Http/Api/Request/V1/UserRequest.php b/app/Http/Api/Request/V1/UserRequest.php index 934d6af1..7a6d51c4 100644 --- a/app/Http/Api/Request/V1/UserRequest.php +++ b/app/Http/Api/Request/V1/UserRequest.php @@ -12,6 +12,7 @@ namespace App\Http\Api\Request\V1; +use App\Http\Common\Request\Traits\NoAuthorizeTrait; use App\Schema\UserSchema; use Hyperf\Validation\Request\FormRequest; @@ -23,10 +24,7 @@ )] class UserRequest extends FormRequest { - public function authorize(): bool - { - return true; - } + use NoAuthorizeTrait; public function rules(): array { diff --git a/app/Http/Common/Request/Traits/HttpMethodTrait.php b/app/Http/Common/Request/Traits/HttpMethodTrait.php new file mode 100644 index 00000000..ffea2f61 --- /dev/null +++ b/app/Http/Common/Request/Traits/HttpMethodTrait.php @@ -0,0 +1,41 @@ +isMethod('POST'); + } + + public function isUpdate(): bool + { + return $this->isMethod('PUT') || $this->isMethod('PATCH'); + } + + public function isDelete(): bool + { + return $this->isMethod('DELETE'); + } + + public function isSearch(): bool + { + return $this->isMethod('GET'); + } +} diff --git a/app/Http/Common/Request/Traits/NoAuthorizeTrait.php b/app/Http/Common/Request/Traits/NoAuthorizeTrait.php new file mode 100644 index 00000000..d462908c --- /dev/null +++ b/app/Http/Common/Request/Traits/NoAuthorizeTrait.php @@ -0,0 +1,21 @@ +orderBy('sort') ->with('children'); } + + public function deleting(Deleting $event) + { + $this->roles()->detach(); + } } diff --git a/app/Model/Permission/Role.php b/app/Model/Permission/Role.php index d4a76b09..8a9762d3 100644 --- a/app/Model/Permission/Role.php +++ b/app/Model/Permission/Role.php @@ -15,6 +15,7 @@ use App\Model\Enums\User\Status; use Carbon\Carbon; use Hyperf\Database\Model\Collection; +use Hyperf\Database\Model\Events\Deleting; use Hyperf\Database\Model\Relations\BelongsToMany; use Hyperf\DbConnection\Model\Model as MineModel; @@ -83,4 +84,10 @@ public function users(): BelongsToMany 'user_id' ); } + + public function deleting(Deleting $event) + { + $this->users()->detach(); + $this->menus()->detach(); + } } diff --git a/app/Model/Permission/User.php b/app/Model/Permission/User.php index f4eaa927..4dfce979 100644 --- a/app/Model/Permission/User.php +++ b/app/Model/Permission/User.php @@ -120,7 +120,6 @@ public function getRoles(array $fields): Collection { return $this->roles() ->where('status', Status::Normal) -// ->select(['id', ...$fields]) ->select($fields) ->get(); } diff --git a/tests/Feature/Admin/GetTokenTrait.php b/tests/Feature/Admin/GetTokenTrait.php index f3e8bb55..63909f3e 100644 --- a/tests/Feature/Admin/GetTokenTrait.php +++ b/tests/Feature/Admin/GetTokenTrait.php @@ -37,6 +37,9 @@ public function getToken(User $user): string 'username' => $user->username, 'password' => '123456', ]); + if (!is_array($result)){ + Assert::fail('Get token failed.'); + } if (! Arr::has($result, 'data.access_token')) { Assert::fail('Get token failed.'); } diff --git a/tests/Feature/Admin/Permission/RoleControllerTest.php b/tests/Feature/Admin/Permission/RoleControllerTest.php index 32fbaca2..7fcf514e 100644 --- a/tests/Feature/Admin/Permission/RoleControllerTest.php +++ b/tests/Feature/Admin/Permission/RoleControllerTest.php @@ -68,13 +68,18 @@ public function testCreate(): void self::assertSame($result['code'], ResultCode::SUCCESS->value); $this->deletePermissions('permission:role:save'); $result = $this->post('/admin/role', $fill, ['Authorization' => 'Bearer ' . $token]); + self::assertSame($result['code'], ResultCode::UNPROCESSABLE_ENTITY->value); + $oldCode = $fill['code']; + $fill['code'] = Str::random(10); + $result = $this->post('/admin/role', $fill, ['Authorization' => 'Bearer ' . $token]); self::assertSame($result['code'], ResultCode::FORBIDDEN->value); - $entity = Role::query()->where('code', $fill['code'])->first(); + $entity = Role::query()->where('code', $oldCode)->first(); self::assertNotNull($entity); self::assertSame($entity->name, $fill['name']); self::assertSame($entity->sort, $fill['sort']); self::assertSame($entity->status->value, $fill['status']); self::assertSame($entity->remark, $fill['remark']); + self::assertSame($entity->code, $oldCode); $entity->forceDelete(); } diff --git a/tests/HttpTestCase.php b/tests/HttpTestCase.php index d1995c2d..169bac0b 100644 --- a/tests/HttpTestCase.php +++ b/tests/HttpTestCase.php @@ -39,7 +39,7 @@ abstract class HttpTestCase extends TestCase public function __construct($name = null, array $data = [], $dataName = '') { - parent::__construct($name, $data, $dataName); + parent::__construct($name); $this->client = make(Client::class); }