Login Redirect Patch #51
Conversation
Ich habe die redaxo/index.php gepatched, damit man nach einem Session-Timeout beim Re-Login nicht immer wieder auf der Startseite landet, sondern auf die zuletzt besuchte Seite, bzw. die Seite, die man eigentlich aufrufen wollte. Funktioniert mit jeder Seite, egal ob AddOn, Medienpool oder Redaxo-Page.
| } else { | ||
| if(!$REX['USER'] && !isset($_SESSION['REDIRECT_REDAXO_AFTER_LOGIN'])){ | ||
| if(!isset($_GET['rex_logout'])){ | ||
| $_SESSION['REDIRECT_REDAXO_AFTER_LOGIN'] = strip_tags($_SERVER['REQUEST_URI']); |
There was a problem hiding this comment.
Hmm warum sollten in der uri tags enthalten sein?
There was a problem hiding this comment.
Macht dass sinn falls vor z.b. Ein post request vorrausgegangen ist??
There was a problem hiding this comment.
IMHO ja, diesen Case jedoch würde ich ignorieren. Es würde jedoch schon ungemein helfen, einfach an der selben Stelle wieder anzukommen. Dieses Fass wollte ich jetzt nicht auf machen, und noch die POSTs nachschicken.
Zum Thema URI-Tags: Dadurch säubere ich halt $_GET. Sonst wäre ja nen XSS möglich. Evtl sollte ich htmlspecialchars() machen?
There was a problem hiding this comment.
Hmm verstehe nicht wie das gegen xss helfen sollte.
There was a problem hiding this comment.
strip_tags killt alle tag elemente, also auch < > etc. bessere wäre natürlich ne preg_replace
There was a problem hiding this comment.
http://stackoverflow.com/questions/3605629/php-prevent-xss-with-strip-tags
Also, strip_tag tut prinzipiell was es soll.. aber htmlspecialchars() wäre sicherlich besser, oder halt preg_replace..
There was a problem hiding this comment.
Hmm da man fast alle zeichen auch mit %irgendwas url codieren kann sehe ich wenig sinn in strip_tags(). Ich schlaf mal drueber ;-)
Htmlspeciachars ist in diesem context aber sichlich auch nicht besser. Hier gehts ja rein um URLs und nicht um html rendering.
There was a problem hiding this comment.
Hier sollten wir ein paar webspaces testen ob die REQUEST_URI von allen Servern geliefert wird
There was a problem hiding this comment.
REQUEST_URI gibts nicht zwingend.. nein.
|
Statt der superglobalen array sollte die rex_* api verwendet werden |
|
Meinst jetzt wegen $_SESSION? Das wäre möglich, aber was ist mit REQUEST_URI ? |
|
Ich meine rex_session() bzw rex_set_session() & rex_server(). Dann sind wir an dieser Stelle auch Instanz-safe. |
|
Also wenn ihr dieses Snippet wirklich übernehmen solltet, würd ichs anpassen.... |
|
+1 |
|
ich finde es grundsätzlich auch gut. wichtig ist. die instance beachten, deswegen haben wir die rex_session und co sachen.
@hirbod Aber gerne machen :) |
|
Prinzipiell auch +1, allerdings sollte es noch so angepasst werden, dass man nur weitergeleitet wird, wenn man sich wieder mit dem selben Benutzer anmeldet. |
|
Ob die abfrage des nutzers noetig ist, weiß ich nicht. Die seite auf die man weitergeleitet wird muss ja selbst sicherstellen (bzw der core) ob der user nach der anmeldung rechte dazu hat. Falls nicht wuerde der core ja automatisch nochmals umleiten wg. Fehlender rechte,.. |
|
@staabm Ja, das sehe ich auch so. Ich habs auch bereits getestet. Es funktioniert, der Core leitet wieder auf die Startseite des Users, wenn man keine Rechte hat. Wg. POST bleib ich bei der Meinung. Nach nem Re-Login würde ich keine POST Daten senden. Ich müsste ja auch alle POSTS sonst prüfen, in eine SESSION speichern und beim Login wieder setzen. Ich weiss nicht, ob das sinnvoll ist. |
|
Wie komme ich sicher an REQUEST_URI und HTTP_HOST und HTTP dran? Vorschläge? Oder wollt ihr das da oben selbst "zu ende" bringen? |
|
POST würd ich auch auf keinen FAll senden.. wg. HTTP/Protocol: https://github.com/gn2netwerk/rexseo/blob/master/config.inc.php#L43 .. afaik gibts nicht mehr options das abzufragen.. |
|
Ich sehe gerade, um die PORT Abfrage kümmert ihr euch aber auch nicht. Muss das wirklich rein? |
|
ich glaub wenn man mit relativen locations arbeitet, dann nein.. die sind zwar offiziell nicht korrekt, funktionieren aber de fakto trotzdem weil die UAs das tolerieren.. |
|
Bei relativen redirects braucht man keinen Port das ist richtig. |
|
Da der Rest von Redaxo auch mit relativen arbeitet würd ich an der Stelle kein extra Faß aufmachen.. d.h. auch relativ und gut.. |
|
Wegen Userabfrage: Mir ging es nicht um die Berechtigung, die ist ja bereits abgedeckt. Ich finde es nur einfach nicht sinnvoll, wenn man auch weitergeleitet wird, wenn man sich mit einem anderen Benutzer einloggt. Umsetzung wäre ja auch kein Problem, man kann den Benutzer ja einfach mit in der Sessionvariable ablegen. |
|
Wie bekomme ich denn bitte beim Logout, bzw. wenn die Session verreckt ist und $REX['USER'] == NULL ist denn den Usernamen heraus? Ich wüsste nicht wie. |
|
Und wie gehts hier weiter? Die Funktion ist wirklich zu praktisch um sie hier versauern zu lassen.. |
|
Hirbod hat recht, dass nach ablauf der Session der Username nicht abgefragt erden kann. Daher ist gregors idee so nicht umsetzbar. Alles andere wurde beschrieben oder ist noch was offen? |
|
...also nach einarbeiten des feedbacks oben kann das in den core rein |
|
Ja, ihr habt Recht, Userabfrage ist doch nicht so einfach möglich. Daher schließe ich mich euch an, also ohne Usercheck. |
|
So, um hier mal wieder Schwung in die Bude zu bekommen.. anbei folgender Vorschlag anhand der comments die hier aufliefen: // snip.. (nur das relevante)..:
if(!rex_session('LOGIN_REDIRECT','boolean') || strpos(rex_session('LOGIN_REDIRECT','string'), 'index.php' === false))
{
header('Location: index.php?page='. $REX['PAGE']);
}
else
{
$login_redirect = rex_session('LOGIN_REDIRECT');
rex_unset_session('LOGIN_REDIRECT');
header('Location: ' . $login_redirect);
}
exit();
}
}
else
{
if(!$REX['USER'] && rex_session('LOGIN_REDIRECT','string')==='')
{
if(rex_get('rex_logout', 'int',-1) !== -1)
{
rex_unset_session('LOGIN_REDIRECT');
}
else
{
rex_set_session('LOGIN_REDIRECT', strip_tags(rex_server('REQUEST_URI','string')));
}
}
}
// snip.. |
|
|
|
Geht ja um backend, sprich kein rewrite.. ist lediglich ne kleine absicherung dagegen das der Wert versehentlich mal totaler Unfug is und man dann ins Nirvana geleitet wird. Ansonsten könnt das auch raus.. |
|
Bezügl. der params whitelist die @dergel vorgeschlagen hat hier mal ad hoc:
.. what else? |
|
Ich würde die Params dann über |
|
Stimmt, linkmap.. habs grad oben rein. Was daran zu speziell für r4 sein soll versteh ich aber nicht. Ob die Lösung sonderlich elegant ist spielt für r4 keine Rolle mehr, wohl aber das feature als solches zur Verfügung zu haben. Ich benutz das seitdem Hirbod die erste Version vorgeschlagen hat.. es ist ne echte Arbeitserleichterung. |
|
Für die 4.4.2 würde ich das eher rausnehmen. Oder habt ihr das ausreichend getestet? |
|
Ich benutze den patch schon länger.. in diversen Installationen.. keine probs, sehr arbeitserleichternd. |
|
Folgendes ist mal passiert: "Der Firmen-Chef" hat einen neuen Block hinzugefügt und hackt herade einen längeren Text per Textile rein. Das Telefon klingelt und er telefoniert ein gutes Stündchen. Danach schreibt er weiter an seinem Artikel und BOOM nach abspeichern des Blockes landet er auf der Logoutseite. Auf die Idee den Back-Button zu drücken und evtl. zur vorherigen Page zurückzukehren ist er nicht gekommen. Sollte man aber auch von nem Redakteur nicht erwarten. Der Text und somit ne halbe Stunde Arbeitszeit waren damit weg. D.h. hier muss imho aufjedenfall was zuerst getan werden. Der Redirect Patch ist ganz nice aber Datenverlust zu erleiden wegen eines zu kurzem Session-Timeouts bzw. fehlendem Meachnismus zum Bestätigen des Logouts ist nicht so fein. Ich habe bei mir den Session-Timout auf 12h erhöht, damit REDAXO einen langen Arbeitstag durchhält, ohne jemandem etwas unter dem Arsch wegzuziehen ;) |
|
@rexdude dein Beispiel ist zwar gut & legitim, aber im Rahmen des bisher skizzierten imho nicht gut bzw. hinreichend betriebssicher umsetzbar. Da müßte man imho einen anderen Weg einschlagen, wäre dann mit deutlich mehr edits am core verbunden, und in Folge noch weniger konsensfähig als es diese kleine Lösung schon (nicht) ist. Eine Imho gute Lösung könnte dann sowas wie ein countdown-timer gesteuertes modales overlay mit login prompt sein, welches die Interaktion mit der page unterbindet bis man per eingabe der credentials sie wieder freigeschaltet hat.. (hint hint: ich glaube das ginge dann sogar per Addon/Plugin ;) |
|
Um mal eine Alternative Lösung für das Problem zu diskutieren: Wie waere es mit setTimeout einfach alle x-minuten (z.B. session timeout - 30 sekunden ) ein AJAX request auf die index.php absetzen, damit die session dadurch autm. Verlaengert wird (solange der user angemeldet). Mir kommt so vor als haetten wir das schonmal diskutiert, bin mir aber net sicher |
|
Ja, wär ne Möglichkeit.. wär auch recht schlank. Wäre dadurch aber gleichbedeutend mit einer endless Session so lange irgendein Fenster offen ist. In meinen Augen nicht schlimm, in einem Office Kontext evtl. nicht mehr ganz so im Sinne von: Fenster offen, Admin beim Middach, Putzfrau gestaltet die Firmenwebsite um.. ;) |
|
@staabm btw.. dann könnt man aber auch gleich n input für |
|
Eine session die sicher verlaengert waere schon besser als einfach nur generell die session duration einzustellen. Die verlaengerung sollte ja nur fuer backend user greifen. Damit es nicht endlos wird koennte man das ganze ja auch auf nen max wert beschraenken, z.b max 6 stunden verlaengern... |
|
@staabm Also ich finde de Idee gut. Hauptsache es wird eine Lösung gefunden damit solche Fälle nicht mehr vorkommen. Ob es max 6 Stunden sind oder mehr, mit oder ohne Ajax entscheidet dann am besten ihr. Und wie schon @jdlx meinte: Die Sache mit der Putzfrau sollte eher selten vorkommen ;) Ich hoffe @hirbod reißt mir nicht den Kopf ab, aber brauchts dann überhaupt noch ein Redirect Patch. Weil dann hätten auch alle Programmierer keinen plötzlichen Rauswurf mehr zu fürchten und ergo wäre ein Redirect zwar nett aber zweitranging, da der Fall ja kaum noch bis gar nicht mehr in der Praxis auftauchen würde. @jdlx Die Idee mit dem Plugin ist cool! Ich frage mich nur zu welchem Addon so ein Plugin wohl passen könnte??? :))) |
|
@rexdude ja das das eine Alternative zu diesem PR, keine Ergänzung. |
|
@staabm ich versteh grad den Unterschied nicht zwischen einem "ping" der bis max. |
|
Der Unterschied ist, dass wenn man die session generell verlängert dass dann auch alle Session-Daten auf den Servern für User liegen bleiben die sich schon gar nicht mehr auf der Seite befinden. Man macht also nicht die Ausnahme zur Regel,... |
|
Ohne expliziten logout ja.. Frage ob das ein Problem wäre. |
|
Logout-Links klicken max. 10% der User. |
|
Ich finde eine automatische Sessionverlängerung mittels AJAX nicht gut. Das ist wie Jan schon sagte eine Endless-Session. Der beste Lösungsweg kam von Jan. Und ich denke, so sollte es sein. Ich benutze einige Web-Tools, ein Beispiel: mite (Zeiterfassung). Mite macht das folgendermaßen: Bist du eingeloggt und nutzt die Seite, bleibst du "alive". Ist klar. Bist du länger abwesend, wird die Session gekillt, aber ohne redirect oder ähnliches. Es wird ein Layer eingeblendet mit der Abfrage der Login-Credentials. Gibt man diese ein, wird im Hintergrund ein neuer "Login" an den Server abgeschickt, mittels Ajax. Das ist smart, das ist fein, das ist super easy umzusetzen. Sogar mittels AddOn oder Plugin. Entscheidet sich der User dann keine Zugangsdaten einzugeben und die Seite zu reloaden, verliert er halt seinen ungespeicherten Stand. Da wir aber ein Layer drüberlegen, kann es z.B. nicht passieren, das der User ein Submit-Button o.Ä. einfach abschickt und rausfliegt. Will er weitermachen, muss er sich einloggen. |
|
@staabm Richtig.. bleibt dennoch die Frage ob es ein prob darstellen würde. Session hijacking? Im Prinzip.. aber wirklich faktisch? So oder so: was den bisherigen PR angeht könnten wir den - weitere Diskussion kann ja stattfinden - erstmal closen von meiner Seite aus. Ich würd jetzt an der Stelle zumindest nicht weiterfrickeln wollen. Falls @rexdude da Bock hat wäre code-wise ein eigener PR eh sinnvoller.. |
|
So, hier das Beispiel.. Gerade darüber gesprochen, schon isses passiert :) |
|
allerdings: wenn der user die checkbox "angemeldet bleiben" aktiviert hat...ist der mechanismus auch für die katz, oder? |
|
@rexdude solche "angemeldet bleiben" Funktionen werden üblicherweise dann per cookie gesetzt und sind halt eine bewußte User-Entscheidung. Was nicht zwingend eine endless Session bedeutet.. häufig ist das dann immernoch "nur" eine deutlich verlängerte Session.. mal so mal so meiner Beobachtung nach zumindest. @hirbod wie triggerst du dein overlay.. ist das ein statischer JS counter ab page-load? Oder kombinierst du das mit AJAX checks ab countdown Zeitpunkt X .. ? |
|
@jdlx Der Overlay wird mittels JS getriggered, nachdem die Session verreckt ist. Davor würde der AJAX-Request nämlich die Session wieder verlängern. Ich gehe stark davon aus, das mite das mit einem internen JS-Counter ab page-load macht. Wenn der Counter abgelaufen ist, wird die Overlay-Maske geladen und darüber gelegt. Ein anderer Lösungsweg fällt mir aktuell nicht ein. Vllt nach dem Essen :) @rexdude Nur weil der Haken jetzt im Screenshot ist, heißt das noch nicht, das ich es drin haben wollte. Hatte ich auch nicht erwähnt, aber generell würde sowas mit einem Cookie gelöst werden, wie @jdlx schon gesagt hatte. |
|
@hirbod achso.. mite.. dachte das wär Teil deiner app. Was einen rein statischen counter ab pageload anginge, beißt sich das imho mit dem (typischen) Szenario, daß mehrere backend pages offen sind, und man davon ausgehen muß, das der counter von einem pageload eines anderen Fensters schon wieder resettet wurde.. das prob ist jetzt: |
|
Vorschlag: Für R4 -> Timeout Erhöhen auf 2h oder evtl. sogar 4h (halber Arbeitstag) + evtl. ganz rudimentär ;) ein Hinweis im Footer "Automatischer Logout nach: 47...46...45min". Sollte der Counter tatzächlich heruntergezählet haben könnte per JS ein Hinweis hochpoppen "Sie wurden ausgeloggt. Wenn Sie ungespeicherte Texte etc. haben, sicher Sie diese jetzt seperat bevor Sie fortfahren". Für R5 -> Evtl. die DeLuxe-Variante ala mite :) Bin jetzt erstmal wech... Cheers |
|
@rexdude Eine Minimallösung für r4 core wäre imho ein Select zum Einstellen von Die overlay Lösung wäre als Plugin umsetzbar .. und als solche dann auch r5 portabel. Zu dem was ich oben schrob wg. der "callback = verlängert Session" Problematik: hab dabei gepflegt die Möglichkeit von Cookies ausgeblendet.. das sollte imho auf dem Wege eigentlich gut machbar sein. Da der PR ja inzwischen eindeutig hinfällig ist, mache ich hier mal dahingehend zu.. weitere Diskussion/Ideen bleiben ja trotzdem möglich. |
ghost
mentioned this pull request
|
@jdlx sollte vielleicht ein neuer issue werden, da es sonst evtl. untergeht... |
ghost
mentioned this pull request
Ich habe die redaxo/index.php gepatched, damit man nach einem
Session-Timeout beim Re-Login nicht immer wieder auf der Startseite
landet, sondern auf die zuletzt besuchte Seite, bzw. die Seite, die man
eigentlich aufrufen wollte. Funktioniert mit jeder Seite, egal ob
AddOn, Medienpool oder Redaxo-Page.