[drupal-devel] [bug] User should be redirected once logged in [SOLUTION-COREERROR]

Sun Aug 21 13:53:13 UTC 2005

Issue status update for 
http://drupal.org/node/24587
Post a follow up: 
http://drupal.org/project/comments/add/24587

 Project:      Drupal
 Version:      cvs
 Component:    base system
 Category:     bug reports
 Priority:     normal
 Assigned to:  Anonymous
 Reported by:  nysus
 Updated by:   Tobias Maier
 Status:       patch (code needs review)
 Attachment:   http://drupal.org/files/issues/issue24587_2_common.inc.patch (1.46 KB)

i dont like it too
but this was the fastest way to do it :D
here comes a new patch

but this changes drupal_get_destination() a little bit

cu tobias

Tobias Maier

Previous comments:
------------------------------------------------------------------------

Wed, 08 Jun 2005 17:39:26 +0000 : nysus

When a user logs in after viewing the custom 403 page, the 403 page is
still displayed.  The user should instead see the page they were trying
to request.

------------------------------------------------------------------------

Wed, 08 Jun 2005 18:23:30 +0000 : Dublin Drupaller

as opposed to a patch..you could try this....

<?php
// get referrer from _SERVER array
$ref = $_SERVER["HTTP_REFERER"];
?>

and insert this into your form action line:

form action="user/login?destination=$ref" method="post"

The php script at the top catches the reffering url and assigns to to
the $ref variable...
I haven't tested the above, so it's only a suggestion...might be easier
to implement that in your custom 403 page rather than wait for a patch.

Dub

------------------------------------------------------------------------

Wed, 08 Jun 2005 20:27:32 +0000 : nysus

Attachment: http://drupal.org/files/issues/customerror.patch (2.73 KB)

Thanks, Dub. But I have a patch here.  Give it a shot if you want.  It's
probably a little more convoluted than it needs to be but it appears to
work.  The good part about it is that you don't have to hack the core. 
The bad part about it is that I had to get rid of the $may_cache
variable for it to work.  The other quirk about it is that let's say
you have user 1 and he gets a 403 error for trying to view admin/block
without authorization.  Then user 2 comes along and gets a 403 error
for trying view admin/settings without authorization.  Then when user 1
logs in, he will see admin/settings instead of admin/block (the page he
was originally trying to view).  This will only be a problem on busy
sites.

If you look at the code, you'll see that I drew heavily from some menu
functions to determine if the user is now authorized to look at a page.
 Basically they way it works is this:

If you get a 403 error, the module does a variable_set to see what path
you were trying to access. When you login, the customerror module
basically runs the same two functions as the menu module to see if the
user has access.  If so, the module does a redirect to the page by
pulling the old path out with variable get.

I'll leave it to a pro with better than ideas to polish this up. 
Hopefully this code has helped pave the way.

------------------------------------------------------------------------

Thu, 09 Jun 2005 02:03:22 +0000 : kbahey

Thanks for the patch nysus.

I don't mind removing the $maycache, but it may be a performance hit
for some large sites.

I think the patch as it exists, is overly overcomplicated. Too much
code in there.

The reason you were getting different users getting each other's path
is that it is saved in the database.

I was thinking of something along the lines of what Dub said. Check if
the referrer is set, and then issue a drupal_goto(). 

Try to replace the customerror_page function with the version below,
merged to the original module (not the nysus modified one).

I did not test it though.

<?php
function customerror_page() {
  $error = arg(1);
  switch($error) {
    case 404:
      drupal_set_title(variable_get('customerror_'. $error
.'_title',
        _customerror_fetch_error($error)));
      $body = variable_get('customerror_' . $error,
_customerror_fetch_error($error));
    case 403:
      //
      $referer = $_SERVER["HTTP_REFERER"];
      if ($referer) {
        drupal_goto($referer);
        }
      break;
    default:
      drupal_set_title(t('undefined error: ') . $error);
      $body = _customerror_fetch_error($error);
      break;
  }
  print theme('page', $body);
}
?>

------------------------------------------------------------------------

Thu, 09 Jun 2005 02:04:59 +0000 : kbahey

Just realize that this may cause a redirection loop (403 -> original
page -> 403 -> ...).

