kallithea Changeset - abc1ada59076

Changeset - abc1ada59076

Parent rev.

Child rev.

[Not reviewed]

default

0 1 0

Søren Løvborg - 10 years ago 2016-04-19 18:03:30
sorenl@unity3d.com

notifications: untangle notification access check

This removes a broken permission check when viewing notifications (the
HasRepoPermissionAny object was created, but never actually called with
a repo_name argument as required). It would be non-trivial to actually
implement the check, as notifications don't track their repository
relationship explicitly, and even then, it's unclear why it would
make sense to allow a repository admin to see notifications to
other users.

It was never a vulnerability, due to a subsequent (and much stricter)
ownership check, which remains but has been untangled for readability.
In short, this changeset is a pure refactoring, except that specifying
a non-existent notification ID will now produce error 404, not 403.

1 file changed with 13 insertions and 17 deletions:

kallithea/controllers/admin/notifications.py

0 comments (0 inline, 0 general)

kallithea/controllers/admin/notifications.py

➞

Show inline comments

@@ @@ -102,73 +102,69 @@ class NotificationsController(BaseContro @@
     def update(self, notification_id):
         """PUT /_admin/notifications/id: Update an existing item"""
         # Forms posted to this method should contain a hidden field:
         #    <input type="hidden" name="_method" value="PUT" />
         # Or using helpers:
         #    h.form(url('notification', notification_id=ID),
         #           method='put')
         # url('notification', notification_id=ID)
         try:
             no = Notification.get(notification_id)
             owner = all(un.user.user_id == c.authuser.user_id
                         for un in no.notifications_to_users)
             if h.HasPermissionAny('hg.admin')() or owner:
                 # deletes only notification2user
                 NotificationModel().mark_read(c.authuser.user_id, no)
                 Session().commit()
                 return 'ok'
         except Exception:
             Session().rollback()
             log.error(traceback.format_exc())
         raise HTTPBadRequest()
     def delete(self, notification_id):
         """DELETE /_admin/notifications/id: Delete an existing item"""
         # Forms posted to this method should contain a hidden field:
         #    <input type="hidden" name="_method" value="DELETE" />
         # Or using helpers:
         #    h.form(url('notification', notification_id=ID),
         #           method='delete')
         # url('notification', notification_id=ID)
         try:
             no = Notification.get(notification_id)
             owner = any(un.user.user_id == c.authuser.user_id
                         for un in no.notifications_to_users)
             if h.HasPermissionAny('hg.admin')() or owner:
                 # deletes only notification2user
                 NotificationModel().delete(c.authuser.user_id, no)
                 Session().commit()
                 return 'ok'
         except Exception:
             Session().rollback()
             log.error(traceback.format_exc())
         raise HTTPBadRequest()
     def show(self, notification_id, format='html'):
         """GET /_admin/notifications/id: Show a specific item"""
         # url('notification', notification_id=ID)
         c.user = self.authuser
         no = Notification.get(notification_id)
         notification = Notification.get_or_404(notification_id)
         owner = any(un.user.user_id == c.authuser.user_id
                     for un in no.notifications_to_users)
         repo_admin = h.HasRepoPermissionAny('repository.admin')
         if no and (h.HasPermissionAny('hg.admin')() or repo_admin or owner):
             unotification = NotificationModel() \
                             .get_user_notification(c.user.user_id, no)
         unotification = NotificationModel() \
             .get_user_notification(self.authuser.user_id, notification)
             # if this association to user is not valid, we don't want to show
             # this message
             if unotification is not None:
                 if not unotification.read:
                     unotification.mark_as_read()
                     Session().commit()
                 c.notification = no
         # if this association to user is not valid, we don't want to show
         # this message
         if unotification is None:
             raise HTTPForbidden()
                 return render('admin/notifications/show_notification.html')
         if not unotification.read:
             unotification.mark_as_read()
             Session().commit()
         raise HTTPForbidden()
         c.notification = notification
         c.user = self.authuser
         return render('admin/notifications/show_notification.html')
     def edit(self, notification_id, format='html'):
         """GET /_admin/notifications/id/edit: Form to edit an existing item"""
         # url('edit_notification', notification_id=ID)

0 comments (0 inline, 0 general)