Should handlers check permissions?

[tedfordgif]

Version: 0.11b1 (or trunk)

From wiki/web_ui.py:

def match_request(self, req):
match = re.match(r'^/wiki(?:/(.*)|$)', req.path_info)
if 'WIKI_VIEW' in req.perm('wiki') and match:
if match.group(1):
req.args['page'] = match.group(1)
return 1

This causes the permredirect plugin to fail for anonymous requests
like /wiki/WikiStart--since no handler matches, the "no handler" 404
is thrown.

Wouldn't it make more sense to check the permissions elsewhere (such
as in the permredirect plugin)? Or would that require API changes?

This works with PermRedirect:

def match_request(self, req):
match = re.match(r'^/wiki(?:/(.*)|$)', req.path_info)
- if 'WIKI_VIEW' in req.perm('wiki') and match:
+ if match:
if match.group(1):
req.args['page'] = match.group(1)
return 1

[Hyuga]

On Feb 19, 12:15 pm, tedfordgif <tedford...@gmail.com> wrote:
> Version: 0.11b1 (or trunk)
>
> From wiki/web_ui.py:
>
> def match_request(self, req):
> match = re.match(r'^/wiki(?:/(.*)|$)', req.path_info)
> if 'WIKI_VIEW' in req.perm('wiki') and match:
> if match.group(1):
> req.args['page'] = match.group(1)
> return 1
>
> This causes the permredirect plugin to fail for anonymous requests
> like /wiki/WikiStart--since no handler matches, the "no handler" 404
> is thrown.
>

> Wouldn't it make more sense to check thepermissionselsewhere (such

> as in the permredirect plugin)? Or would that require API changes?

I have to agree with this, not so much in the context of the
permredirect plugin, but just in general. I've been aware of this for
a while, but I just finally got a user complaining about it, and I
have to agree with them. For example, their project disables all
anonymous access, so when a user goes to WikiStart they just get a 404-
like message until they log in, rather than (to them) a more obvious
permission error message.

Is there a reason that the WIKI_VIEW permission needs to be checked in
match_request(), given that it's already checked towards the beginning
of process_request()? Even if there is a reason, perhaps it would be
better to call req.perm('wiki').require('WIKI_VIEW'), so that a
PermissionError is raised, instead of HTTPNotFound. Futhermore, it
seems odd that there's a permission check in the match_request() for
the WikiModule, but not, say, for the TicketModule. Is there a reason
for this seeming inconsistency?

Thanks,
Erik

[osimons]

On Feb 19, 6:15 pm, tedfordgif <tedford...@gmail.com> wrote:
> Version: 0.11b1 (or trunk)

> def match_request(self, req):
> match = re.match(r'^/wiki(?:/(.*)|$)', req.path_info)
> - if 'WIKI_VIEW' in req.perm('wiki') and match:
> + if match:
> if match.group(1):
> req.args['page'] = match.group(1)
> return 1

Agree with arguments raised to remove permission check in
match_request(). Review of all modules showed that this was the only
module doing it this way, and as you both say it makes more sense to
recognize the match here and leave permissions for process_request()
to deal with.

Patch committed in [6819].

Thanks!


:::simon

https://www.coderesort.com