Prof review #2
Prof review #2
Conversation
… and proxy connections
…irties context for before method instead of before class db reset
…for readability and file length
… workaround for checking timestamp)
# Conflicts: # .gitignore # README.md # docker/Grosse_AdvSoft_App/db-primary/Dockerfile # docker/Grosse_AdvSoft_App/db-replica/Dockerfile # docker/Grosse_AdvSoft_App/docker-compose.yml # src/main/resources/logback-spring.xml # src/main/resources/templates/component-templates/css-script-tmpl.html # src/main/resources/templates/component-templates/footer-tmpl.html # src/main/resources/templates/component-templates/header-tmpl.html # src/main/resources/templates/component-templates/nav-tmpl.html # src/main/resources/templates/component-templates/table.html # src/main/resources/templates/index.html # src/main/resources/templates/ip_info.html # src/main/resources/templates/page-content/index.html
| */ | ||
| override fun preHandle(req: HttpServletRequest, response: HttpServletResponse, handler: Any): Boolean { | ||
| log.debug("preHandle: PreHandle tracking initiated") | ||
| // ignore favicon requests |
| try { | ||
| log.debug("preHandle(): Attempting to acquire client request information") | ||
| clientInfo = userAgentParser.parse(req.getHeader("User-Agent")) | ||
| ipv4 = if (req.remoteAddr == "0:0:0:0:0:0:0:1") { |
There was a problem hiding this comment.
Consider making this a class constant, so the magic string address is not buried inside the code.
| } | ||
| uri = req.requestURI | ||
| } catch (e: Exception) { | ||
| log.error("preHandle(): failed to parse client information") |
There was a problem hiding this comment.
This should be:
log.error("preHandle(): failed to parse client information", e)
[ Include the exception as the last argument anytime you have an exception to give good error logs. ]
There was a problem hiding this comment.
Also, catching 'Exception' is considered bad practice. Instead, be more specific in the kind of exception you are expecting to catch (IOException, etc...).
| log.error("preHandle(): failed to parse client information") | ||
| return false | ||
| } | ||
| trackerService.trackClient( |
There was a problem hiding this comment.
As has been spoke about before, trackerService.trackClient should take a DTO object for a parameters instead of a bunch of random String variables, as it easy to switch the order up accidentally, and a DTO would have setters with descriptive names that make the code easier to follow.
| import org.springframework.web.servlet.config.annotation.WebMvcConfigurer | ||
|
|
||
| /** | ||
| * Configuration class which registers the application interceptor as a valid interceptor to Spring |
There was a problem hiding this comment.
Nit:
/**
* Configuration class which registers the this class as an request interceptor for Spring.
*/
No need to explain how Spring/Dependency Injections works in comments, as if the readers of the code haven't gotten that part figured out with the @component annotation, they are going to be lost long before they get to this class.
| Thread.sleep(100) | ||
| val timeU2f = Timestamp(Date().time) | ||
|
|
||
| val results = trackService.query() |
There was a problem hiding this comment.
One query, and then using the results. Good (unlike the previous tests in the class above).
|
|
||
| trackService.trackClient(ipv4, browser, browserMajorVersion, platform, platformVersion, requestURI) | ||
|
|
||
| Thread.sleep(100) |
| } | ||
|
|
||
| // test method disabled as there is currently no sanitization in place. this is a task for the later life | ||
| // of the project (pre-deployment but outside the scope of this semester project) |
|
|
||
| @Disabled | ||
| @Test | ||
| fun trackClientTestSQLInjectionUserAgent() { |
There was a problem hiding this comment.
No useful comments, not details, either delete or provide more context on what this test is supposed to do.
|
|
||
| val result: MvcResult = idxMvc.perform(post(uri)).andReturn() | ||
| assertTrue( | ||
| "invalidPostMethodToIndexTest(): should not be 200 when valid URI", |
There was a problem hiding this comment.
This comment is confusing. It's a valid URI, why should it fail?
[ Valid URI, invalid request type, should be in the comment. ]
Code review