On Fri, 26 Jun 2015 at 21:03:17, Gordian Edenhofer wrote:
Displaying flag, notify, vote, adopt and file request links for users which did not authenticate themselves and letting those fake buttons link to the login page.
Forgot to mention it in your previous patches but could you add your sign-off, please (simply pass -s when running `git commit`)?
--- Agreed, the statements were kind of redundant. I hope this patch is more straightforward.
Looks much better indeed, thanks! Some comments below.
web/lib/aur.inc.php | 33 ++++++++++++++++++++++----------- web/template/pkgbase_actions.php | 26 ++++++++++++-------------- 2 files changed, 34 insertions(+), 25 deletions(-)
diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php index 95f72ce..98ebde4 100644 --- a/web/lib/aur.inc.php +++ b/web/lib/aur.inc.php @@ -226,11 +226,16 @@ function html_format_maintainers($maintainer, $comaintainers) { * * @param string $uri The link target * @param string $desc The link label + * @param string $uid The User ID * * @return string The generated HTML code for the action link */ -function html_action_link($uri, $desc) { - $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; +function html_action_link($uri, $desc, $uid="") {
Do we really need to make the $uid parameter optional? Do we skip the parameter somewhere? If it makes sense, could we please use false as default value instead of an empty string?
+ if ($uid) { + $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '">'; + } [...]
Cool, this is kind of what I expected! I wonder whether we can directly set a referer here, though? Shouldn't something like get_uri('/login/', true) . '?referer=' . urlencode($uri) work? Having said that, I now see a potential problem with the GET parameter approach of implementing FS#32481. You could build a malicious login link that redirects to a certain page and send that link to a privileged user (i.e. a TU or a Developer). I am not aware of any action that cannot be undone and doesn't require any additional confirmation, so that probably isn't very critical. We should fix it anyway... Regards, Lukas