search

japacible / commission-me

Platform for commissioners and buyers to connect and finalize sales.
http://commissionme.herokuapp.com/
4 stars 3 forks source link

Code review for log-in verification and better redirect #214

Closed kcorman closed 10 years ago

kcorman commented 10 years ago

678f5db

Added two helpful methods: One is a general purpose 'redirect_back' which can be used to attempt to redirect the user backwards, and then redirect to root if that fails without throwing an exception. The other is a method which redirects a non-logged in user to the authentication page, where they can authenticate. In addition the authentication page has been updated to forward users to their original destination if it was set.

jbrodhacker commented 10 years ago

Sorry, I don't think I got an email about this at all! I'm reviewing it now.

jbrodhacker commented 10 years ago

Try as I might, I can't find anything wrong with it. I'm just curious if it makes a difference (or if it'd even be possible) if the page request was a POST rather than a GET.

jbrodhacker commented 10 years ago

Other than that curiosity, it looks good to me :)

kcorman commented 10 years ago

It totally would! I can tell you really reviewed the code thoroughly. :) POST parameters wouldn't go through. I suspect if you managed to, for example, commission an artist and then opened another tab to log out, then when you hit submit on the original tab it would try to redirect you to authentication, then from there to "update" controller. This would result in an empty params given to that controller. I consider this a minor issue and more trouble than its worth to solve right now, and controller should be handling bad input graciously anyway (even though the ones I set up they probably don't >.>)

jbrodhacker commented 10 years ago

Yeah, I don't think I have much nil checking either aside from the current_user... I think it'd be more important (but only slightly) if we had an expiration for how long someone can be logged in without being active. I agree with not dealing with it right now :)