:glob ✱ Reporter
	Description • 9 years ago

the current system for logging bmo access relies primarily on the httpd access logs.  these logs are incapable of presenting the complete picture, and are difficult to analyse.

we need to create an internal log for interesting events.  initially we'll store this information within the database, however it's possbile that this information will be moved to a different system in the future.

log authenticated requests for:
- bug get (show_bug and api)
- attachment get (attachments.cgi and api)
- searching (buglist.cgi and api)
- api-key management

capture:
- timestamp
- ip-address
- user-agent
- user-id
- bug-id (bug-get and attachment-get requests only)
- attach-id (attachment-get requests only)
- request-url
- method
- action:
  - 'bug-get'
  - 'attachment-get'
  - 'search'
  - 'api-key-create'
  - 'api-key-revoke'
- server:
  - 'web',
  - 'xmlrpc'
  - 'jsonrpc'
  - 'rest'
  - 'bzapi'

a back of the envelope calculate indicates we'll create about one million rows per month.  this hasn't raised concerns from the DBAs.

the retention period of this data is to be decided, but it's likely to be in the order of six months.

the table itself needs to be innodb and should not have any foreign keys, and needs an integer primary key.

	:glob ✱ Reporter
	Comment 1 • 9 years ago

we might want to also log the bug's groups, captured at the time the user visited the bug.
that would make bug 1196627 much easier.

	:glob ✱ Reporter
	Comment 3 • 9 years ago

Comment on attachment 8671334 [details] [diff] [review]
1196626_1.patch

Review of attachment 8671334 [details] [diff] [review]:
-----------------------------------------------------------------

this looks good, but i'd like to see switching to the main-db moved into log_user_request before doing a full review.

::: Bugzilla.pm
@@ +607,5 @@
> +    if ($script_name =~ /rest\.cgi/) {
> +        $server = $script_name =~ /BzAPI/ ? "bzapi" : "rest";
> +    }
> +    elsif ($script_name =~ /xmlrpc\.cgi/) {
> +        $server = "xmlrpc"; 

remove trailing whitespace

@@ +618,5 @@
> +    foreach my $param (@params) {
> +        trick_taint($param) if defined $param;
> +    }
> +
> +    eval {

switching to the main_db should happen here .. the callers shouldn't have to do it.

::: show_bug.cgi
@@ +136,5 @@
> +if ($user->id) {
> +    Bugzilla->with_main_db(
> +        sub {
> +            foreach my $bug_id (@bugids) {
> +                Bugzilla->log_user_request($bug_id, undef, 'bug-get', 'web');

log_user_request only takes 3 args

	:glob ✱ Reporter
	Comment 5 • 9 years ago

Comment on attachment 8671465 [details] [diff] [review]
1196626_2.patch

Review of attachment 8671465 [details] [diff] [review]:
-----------------------------------------------------------------

during testing there's something i realised we should add that wasn't part of the initial design (sorry!).

can you please add an parameter so we can toggle this logging.  it should be disabled by default.  there's two reasons: there's no need to perform logging (and consume disk space) on development systems, and should we hit a problem in prod i'd like to be able to quickly turn this off (eg. if it pans out that we have an unexpectedly high volume of loggable requests that result in performance issues).

this r- is only because i didn't ask for the param before and need a new patch; the patch itself looks good.

	:glob ✱ Reporter
	Comment 7 • 9 years ago

Comment on attachment 8671990 [details] [diff] [review]
1196626_3.patch

Review of attachment 8671990 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

please commit just the schema changes, and attach a new patch with that addresses the review points for deployment following the schema update.

::: Bugzilla.pm
@@ +596,5 @@
>  
> +sub log_user_request {
> +    my ($class, $bug_id, $attach_id, $action) = @_;
> +
> +    if (Bugzilla->params->{log_user_requests}) {

nit: return early instead of putting the whole method within an if block.

::: Bugzilla/Config/Admin.pm
@@ +67,5 @@
> +
> +  {
> +    name => 'log_user_requests',
> +    type => 'b',
> +    default => 1,

this needs to default to 'off', not on.

::: template/en/default/admin/params/admin.html.tmpl
@@ +41,5 @@
>  
>    last_visit_keep_days => "This option controls how many days $terms.Bugzilla will " _
> +                          "remember when users visit specific ${terms.bugs}.",
> +
> +   log_user_requests => "This option controls logging of user requests in the user_request_log table"}

throw the word 'authenticated' in there somewhere please

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 8 • 9 years ago

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   d69cebd..d0acb64  master -> master

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 9 • 9 years ago

patch for commit during push.

	:glob ✱ Reporter
	Comment 10 • 9 years ago

Comment on attachment 8673388 [details] [diff] [review]
1196626_4.patch

Review of attachment 8673388 [details] [diff] [review]:
-----------------------------------------------------------------

carrying forward r+

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 11 • 9 years ago

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   b21167f..a0fcc8f  master -> master

(per discussion with dkl, this can be commited now as the schema change was pushed to prod this week)