Oh well. Someone may think of something better.

------------------------------------------------------------------------

Thu, 09 Jun 2005 16:08:10 +0000 : nysus

Yes, infinite redirects are a problem.  I ran into it quite frequently
while developing this solution.

Like I said, there is probably a better way.  I don't see any way
around using the database, however.  You need to be able to remember
the initial page the user tried to come through.  

To avoid the problem I mentioned above of one user clobbering the other
user's redirect is to use info in the user's cookie to give him a unique
"redirect identifier."  But then the database will get littered with
redirect data.  You would have to set up a cron to delete the old ones.

I'll also try Dublin's idea at some point and see how it goes.

------------------------------------------------------------------------

Thu, 09 Jun 2005 16:28:13 +0000 : kbahey

The session ID is a good unique identifier.

So, we could use the Session ID to identify the original page that we
would redirect to.

However, if we use variable_get() this means there will be one entry
per session, and on a large site, this can be a) too big, and b) a
significant overhead

Unless we delete it from the table upon redirect, which solves a) but
worsens b) (more db operations).

Perhaps we can make this a config option. If the site is small, then
this is a good option to turn on. If it is a large site, then the site
admin should turn this off.

------------------------------------------------------------------------

Mon, 04 Jul 2005 14:52:37 +0000 : spiff06

+1

------------------------------------------------------------------------

Mon, 04 Jul 2005 17:36:40 +0000 : kbahey

Please do not change the title. It will make it difficult for others to
find the issues.

As for a solution, the patches provided seem to work, but they are
overly complex.

I am not going to apply them as is, unless a simpler and cleaner patch
arise.

------------------------------------------------------------------------

Sun, 17 Jul 2005 02:14:04 +0000 : ymcp

Has there been any progress on this?  It may seem like a minor issue,
but it is a real show-stopper for a site that I am trying to port to
drupal.

Users are not permitted to see any content without being logged in, so
the custom 403 page serves very well as a "welcome, please log in"
screen... but when they log in they still see the "please log in"
message.  They're going to find that very confusing.

------------------------------------------------------------------------

Sun, 17 Jul 2005 07:58:38 +0000 : kbahey

As far as the solutions mentioned, they are too complex and not simple
and clean.

I am waiting for someone to put a better patch, since I am not working
actively on this at the moment.

------------------------------------------------------------------------

Tue, 19 Jul 2005 07:22:25 +0000 : kbahey

When this patch is in core, it could be the basis of a better solution.

Maybe the previous patch to CustomError can be reworked in the future
to use this.

------------------------------------------------------------------------

Thu, 21 Jul 2005 07:19:13 +0000 : kbahey

Redirection is now in core. Check it here http://drupal.org/node/26467

Anyone cares for a patch based on that?

------------------------------------------------------------------------

Sun, 21 Aug 2005 12:40:15 +0000 : Tobias Maier

I'm investigating this issue

as far as I can say now it is _no_ error of customerror its an error of
the user-login-block and a little bit of menu_set_active_item()
(the form-destination is set to the URL of the error-page)

maybe youll get a solution today.

but this will affect the core.

i dont want to change the Project/component by now.
but if I have a solution

cu tobias

------------------------------------------------------------------------

Sun, 21 Aug 2005 13:10:22 +0000 : Tobias Maier

Attachment: http://drupal.org/files/issues/issue24587_common.inc.patch (612 bytes)

I found a solution
1. As I mentioned this is no error of customerror!
2. this is a bug and no new feature witch should come (because it is
done the right way if you are not using any other errorpage

the patch adds the line
$_REQUEST['destination'] =
urldecode(substr(drupal_get_destination(),12));
to drupal_access_denied()
--> the $_REQUEST['destination'] is now set before user_block() asks
for destionation

Ciao Tobias

------------------------------------------------------------------------

Sun, 21 Aug 2005 13:21:26 +0000 : kbahey

Attachment: http://drupal.org/files/issues/issue24587_common.inc_0.patch (606 bytes)

The patch is garbled (DOS editor?)

Here is the same patch in a proper format so others can review it.

What I do not like is the 12 in the substr. This kind of hard coded
numbers has to be at least documented so it can be changed in the
future if need be.