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

Fix(RequestMethod): Any interface HTTP Request method verification #9

Merged
merged 17 commits into from Jan 13, 2021

Conversation

Hcreak
Copy link
Contributor

@Hcreak Hcreak commented Jan 12, 2021

对部分接口的HTTP请求方式进行校验 不正确的请求方式将被阻止并给出ERR_METHOD_INVALID错误信息

Hcreak and others added 7 commits January 7, 2021 18:47
* Fix(TokenExpireTime): Agent TokenExpireTime || Console TokenExpireTime (#7)

* fix(TokenExpireTime):agent token not console token
* Fix(Dockerfile):NodeJS Version
* Fix(TokenExpireTime):Update Docs

* Update axios to >=0.21.1:  GHSA-4w2v-q235-vp99

* Add interface parameter verification logic (application, user, role)

* Add interface parameter verification logic (category,permission,resource,user_role)

Co-authored-by: iGeeky <igeeky.io@gmail.com>
HTTP Request method verification
@iGeeky
Copy link
Owner

iGeeky commented Jan 12, 2021

@Hcreak I think using exceptions to implement such checks might make the code a bit cleaner. This requires the following steps:

  1. Define an exception under the errors package, for example: MethodInvalidError
  2. Define the method check function in basic-service. checkMethod(method), which throws an exception if it does not match.
  3. handle the exception in middlewares.error-cache.
  4. call this.checkMethod('GET') in the function that needs to check the method.

In addition, you need to submit the corresponding unit test at the same time. To ensure that the newly added code is covered by the unit tests.

Doing this, in terms of performance, may not be the best, but it will make the code in the business method more simple.Often a single line of code is all that is needed to complete the check.
It also makes it easier to write unit tests: the current approach requires a method check failure in each business function to cover the branch where the check failed. With exceptions, you usually only need to write one check failure case.

@Hcreak
Copy link
Contributor Author

Hcreak commented Jan 12, 2021

@iGeeky

  1. handle the exception in middlewares.error-cache.

I don't understand this place,
I maybe not find you have used this middleware in your code.

@iGeeky
Copy link
Owner

iGeeky commented Jan 12, 2021

@Hcreak
I'm sorry, I made a typo. It should be middlewares/error-catch.
The location to use it is: https://github.com/iGeeky/wolf/blob/master/server/app.js#L39

Copy link
Owner

@iGeeky iGeeky left a comment

Choose a reason for hiding this comment

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

When you modify the code, you need to execute the unit tests completely and make sure that all cases pass.
For a complete test, use the command: cd wolf/server && npm run test
To test a single module: cd wolf/server && mocha --exit test/20.user.test.js
In addition, you need to add test cases for the newly added code. Please refer to the code under wolf/server/test.

@@ -37,6 +38,14 @@ class BasicService extends Service {
return this.success({exist})
}

// for REST
async checkMethod(method) {
Copy link
Owner

Choose a reason for hiding this comment

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

You don't need async here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.It does not need to be asynchronous. Sorry I have not finished.

@iGeeky
Copy link
Owner

iGeeky commented Jan 13, 2021

@Hcreak After you change the response code to 400, many test cases will need to change the expected status code to 400. Make sure that all test cases pass.

@Hcreak
Copy link
Contributor Author

Hcreak commented Jan 13, 2021

@iGeeky All unit tests have passed :-)

@iGeeky iGeeky merged commit 91a6a3f into iGeeky:master Jan 13, 2021
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

2 participants