Refactor group_decorator + fix /conf/jobfair 403'ing for logged-in users #328
Conversation
| import simplejson as json | ||
|
|
||
| def main(request): | ||
| return render_to_response('conf/main.html',{"section":"conf", "page":"main"},context_instance=RequestContext(request)) | ||
|
|
||
| def jobfair(request): | ||
| if request.user.is_authenticated(): | ||
| if request.user.is_authenticated() and check_group_admin(['Conference', '!Company'], request): |
There was a problem hiding this comment.
This seems more like a helper function rather than something that should be checked like this. I would stick to using the decorator to check group membership for permissions.
Also, why do we need this check here? What happens if someone who is not company or corporate accesses this page while logged in? Won't they be redirected to login instead of getting the permission denied?
There was a problem hiding this comment.
Won't they be redirected to login instead of getting the permission denied?
Yes - the intent is that they should see the "what this page is for + login" page rather than a nondescriptive "permission denied" one.
Maybe the best solution is to redirect them to a version of the login page without the login form?
Looping in @zmmille2 in case he has any comments.
There was a problem hiding this comment.
While it'd be nice to get in a fix relatively soon, it's not a do or die kind of thing. Trying to access the page logged in gave me permission denied with no relevant info, while accessing the page while not logged in gave me information about resume book and the login pages. I don't want students looking to upload their resumes to think that the page is broken if they're logged in.
That said, trying to repro this just now is now giving me what I'd imagine is the correct behavior. This hasn't been pushed out yet, has it?
There was a problem hiding this comment.
No, it hasn't - but I'm still seeing it (the bug) on prod.
|
After talking with Zach, our plan is is to show the normal login page to everyone. However, if you are logged in and don't have access, the login form will be replaced with an Access Denied message. |
ace-n
force-pushed
the
fix-jobfair
branch
2 times, most recently
from
98132e3
to
4ffb62d
Compare
|
@achalv @sskhandek Can one of you check over this and sign-off if you think it looks good? |
|
LGTM! 🚢 it! |
|
Looks good to me Sujay Khandekar UIUC Class of 2017 | Computer Science On Sun, Sep 27, 2015 at 3:10 AM, Achal Varma notifications@github.com
|
Refactor group_decorator + fix /conf/jobfair 403'ing for logged-in users
As title.
@colegleason mind sanity-checking this (or asking someone else who's qualified to)? (I believe @zmmille2 wants this fixed fairly soon.)