-
Notifications
You must be signed in to change notification settings - Fork 297
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
Development
: Add CORS allowed-origin-patterns
#9405
Conversation
WalkthroughThe changes in the pull request focus on updating the CORS filter configuration in the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
src/main/resources/config/application.yml
is excluded by!**/*.yml
📒 Files selected for processing (1)
- src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, we should maybe test if the App etc are still able to fetch all their data
yes that would be good. currently cors is disabled on production and testservers. so we need the cors adjustment on the test servers first |
Config
CORS allowed-origin-patternsGeneral
: CORS allowed-origin-patterns
General
: CORS allowed-origin-patternsGeneral
: Add CORS allowed-origin-patterns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code lgtm. How could I test this locally?
its hard to test. basically you would need a second website with a different URL that makes requests to the server. and then you would set the allowed-origin-patterns in the application.yml you could open up just any website and then in the inspect console do a fetch(localhost:8080/api/..., {method: POST}) and see if it throws an error. and if you then add the website in the application.yml as a pattern it should not throw an error anymore |
General
: Add CORS allowed-origin-patternsDevelopment
: Add CORS allowed-origin-patterns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java
Outdated
Show resolved
Hide resolved
9d01e84
Co-authored-by: Marcel Gaupp <m.gaupp@tum.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java (1)
Line range hint
121-126
: LGTM! Consider extracting the condition for improved readability.The changes effectively extend the CORS filter registration to include
allowedOriginPatterns
, which aligns with the PR objectives. The implementation adheres to the single responsibility principle and maintains simplicity. The use ofCollectionUtils.isEmpty()
is a good practice for null-safe empty checks.To further improve code readability and adhere to the "small_methods" principle mentioned in the coding guidelines, consider extracting the condition into a separate method:
@Bean public CorsFilter corsFilter() { UrlBasedCorsConfigurationSource source = new UrlBasedCorsConfigurationSource(); CorsConfiguration config = jHipsterProperties.getCors(); - if (!CollectionUtils.isEmpty(config.getAllowedOrigins()) || !CollectionUtils.isEmpty(config.getAllowedOriginPatterns())) { + if (shouldRegisterCorsFilter(config)) { log.debug("Registering CORS filter"); source.registerCorsConfiguration("/api/**", config); source.registerCorsConfiguration("/management/**", config); source.registerCorsConfiguration("/v3/api-docs", config); } return new CorsFilter(source); } +private boolean shouldRegisterCorsFilter(CorsConfiguration config) { + return !CollectionUtils.isEmpty(config.getAllowedOrigins()) || !CollectionUtils.isEmpty(config.getAllowedOriginPatterns()); +}This refactoring improves readability while maintaining the functionality and adhering to the coding guidelines.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java (1)
Pattern
src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java (1)
27-27
: LGTM! Appropriate import added for CollectionUtils.The addition of
import org.springframework.util.CollectionUtils;
is correct and necessary for using theCollectionUtils.isEmpty()
method in thecorsFilter()
method. This aligns with the coding guidelines to use appropriate utility classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM as long as the origins are authentic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
Checklist
General
Server
Description
Fixes CORS so that configured
allowed-origin-patterns
are also accepted.Furthermore adds the VSCode plugin origins to the CORS in
application.yml
Steps for Testing
only Config changes so code reviews are appreciated
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Test Coverage
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
@coderabbitai pause