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 7 insertions and 11 deletions:

kallithea/controllers/admin/notifications.py

0 comments (0 inline, 0 general)

kallithea/controllers/admin/notifications.py

➞

Show inline comments

@@ @@ -54,121 +54,117 @@ class NotificationsController(BaseContro @@
     @LoginRequired()
     @NotAnonymous()
     def __before__(self):
         super(NotificationsController, self).__before__()
     def index(self, format='html'):
         """GET /_admin/notifications: All items in the collection"""
         # url('notifications')
         c.user = self.authuser
         notif = NotificationModel().get_for_user(self.authuser.user_id,
                                             filter_=request.GET.getall('type'))
         p = safe_int(request.GET.get('page', 1), 1)
         c.notifications = Page(notif, page=p, items_per_page=10)
         c.pull_request_type = Notification.TYPE_PULL_REQUEST
         c.comment_type = [Notification.TYPE_CHANGESET_COMMENT,
                           Notification.TYPE_PULL_REQUEST_COMMENT]
         _current_filter = request.GET.getall('type')
         c.current_filter = 'all'
         if _current_filter == [c.pull_request_type]:
             c.current_filter = 'pull_request'
         elif _current_filter == c.comment_type:
             c.current_filter = 'comment'
         return render('admin/notifications/notifications.html')
     def mark_all_read(self):
         if request.environ.get('HTTP_X_PARTIAL_XHR'):
             nm = NotificationModel()
             # mark all read
             nm.mark_all_read_for_user(self.authuser.user_id,
                                       filter_=request.GET.getall('type'))
             Session().commit()
             c.user = self.authuser
             notif = nm.get_for_user(self.authuser.user_id,
                                     filter_=request.GET.getall('type'))
             c.notifications = Page(notif, page=1, items_per_page=10)
             return render('admin/notifications/notifications_data.html')
     def create(self):
         """POST /_admin/notifications: Create a new item"""
         # url('notifications')
     def new(self, format='html'):
         """GET /_admin/notifications/new: Form to create a new item"""
         # url('new_notification')
     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)
+            .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 unotification is None:
             raise HTTPForbidden()
                 if not unotification.read:
                     unotification.mark_as_read()
                     Session().commit()
                 c.notification = no
         c.notification = notification
         c.user = self.authuser
                 return render('admin/notifications/show_notification.html')
         raise HTTPForbidden()
     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)