fix: GeoIP Interceptor + Test Cases #137
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes introduce a new GitHub Actions workflow for CodeQL analysis and update the README for enhanced clarity. They include modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant GeoIPInterceptor
participant GeoService
User ->> App: Send Request
App ->> GeoIPInterceptor: Intercept Request
GeoIPInterceptor ->> GeoService: Get Location by IP
GeoService -->> GeoIPInterceptor: Location Data
GeoIPInterceptor -->> App: Allow/Deny Request
App -->> User: Response
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
packages/common/src/interceptors/file-upload.interceptor.ts (1)
Line range hint
34-50:
Improve error handling by using proper logging.The error should be logged properly instead of using
console.log.- console.log(error); + // Use proper logging instead of console.log + this.logger.error('Error occurred during file upload', error);README.md (3)
9-20: Getting started section update approved.The updated instructions are clearer and more user-friendly.
Minor grammatical improvements.
Consider the following changes for better readability and correctness:
- Bootstrap a new project using: + Bootstrap a new project using: - During the setup the CLI will prompt you to select all the toolings you want by letting you pick and choose between what you want to have in your service. + During the setup, the CLI will prompt you to select the toolings you want by letting you choose what you want to have in your service.Tools
LanguageTool
[uncategorized] ~20-~20: A comma might be missing here.
Context: ...ncil new <PROJECT_NAME> ``` During the setup the CLI will prompt you to select all t...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~20-~20: ‘pick and choose’ might be wordy. Consider a shorter alternative.
Context: ...ll the toolings you want by letting you pick and choose between what you want to have in your s...(EN_WORDINESS_PREMIUM_PICK_AND_CHOOSE)
Line range hint
27-39:
Local setup section update approved.The updated instructions are clearer and more user-friendly.
Minor grammatical improvements.
Consider the following changes for better readability and correctness:
- Navigate to the required directory where you want to hack around and refer directory level READMEs to understand more about the code stored there. + Navigate to the required directory where you want to hack around and refer to directory-level READMEs to understand more about the code stored there.Tools
LanguageTool
[style] ~38-~38: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ate to the required directory where you want to hack around and refer directory level R...(REP_WANT_TO_VB)
[uncategorized] ~38-~38: Possible missing preposition found.
Context: ...where you want to hack around and refer directory level READMEs to understand more about ...(AI_EN_LECTOR_MISSING_PREPOSITION)
[grammar] ~44-~44: Make sure that the noun ‘setup’ is correct. Did you mean the past participle “set up”?
Context: ... have request for a specific tool to be setup automatically, please open a issue tick...(BE_VB_OR_NN)
[misspelling] ~44-~44: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... to be setup automatically, please open a issue ticket and we'll try to get it ad...(EN_A_VS_AN)
[uncategorized] ~44-~44: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...utomatically, please open a issue ticket and we'll try to get it added at the earlie...(COMMA_COMPOUND_SENTENCE)
[misspelling] ~54-~54: This word is normally spelled as one.
Context: ...) - Grafana - Elastic Search - [Logstash](...(EN_COMPOUNDS_ELASTIC_SEARCH)
46-57: Acknowledgements section update approved.The acknowledgements provide proper credit to the technologies used.
Minor spelling correction.
Consider the following change for correctness:
- [Elastic Search](https://www.elastic.co/) + [Elasticsearch](https://www.elastic.co/)Tools
LanguageTool
[misspelling] ~54-~54: This word is normally spelled as one.
Context: ...) - Grafana - Elastic Search - [Logstash](...(EN_COMPOUNDS_ELASTIC_SEARCH)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
packages/user-service/yarn.lockis excluded by!**/yarn.lock,!**/*.locksample/06-file-upload/yarn.lockis excluded by!**/yarn.lock,!**/*.locksample/07-geopip-blocking/package-lock.jsonis excluded by!**/package-lock.jsonsample/07-geopip-blocking/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (24)
- .github/workflows/codeql.yml (1 hunks)
- README.md (2 hunks)
- packages/common/src/interceptors/file-upload.interceptor.ts (1 hunks)
- packages/common/src/interceptors/geoip.interceptor.ts (1 hunks)
- packages/common/src/interceptors/index.ts (1 hunks)
- packages/common/test/file-upload.interceptor.spec.ts (1 hunks)
- sample/06-file-upload/nest-cli.json (1 hunks)
- sample/06-file-upload/package.json (3 hunks)
- sample/07-geopip-blocking/.eslintrc.js (1 hunks)
- sample/07-geopip-blocking/.example.env (1 hunks)
- sample/07-geopip-blocking/.gitignore (1 hunks)
- sample/07-geopip-blocking/.prettierrc (1 hunks)
- sample/07-geopip-blocking/README.md (1 hunks)
- sample/07-geopip-blocking/nest-cli.json (1 hunks)
- sample/07-geopip-blocking/package.json (1 hunks)
- sample/07-geopip-blocking/src/app.controller.spec.ts (1 hunks)
- sample/07-geopip-blocking/src/app.controller.ts (1 hunks)
- sample/07-geopip-blocking/src/app.module.ts (1 hunks)
- sample/07-geopip-blocking/src/app.service.ts (1 hunks)
- sample/07-geopip-blocking/src/main.ts (1 hunks)
- sample/07-geopip-blocking/test/app.e2e-spec.ts (1 hunks)
- sample/07-geopip-blocking/test/jest-e2e.json (1 hunks)
- sample/07-geopip-blocking/tsconfig.build.json (1 hunks)
- sample/07-geopip-blocking/tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (13)
- packages/common/src/interceptors/index.ts
- sample/06-file-upload/nest-cli.json
- sample/07-geopip-blocking/.example.env
- sample/07-geopip-blocking/.gitignore
- sample/07-geopip-blocking/.prettierrc
- sample/07-geopip-blocking/nest-cli.json
- sample/07-geopip-blocking/src/app.controller.spec.ts
- sample/07-geopip-blocking/src/app.module.ts
- sample/07-geopip-blocking/src/app.service.ts
- sample/07-geopip-blocking/src/main.ts
- sample/07-geopip-blocking/test/jest-e2e.json
- sample/07-geopip-blocking/tsconfig.build.json
- sample/07-geopip-blocking/tsconfig.json
Additional context used
LanguageTool
README.md
[uncategorized] ~20-~20: A comma might be missing here.
Context: ...ncil new <PROJECT_NAME> ``` During the setup the CLI will prompt you to select all t...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[style] ~20-~20: ‘pick and choose’ might be wordy. Consider a shorter alternative.
Context: ...ll the toolings you want by letting you pick and choose between what you want to have in your s...(EN_WORDINESS_PREMIUM_PICK_AND_CHOOSE)
[style] ~38-~38: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ate to the required directory where you want to hack around and refer directory level R...(REP_WANT_TO_VB)
[uncategorized] ~38-~38: Possible missing preposition found.
Context: ...where you want to hack around and refer directory level READMEs to understand more about ...(AI_EN_LECTOR_MISSING_PREPOSITION)
[grammar] ~44-~44: Make sure that the noun ‘setup’ is correct. Did you mean the past participle “set up”?
Context: ... have request for a specific tool to be setup automatically, please open a issue tick...(BE_VB_OR_NN)
[misspelling] ~44-~44: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... to be setup automatically, please open a issue ticket and we'll try to get it ad...(EN_A_VS_AN)
[uncategorized] ~44-~44: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...utomatically, please open a issue ticket and we'll try to get it added at the earlie...(COMMA_COMPOUND_SENTENCE)
[misspelling] ~54-~54: This word is normally spelled as one.
Context: ...) - Grafana - Elastic Search - [Logstash](...(EN_COMPOUNDS_ELASTIC_SEARCH)
sample/07-geopip-blocking/README.md
[uncategorized] ~63-~63: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...`` ## Support Nest is an MIT-licensed open source project. It can grow thanks to the spon...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~63-~63: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...anks to the sponsors and support by the amazing backers. If you'd like to join them, pl...(AWESOME)
Markdownlint
sample/07-geopip-blocking/README.md
32-32: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
18-18: null
Images should have alternate text (alt text)(MD045, no-alt-text)
20-20: null
Images should have alternate text (alt text)(MD045, no-alt-text)
5-5: Unused link or image reference definition: "circleci-image"
Link and image reference definitions should be needed(MD053, link-image-reference-definitions)
6-6: Unused link or image reference definition: "circleci-url"
Link and image reference definitions should be needed(MD053, link-image-reference-definitions)
Gitleaks
sample/07-geopip-blocking/README.md
5-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (39)
sample/07-geopip-blocking/src/app.controller.ts (3)
8-11: LGTM!The method implementation is correct and straightforward.
13-16: LGTM!The method implementation is correct and straightforward.
18-21: LGTM!The method implementation is correct and straightforward.
sample/07-geopip-blocking/.eslintrc.js (6)
2-2: LGTM!The parser setting is appropriate for a TypeScript project.
3-7: LGTM!The parserOptions settings are appropriate for a TypeScript project.
8-8: LGTM!The plugins setting is appropriate for a TypeScript project.
9-12: LGTM!The extends settings are appropriate for a TypeScript project.
14-17: LGTM!The env settings are appropriate for a Node.js and Jest project.
19-24: LGTM!The rules settings are appropriate for the project.
.github/workflows/codeql.yml (4)
3-7: LGTM!The
onconfiguration is appropriate for the intended functionality.
9-12: LGTM!The
jobsconfiguration is appropriate for the intended functionality.
13-16: LGTM!The
permissionsconfiguration is appropriate for the intended functionality.
18-59: LGTM!The
stepsconfiguration is appropriate for the intended functionality.README.md (2)
3-4: Framework description update approved.The updated description emphasizes the focus on developer experience and productivity, which is a positive change.
40-44: Documentation section update approved.The addition of a link to technical documentation improves accessibility to information.
Tools
LanguageTool
[grammar] ~44-~44: Make sure that the noun ‘setup’ is correct. Did you mean the past participle “set up”?
Context: ... have request for a specific tool to be setup automatically, please open a issue tick...(BE_VB_OR_NN)
[misspelling] ~44-~44: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... to be setup automatically, please open a issue ticket and we'll try to get it ad...(EN_A_VS_AN)
[uncategorized] ~44-~44: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...utomatically, please open a issue ticket and we'll try to get it added at the earlie...(COMMA_COMPOUND_SENTENCE)
sample/06-file-upload/package.json (2)
27-27: New dependencies added.The new dependencies
@samagra-x/stenciland@samagra-x/schematicsare likely required for new functionality.Verify dependency versions.
Ensure that the versions of the new dependencies are correct and compatible with the rest of the project.
Also applies to: 38-39
Verification successful
Verify dependency versions and consistency.
The new dependencies
@samagra-x/stenciland@samagra-x/schematicshave been added with versions^0.0.6and^0.0.5respectively. Ensure these versions are consistent across the project and there are no known compatibility issues.
@samagra-x/stencilversion^0.0.6is consistent with other instances in the project.@samagra-x/schematicsversion^0.0.5is consistent with other instances in the project.No immediate compatibility issues were identified.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the versions of new dependencies added. # Test: Search for the versions of the new dependencies. Expect: Versions should be correct and compatible. rg --type json '@samagra-x/stencil' rg --type json '@samagra-x/schematics'Length of output: 5364
38-39: New devDependencies added.The new devDependencies
@samagra-x/schematicsand@samagra-x/stencil-cliare likely required for new functionality.Verify devDependency versions.
Ensure that the versions of the new devDependencies are correct and compatible with the rest of the project.
Verification successful
Verified devDependency versions.
The versions of the new devDependencies
@samagra-x/schematicsand@samagra-x/stencil-cliare consistently set to "^0.0.5" across the project files. This indicates that the versions are correct and compatible with the rest of the project.
- sample/06-file-upload/package.json
- sample/07-geopip-blocking/package.json
- sample/04-logging/package.json
- sample/05-temporal-package/package.json
- sample/02-monitoring/package.json
- sample/03-response-formatting/package.json
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the versions of new devDependencies added. # Test: Search for the versions of the new devDependencies. Expect: Versions should be correct and compatible. rg --type json '@samagra-x/schematics' rg --type json '@samagra-x/stencil-cli'Length of output: 4501
sample/07-geopip-blocking/package.json (2)
23-31: New dependencies added.The new dependencies for the
07-geopip-blockingproject are likely required for new functionality.Verify dependency versions.
Ensure that the versions of the new dependencies are correct and compatible with the rest of the project.
34-56: New devDependencies added.The new devDependencies for the
07-geopip-blockingproject are likely required for new functionality.Verify devDependency versions.
Ensure that the versions of the new devDependencies are correct and compatible with the rest of the project.
Verification successful
Verify remaining devDependencies.
Ensure that the versions of the remaining new devDependencies are correct and compatible with the rest of the project.
New devDependencies verified.
The versions of the new devDependencies added in
sample/07-geopip-blocking/package.jsonare correct and compatible with the rest of the project.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the versions of new devDependencies added. # Test: Search for the versions of the new devDependencies. Expect: Versions should be correct and compatible. rg --type json '@nestjs/cli' rg --type json '@nestjs/schematics'Length of output: 7225
Script:
#!/bin/bash # Description: Verify the versions of remaining new devDependencies added. # Test: Search for the versions of the remaining new devDependencies. Expect: Versions should be correct and compatible. rg --type json '@samagra-x/schematics' rg --type json '@samagra-x/stencil-cli' rg --type json '@types/express' rg --type json '@types/jest' rg --type json '@types/node' rg --type json '@types/supertest' rg --type json '@typescript-eslint/eslint-plugin' rg --type json '@typescript-eslint/parser' rg --type json 'eslint' rg --type json 'eslint-config-prettier' rg --type json 'eslint-plugin-prettier' rg --type json 'jest' rg --type json 'prettier' rg --type json 'source-map-support' rg --type json 'supertest' rg --type json 'ts-jest' rg --type json 'ts-loader' rg --type json 'ts-node' rg --type json 'tsconfig-paths' rg --type json 'typescript'Length of output: 353026
packages/common/test/file-upload.interceptor.spec.ts (6)
1-11: Test setup approved.The test setup appropriately imports the necessary modules and initializes the interceptor.
13-15: 'Should be defined' test case approved.The test case ensures that the interceptor is correctly initialized.
17-26: 'Should handle file upload' test case approved.The test case ensures that the interceptor correctly handles file uploads.
30-44: 'Should handle errors' test case approved.The test case ensures that the interceptor correctly handles errors during file uploads.
47-59: 'Should handle getting an uploaded file when file is not present or null' test case approved.The test case ensures that the interceptor correctly handles cases where the file is not present or null.
63-73: Helper functions approved.The helper functions create mock contexts and next handlers effectively for the test cases.
packages/common/src/interceptors/geoip.interceptor.ts (4)
1-13: LGTM! Imports are appropriate.The imports are necessary for logging, error handling, and making HTTP requests.
16-31: LGTM! Enhanced logging and flexibility.The new properties and constructor changes improve logging and allow customizable access control.
33-72: LGTM! Improved logging and geolocation checks.The
interceptmethod enhancements improve debugging and ensure requests are allowed based on the country.
74-87: LGTM! Appropriate use of HttpService and error handling.The
getLocationmethod correctly usesHttpServiceto get the country from the IP address and handles errors appropriately.sample/07-geopip-blocking/README.md (4)
1-21: LGTM! Useful badges and links.The badges and links provide useful information about the project status and support options.
Tools
Markdownlint
18-18: null
Images should have alternate text (alt text)(MD045, no-alt-text)
20-20: null
Images should have alternate text (alt text)(MD045, no-alt-text)
5-5: Unused link or image reference definition: "circleci-image"
Link and image reference definitions should be needed(MD053, link-image-reference-definitions)
6-6: Unused link or image reference definition: "circleci-url"
Link and image reference definitions should be needed(MD053, link-image-reference-definitions)
Gitleaks
5-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
25-33: LGTM! Clear description and installation instructions.The description provides a clear overview of the project, and the installation instructions are easy to follow.
Tools
Markdownlint
32-32: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
35-59: LGTM! Clear running and testing instructions.The instructions provide clear commands for running the app in different modes and running tests.
61-73: LGTM! Useful support, contact, and license information.The support, contact, and license information provide useful details about the project.
Tools
LanguageTool
[uncategorized] ~63-~63: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...`` ## Support Nest is an MIT-licensed open source project. It can grow thanks to the spon...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~63-~63: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...anks to the sponsors and support by the amazing backers. If you'd like to join them, pl...(AWESOME)
sample/07-geopip-blocking/test/app.e2e-spec.ts (6)
1-7: LGTM! Appropriate imports and describe block.The imports are necessary for testing, and the describe block sets up the test suite for
GeoIpInterceptor.
8-33: LGTM! Correctly mocks GeoIpInterceptor.The beforeEach block sets up a mocked
GeoIpInterceptorfor testing purposes.
35-59: LGTM! Correctly tests allowed requests from India.The test cases verify that the interceptor allows requests from India for both IPv4 and IPv6 addresses.
61-124: LGTM! Correctly tests requests from outside India.The test cases verify that the interceptor allows requests from outside India for both IPv4 and IPv6 addresses.
126-141: LGTM! Correctly tests blocking IPv6 addresses outside India.The test case verifies that the interceptor blocks IPv6 addresses from outside India.
143-153: LGTM! Correctly creates mock ExecutionContext.The helper function correctly creates a mock
ExecutionContextwith the provided IP address for testing.
…ide-add Adds a dev setup and dev workflow guide to CONTRIBUTIING.md
|
* Update nest-cli.json * Update package.json * Update package.json * Update package.json * Update package.json * Update package.json
…amagraX-Stencil#117) * Fix/npm-scripts * readme:add-cmd-stencil-cli --------- Co-authored-by: Yash Mittal <[email protected]>
techsavvyash
changed the title
Handled error of server crashing if upload location is invalid
…X-Stencil#107) * tests * response-time-test
…X-Stencil#107) * tests * response-time-test
packages/common/src/interceptors/test/response-time.interceptor.spec.ts
Outdated
Show resolved
Hide resolved
|
CI on this thing is failing. @Savio629 |
Fixed it ✔
So instead of passing just list of countries, we can pass key value pair of list of countries, coordinates, cities, geofences. |
The list of countries, coordinates, cities, geofences is workingmain.ts.-.New.folder.-.Visual.Studio.Code.2024-08-21.11-21-41.mp4The denied request should send a error message in the response right?Right now, it is just logging the denied request.
|
| countries: ['India', 'United States'], | ||
| cities: ['Mumbai', 'New York'], | ||
| coordinates: [{ lat: 35.6897, lon: 139.6895 }], // Tokyo | ||
| geofences: [{ lat: 51.5074, lon: -0.1278, radius: 50 }], // London, UK |
There was a problem hiding this comment.
could you document how do these params work together when more than one are defined?
will it work if the user sends in the location for Mumbai as a geofence? if we have already allowed for "India" and "United States" in countries then why do we even need to add cities? Will it allow for requests from tokyo and london because the coordinates and geofences allow for them? seems very confusing.
There was a problem hiding this comment.
will it work if the user sends in the location for Mumbai as a geofence?
Yes
if we have already allowed for "India" and "United States" in countries then why do we even need to add cities?
I had added mumbai and new york as an example, we could add cities of another countries to work with that specific city.
Will it allow for requests from tokyo and london because the coordinates and geofences allow for them?
Yes
Every paramater is independent with each other.
The request is allowed from the list of given countries, cities, coordinates and geofences.
There was a problem hiding this comment.
Do you think this independence is good? if I have allowed the request for country India and I have added coordinates for Mumbai in the config and then I send lat long from Delhi to access, will my request be allowed to pass through or will it be rejected?
There was a problem hiding this comment.
The thing is as India is a country so if we add any city which belongs to India it will allow the request.
All the params work as ||
I think I should have added a city which doesn't belong to the specified(allowed) country.
| expect(result.country).toEqual(geoData.country); | ||
| expect(result.regionName).toEqual(geoData.regionName); | ||
| }); | ||
|
|
There was a problem hiding this comment.
where are the test cases to check for other input ways? coordinates? geofences? failure cases when the user request is outside the jurisdiction of these countries and not just from localhost?
There was a problem hiding this comment.
Will update the pr with the above testcases
There was a problem hiding this comment.
Pushed e2e test
Finding it difficult to create integration test...
Checklist
Make sure you have
Description
Describe the aim/objective of this PR
Steps to test
Add steps to run and test teh desired changes
References
Add relevant links and screenshots outlining the working status of this PR
Updated pr : #134
Demo
If relevant to your change, attach a screen recording showcasing that the change is working and giving desired results.
Summary by CodeRabbit
New Features
GeoIPInterceptorto perform geolocation checks based on IP addresses.@samagra-x/stencildependency topackage.json.Bug Fixes
nest-cli.jsoncollection reference to@nestjs/schematics.Chores
sample/07-geopip-blocking..gitignoreand.example.envfor configurations and exclusions.Tests
FastifyFileInterceptorandAppController.GeoIpInterceptor.Documentation
Parent issue: #93