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

Audit Module Milestone 1 #129

Merged
merged 1 commit into from
Jan 14, 2016
Merged

Audit Module Milestone 1 #129

merged 1 commit into from
Jan 14, 2016

Conversation

@Gnafu
Copy link
Contributor

Gnafu commented Dec 11, 2015

I did a test deploy, here's my comments:

  • Use a prefix for the AUDITING_CONF variable, because it can exist another application that needs it: I suggest GEOSTORE_AUDITING_CONF
  • The information dumped with the "remoteUser" property is too detailed (for example groups are fully dumped along the username, too much information): I suggest to break username, role and groups into three different properties.

@nmco
Copy link
Collaborator Author

nmco commented Dec 11, 2015

Thank you for the comments, I will update the pull request.

@nmco
Copy link
Collaborator Author

nmco commented Dec 16, 2015

Updated.

@nmco nmco changed the title First version of Audit Audit Module Milestone 1 Dec 17, 2015
@tdipisa
Copy link
Member

tdipisa commented Jan 11, 2016

I tested locally on windows. Below you can find my feedbacks:

  • could be useful to add a startTime and endTime into the Request report
  • the footer xml never seems to be applied
  • more description in the WIKI page should be added for each configuration property
  • about the following point in the proposal have you any news?
    "allow fine grained control over which REST end points should produce auditing information"
  • There are some code conflict in the pull request that probably you can solve during the next commit.

@simboss
Copy link
Member

simboss commented Jan 11, 2016

I would leave point 4 aside for the time being.

@nmco
Copy link
Collaborator Author

nmco commented Jan 12, 2016

Update as requested:

  • add info: startTime, endTime, totalTime, responseContentType and responseLength
  • add JVM shutdown hook (with proper concurrency handling) to add the footer
  • add some missing license headers

I will open an enhancement report with the missing features.

@nmco nmco force-pushed the auditing branch 3 times, most recently from 08ce7fa to 7534a3f Compare January 12, 2016 23:35
@tdipisa
Copy link
Member

tdipisa commented Jan 13, 2016

Your tests sound good for me, in any case there are some test failures involving the file paths:

  • the configuration dir path from the auditing configuration (AuditingConfigurationTest.java:58)
  • the template dir path (AuditingOutputTest.java:56)

I've tested on Windows. Can you check on that?

@nmco nmco force-pushed the auditing branch 2 times, most recently from 3d0dc5b to a432280 Compare January 13, 2016 15:38
tdipisa pushed a commit that referenced this pull request Jan 14, 2016
Audit Module Milestone 1
@tdipisa tdipisa merged commit 6876fcf into geosolutions-it:master Jan 14, 2016
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.

4 participants