[PATCH 1 of 2] model: check for invalid users centrally

[Mads Kiilerich]

On 06/22/2015 06:45 PM, Thomas De Schampheleire wrote:
> # HG changeset patch
> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
> # Date 1434984724 -7200
> #      Mon Jun 22 16:52:04 2015 +0200
> # Node ID 68032f8bd7f52c676bb5143a03b0dfd5df242f0c
> # Parent  7303d0035d82a20f93df1c78b3358403b7ff9827
> model: check for invalid users centrally
>
> Commit 9a23b444a7fefe5b67e57a42d26343787d992874 introduced
> UserInvalidException. The raising of this exception, however, was done only
> in pullrequest context, while it could have been done in
> BaseModel._get_user().

Why not be more consistent and do it in _get_instance (using a more 
generic EntityNotFound exception (or something like that)? Is that the 
end goal and this just an intermediate step?

>
> Note: the same 'use-exceptions-rather-than-explicit-checks-for-None'
> approach could actually be introduced for the other _get_X methods in
> model, like _get_repo.

Why do you think exceptions are better than None? Is it often one extra 
line of code and usually slower (not that matters in most of these 
cases). (I might agree but it would be nice to have reasoning/assumption 
stated explicitly.)

> diff --git a/kallithea/controllers/api/api.py b/kallithea/controllers/api/api.py
> --- a/kallithea/controllers/api/api.py
> +++ b/kallithea/controllers/api/api.py
> @@ -37,6 +37,7 @@ from kallithea.lib.auth import (
>       PasswordGenerator, AuthUser, HasPermissionAllDecorator,
>       HasPermissionAnyDecorator, HasPermissionAnyApi, HasRepoPermissionAnyApi,
>       HasRepoGroupPermissionAnyApi, HasUserGroupPermissionAny)
> +from kallithea.lib.exceptions import UserInvalidException
>   from kallithea.lib.utils import map_groups, repo2db_mapper
>   from kallithea.lib.utils2 import (
>       str2bool, time_to_datetime, safe_int, Optional, OAttr)
> @@ -72,8 +73,9 @@ def get_user_or_error(userid):
>   
>       :param userid:
>       """
> -    user = UserModel().get_user(userid)
> -    if user is None:
> +    try:
> +        user = UserModel().get_user(userid)
> +    except UserInvalidException, u:
>           raise JSONRPCError("user `%s` does not exist" % (userid,))
>       return user
>   
> diff --git a/kallithea/lib/base.py b/kallithea/lib/base.py
> --- a/kallithea/lib/base.py
> +++ b/kallithea/lib/base.py
> @@ -50,7 +50,7 @@ from kallithea.lib.utils2 import str2boo
>   from kallithea.lib import auth_modules
>   from kallithea.lib.auth import AuthUser, HasPermissionAnyMiddleware, CookieStoreWrapper
>   from kallithea.lib.utils import get_repo_slug
> -from kallithea.lib.exceptions import UserCreationError
> +from kallithea.lib.exceptions import UserCreationError, UserInvalidException
>   from kallithea.lib.vcs.exceptions import RepositoryError, EmptyRepositoryError, ChangesetDoesNotExistError
>   from kallithea.model import meta
>   
> @@ -331,12 +331,16 @@ class BaseController(WSGIController):
>   
>           c.repo_name = get_repo_slug(request)  # can be empty
>           c.backends = BACKENDS.keys()
> -        c.unread_notifications = NotificationModel()\
> -                        .get_unread_cnt_for_user(c.authuser.user_id)
>   
>           self.cut_off_limit = safe_int(config.get('cut_off_limit'))
>   
> -        c.my_pr_count = PullRequestModel().get_pullrequest_cnt_for_user(c.authuser.user_id)
> +        try:
> +            c.unread_notifications = NotificationModel()\
> +                            .get_unread_cnt_for_user(c.authuser.user_id)
> +            c.my_pr_count = PullRequestModel().get_pullrequest_cnt_for_user(c.authuser.user_id)
> +        except UserInvalidException, u:
> +            c.unread_notifications = 0
> +            c.my_pr_count = 0

It seems like NotificationModel already quietly ignored invalid user 
IDs. That seems weird and wrong ... but perhaps a consequence of the 
"feature" of being able to delete users? It seems like the weirdness 
propagates a bit here ...

/Mads


>           self.sa = meta.Session
>           self.scm_model = ScmModel(self.sa)
> diff --git a/kallithea/model/__init__.py b/kallithea/model/__init__.py
> --- a/kallithea/model/__init__.py
> +++ b/kallithea/model/__init__.py
> @@ -46,6 +46,7 @@ Original author and date, and relevant c
>   
>   import logging
>   from kallithea.model import meta
> +from kallithea.lib.exceptions import UserInvalidException
>   from kallithea.lib.utils2 import safe_str, obfuscate_url_pw
>   
>   log = logging.getLogger(__name__)
> @@ -110,8 +111,11 @@ class BaseModel(object):
>           :param user: UserID, username, or User instance
>           """
>           from kallithea.model.db import User
> -        return self._get_instance(User, user,
> -                                  callback=User.get_by_username)
> +        u = self._get_instance(User, user,
> +                               callback=User.get_by_username)
> +        if u is None:
> +            raise UserInvalidException(user)
> +        return u
>   
>       def _get_repo(self, repository):
>           """
> diff --git a/kallithea/model/notification.py b/kallithea/model/notification.py
> --- a/kallithea/model/notification.py
> +++ b/kallithea/model/notification.py
> @@ -35,6 +35,7 @@ from sqlalchemy.orm import joinedload, s
>   
>   import kallithea
>   from kallithea.lib import helpers as h
> +from kallithea.lib.exceptions import UserInvalidException
>   from kallithea.lib.utils2 import safe_unicode
>   from kallithea.model import BaseModel
>   from kallithea.model.db import Notification, User, UserNotification
> @@ -84,10 +85,10 @@ class NotificationModel(BaseModel):
>           recipients_objs = []
>           if recipients:
>               for u in recipients:
> -                obj = self._get_user(u)
> -                if obj:
> +                try:
> +                    obj = self._get_user(u)
>                       recipients_objs.append(obj)
> -                else:
> +                except UserInvalidException:
>                       # TODO: inform user that requested operation couldn't be completed
>                       log.error('cannot email unknown user %r', u)
>               recipients_objs = set(recipients_objs)
> @@ -154,7 +155,7 @@ class NotificationModel(BaseModel):
>           try:
>               notification = self.__get_notification(notification)
>               user = self._get_user(user)
> -            if notification and user:
> +            if notification:
>                   obj = UserNotification.query()\
>                           .filter(UserNotification.user == user)\
>                           .filter(UserNotification.notification
> @@ -192,7 +193,7 @@ class NotificationModel(BaseModel):
>           try:
>               notification = self.__get_notification(notification)
>               user = self._get_user(user)
> -            if notification and user:
> +            if notification:
>                   obj = UserNotification.query()\
>                           .filter(UserNotification.user == user)\
>                           .filter(UserNotification.notification
> diff --git a/kallithea/model/pull_request.py b/kallithea/model/pull_request.py
> --- a/kallithea/model/pull_request.py
> +++ b/kallithea/model/pull_request.py
> @@ -32,7 +32,6 @@ from pylons.i18n.translation import _
>   
>   from kallithea.model.meta import Session
>   from kallithea.lib import helpers as h
> -from kallithea.lib.exceptions import UserInvalidException
>   from kallithea.model import BaseModel
>   from kallithea.model.db import PullRequest, PullRequestReviewers, Notification,\
>       ChangesetStatus, User
> @@ -119,8 +118,6 @@ class PullRequestModel(BaseModel):
>           #members
>           for member in set(reviewers):
>               _usr = self._get_user(member)
> -            if _usr is None:
> -                raise UserInvalidException(member)
>               reviewer = PullRequestReviewers(_usr, pr)
>               Session().add(reviewer)
>