Closed Bug 1091806 Opened 7 years ago Closed 6 years ago

Audit logging for review board

Categories

(MozReview Graveyard :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jeff, Assigned: dminor)

References

Details

Attachments

(1 file)

Implement application audit logging for use in debugging, workflow analysis, security, access control and fraud monitoring.

Base set of security events: 
Account creation
Account modification
Account disable/deletion
Account login attempt
Account login result (success/failure)

Application events: 
Review request created.
Review request published. 
Bug updated.
others?

Logs can be sent directly to a local heka instance, sent to a filesystem file, or sent directly to mozdef using something like https://github.com/gdestuynder/mozdef_lib

Audit entries should be self contained with as much info as possible (source IP, username, bug #, etc rather than sending bits of info requiring extra correlation).
Priority: -- → P2
We should 

(a) have more logs about general actions
(b) have a way for devs to get to them.
These should also issue alerts of some kind for errors.
Duplicate of this bug: 1100993
We have too many P1s, so I'm spreading out the priorities.  P3 -> P4, P2 -> P3, and some portion of P1s will become P2.
Priority: P2 → P3
Assignee: nobody → dminor
OS: Mac OS X → Unspecified
Hardware: x86 → Unspecified
For (b) in comment 1 I would suggest using papertraillapp.com. There is already a mozilla account set up.
Status: NEW → ASSIGNED
Comment on attachment 8712331 [details]
MozReview Request: mozreview: audit logging for mozreview (bug 1091806) r=gps

https://reviewboard.mozilla.org/r/32511/#review30189

::: pylib/mozreview/mozreview/batchreview/resources.py:98
(Diff revision 1)
> +            logging.info('Could not create batchreview, review request '

Should we include the user here, so we don't have to try to match it with the above log?

::: pylib/mozreview/mozreview/batchreview/resources.py:117
(Diff revision 1)
>              except ValueError:
> +                logging.warning('Could not create batchreview, invalid '
> +                                'diff comments')
> +
>                  return INVALID_FORM_DATA, {
>                      'fields': {
>                          'diff_comments': 'Not valid JSON.',
>                      },
>                  }
>  
>              if not isinstance(diff_comments, list):
> +                logging.warning('Could not create batchreview, invalid '
> +                                'diff comments')
>                  return INVALID_FORM_DATA, {
>                      'fields': {

Include user and review request id?

::: pylib/mozreview/mozreview/batchreview/resources.py:173
(Diff revision 1)
>          except KeyError:
> +            logging.warning('Could not create batchreview, invalid '
> +                            'diff comments')
>              return INVALID_FORM_DATA, {
>                  'fields': {
>                      'diff_comments': 'Diff comments were malformed',
>                  },
>              }
>  
>          except ObjectDoesNotExist:
> +            logging.warning('Could not create batchreview, invalid '
> +                            'filediff_id')
>              return INVALID_FORM_DATA, {
>                  'fields': {
>                      'diff_comments': 'Invalid filediff_id',
>                  },
>              }
>  
> +        logging.info('batchreview created: %s' % review.id)

Same here.

::: pylib/mozreview/mozreview/bugzilla/client.py:158
(Diff revision 1)
> +        logging.info('Checking if bug %d is confidential.' % bug_id)

Probably useful to report the result too.

::: pylib/mozreview/mozreview/bugzilla/client.py:178
(Diff revision 1)
> +        logging.info('Posting review request to bug %d.' % bug_id)

Include review request id?  Feel free to fix the argument name, too, which should be review_request_id, not review_id.

::: pylib/mozreview/mozreview/bugzilla/client.py:384
(Diff revision 1)
> +        logging.info('Obsoleting attachments on bug %d.' % bug_id)

Do we want to report which attachments end up being actually obsoleted?

::: pylib/mozreview/mozreview/ldap/__init__.py:47
(Diff revision 1)
> +    logging.info('querying ldap group association: %s in %s' % (

Nit: capital 'Q'.  Also include the result?

::: pylib/mozreview/mozreview/ldap/resources.py:141
(Diff revision 1)
> +        logging.info('request to update ldap association made by user: %s' % (

Capital 'r'.  And should we record the result?

::: pylib/mozreview/mozreview/resources/bugzilla_login.py:68
(Diff revision 1)
> +                logging.info('login attempt failed for username: %s' %

Capital 'L'.

::: pylib/mozreview/mozreview/resources/bugzilla_login.py:74
(Diff revision 1)
> +            logging.info('login attempt failed for username: %s - no api '

Capital 'L'.

::: pylib/mozreview/mozreview/resources/bugzilla_login.py:82
(Diff revision 1)
>          # error response should be interpretted by clients to direct

Can you fix "interpreted" in this comment?

::: pylib/mozreview/mozreview/resources/bugzilla_login.py:85
(Diff revision 1)
> +            logging.info('login attempt failed for username: %s - weblogin '

Capital 'L'.

::: pylib/mozreview/mozreview/resources/bugzilla_login.py:107
(Diff revision 1)
> +        logging.info('login attempt succeeded for username: %s' %

Capital 'L'.

::: pylib/rbbz/rbbz/extension.py:458
(Diff revision 1)
> +    logging.info('Publishing review for user: %s id: %s' % (

Probably handy to record the review request ID as well.

::: pylib/rbbz/rbbz/extension.py:506
(Diff revision 1)
>      if not is_review_request_pushed(review_request):

Log this?
Attachment #8712331 - Flags: review?(mcote)
https://reviewboard.mozilla.org/r/32511/#review30193

::: pylib/mozreview/mozreview/decorators.py:50
(Diff revision 1)
> -                logging.error('No MozReviewUserProfile for authenticated user')
> +                logging.error('No MozReviewUserProfile for authenticated user: %s',

Throughout this patch `logging` is being used. This will use the "root" logger, which isn't helpful for routing. You should be creating a logger tied to the module name so events can be routed appropriately. See batch_review_requests.py for an example of how this should be done.
Comment on attachment 8712331 [details]
MozReview Request: mozreview: audit logging for mozreview (bug 1091806) r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32511/diff/1-2/
Attachment #8712331 - Flags: review?(mcote)
Attachment #8712331 - Flags: review+
Comment on attachment 8712331 [details]
MozReview Request: mozreview: audit logging for mozreview (bug 1091806) r=gps

https://reviewboard.mozilla.org/r/32511/#review31085

Excellent. I love logs.

::: pylib/mozreview/mozreview/bugzilla/client.py:152
(Diff revision 2)
> +        logger.info('Posting comment on bug %d.' % bug_id)

Thank you for adding the log entries for Bugzilla interaction. This will help us get timing information for these HTTP requests, which I suspect are a source of slowdown for various operations.

::: pylib/mozreview/mozreview/resources/bugzilla_login.py:113
(Diff revision 2)
> +            logger.info('Login attempt failed for username: %s' %

Let's throw "Bugzilla" or "API key" somewhere in this message to help us disambiguate between the various auth systems.

::: pylib/mozreview/mozreview/resources/bugzilla_login.py:121
(Diff revision 2)
> +        logger.info('Login attempt succeeded for username: %s' %

Ditto.

::: pylib/rbbz/rbbz/auth.py:132
(Diff revision 2)
> +        logger.info('Login attempt (apikey) for user %s: ' % username)

I suppose the messages below could also gain "(apikey)" annotations.
Comment on attachment 8712331 [details]
MozReview Request: mozreview: audit logging for mozreview (bug 1091806) r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32511/diff/2-3/
Attachment #8712331 - Attachment description: MozReview Request: mozreview: audit logging for mozreview (bug 1091806) r?mcote → MozReview Request: mozreview: audit logging for mozreview (bug 1091806) r=gps
Attachment #8712331 - Flags: review?(mcote)
Landed in https://hg.mozilla.org/hgcustom/version-control-tools/rev/312fdb6a15c5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.