[v3,2/2] Require TUs to explicitly request to overwrite a pkgbase

Message ID 20170721173926.6327-1-eschwartz@archlinux.org
State Superseded, archived
Headers show
Series
  • Untitled series #39
Related show

Commit Message

Eli Schwartz July 21, 2017, 5:39 p.m. UTC
AUR_PRIVILEGED allows people with privileged AUR accounts to evade the
block on non-fast-forward commits. While valid in this case, we should
not do so by default, since in at least one case a TU did this without
realizing there was an existing package.
( https://aur.archlinux.org/packages/rtmidi/ )

Switch to using allow_overwrite to check for destructive actions.
Use .ssh/config "SendEnv" on the TU's side and and sshd_config
"AcceptEnv" in the AUR server to specifically request overwrite access.
TUs should use: `export AUR_OVERWRITE=1; git push`

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
---

v2: only require confirmation for force-pushing history.

If you want to restrict who can use this feature, it should be as simple
as adding a new account_type and modifying the env_vars passthrough,
which can be implemented separately.

v3: It might be somwhat helpful if we actually import os before using
it. :p

But I don't have an aurweb test environment set up...

 INSTALL              | 1 +
 aurweb/git/auth.py   | 2 ++
 aurweb/git/update.py | 3 ++-
 3 files changed, 5 insertions(+), 1 deletion(-)

Comments

Lukas Fleischer July 21, 2017, 8:57 p.m. UTC | #1
On Fri, 21 Jul 2017 at 19:39:26, Eli Schwartz wrote:
> AUR_PRIVILEGED allows people with privileged AUR accounts to evade the
> block on non-fast-forward commits. While valid in this case, we should
> not do so by default, since in at least one case a TU did this without
> realizing there was an existing package.
> ( https://aur.archlinux.org/packages/rtmidi/ )
> 
> Switch to using allow_overwrite to check for destructive actions.
> Use .ssh/config "SendEnv" on the TU's side and and sshd_config
> "AcceptEnv" in the AUR server to specifically request overwrite access.
> TUs should use: `export AUR_OVERWRITE=1; git push`

This may just be nitpicking, but since this commit message currently is
the only documentation for this feature, can we suggest something that
does not preserve AUR_OVERWRITE for future operations instead? I think
you also forgot to specify the --force option.

Maybe `AUR_OVERWRITE=1 git push --force`?

Actually, I would prefer a variable name that makes the request even
more explicit. How about AUR_FORCE_PUSH?

Oh, and talking about documentation, it might be a good idea to add a
sentence on AUR_OVERWRITE/AUR_FORCE_PUSH to the git-serve section in
doc/git-interface.txt as well...

> [...]
> @@ -262,7 +263,7 @@ def main():
>          walker = repo.walk(sha1_old, pygit2.GIT_SORT_TOPOLOGICAL)
>          walker.hide(sha1_new)
>          if next(walker, None) is not None:
> -            if privileged:
> +            if allow_overwrite:
>                  warn("non-fast-forward push (are you absolutely sure you mean this?)")
> [...]

I know I said earlier that the warning is a good idea. However, thinking
about it again, it seems fairly awkward to ask "are you absolutely sure
you mean this?" after the TU said "I really want to force push!" by
repeating "force" twice in `AUR_FORCE_PUSH=1 git push --force`. So could
you please resend this as a separate patch, based on master instead 1/2
in this series?

Thanks!

Regards,
Lukas
Eli Schwartz July 21, 2017, 9:05 p.m. UTC | #2
On 07/21/2017 04:57 PM, Lukas Fleischer wrote:
> Oh, and talking about documentation, it might be a good idea to add a
> sentence on AUR_OVERWRITE/AUR_FORCE_PUSH to the git-serve section in
> doc/git-interface.txt as well...

Will do

> I know I said earlier that the warning is a good idea. However, thinking
> about it again, it seems fairly awkward to ask "are you absolutely sure
> you mean this?" after the TU said "I really want to force push!" by
> repeating "force" twice in `AUR_FORCE_PUSH=1 git push --force`. So could
> you please resend this as a separate patch, based on master instead 1/2
> in this series?

Sure, that and a better commit message.

To be honest, I wrote patch #1 several weeks ago and then figured out
how to implement #2 yesterday, while reading more of the aurweb module.
So it was basically tacked on, and I wasn't even sure whether you would
want it etc.

So I will redo it keeping all your feedback in mind, probably on Sunday.
Thanks for going over this!

Patch

diff --git a/INSTALL b/INSTALL
index 8c9c4dd..369e1e3 100644
--- a/INSTALL
+++ b/INSTALL
@@ -76,6 +76,7 @@  read the instructions below.
         PasswordAuthentication no
         AuthorizedKeysCommand /usr/local/bin/aurweb-git-auth "%t" "%k"
         AuthorizedKeysCommandUser aur
+        AcceptEnv AUR_OVERWRITE
 
 9) If you want to enable smart HTTP support with nginx and fcgiwrap, you can
    use the following directives:
diff --git a/aurweb/git/auth.py b/aurweb/git/auth.py
index 022b0ff..d02390d 100755
--- a/aurweb/git/auth.py
+++ b/aurweb/git/auth.py
@@ -1,5 +1,6 @@ 
 #!/usr/bin/python3
 
+import os
 import shlex
 import re
 import sys
@@ -52,6 +53,7 @@  def main():
     env_vars = {
         'AUR_USER': user,
         'AUR_PRIVILEGED': '1' if account_type > 1 else '0',
+        'AUR_OVERWRITE' : os.environ.get('AUR_OVERWRITE', '0') if account_type > 1 else '0',
     }
     key = keytype + ' ' + keytext
 
diff --git a/aurweb/git/update.py b/aurweb/git/update.py
index 3b9ff97..5419338 100755
--- a/aurweb/git/update.py
+++ b/aurweb/git/update.py
@@ -238,6 +238,7 @@  def main():
     user = os.environ.get("AUR_USER")
     pkgbase = os.environ.get("AUR_PKGBASE")
     privileged = (os.environ.get("AUR_PRIVILEGED", '0') == '1')
+    allow_overwrite = (os.environ.get("AUR_OVERWRITE", '0') == '1')
     warn_or_die = warn if privileged else die
 
     if len(sys.argv) == 2 and sys.argv[1] == "restore":
@@ -262,7 +263,7 @@  def main():
         walker = repo.walk(sha1_old, pygit2.GIT_SORT_TOPOLOGICAL)
         walker.hide(sha1_new)
         if next(walker, None) is not None:
-            if privileged:
+            if allow_overwrite:
                 warn("non-fast-forward push (are you absolutely sure you mean this?)")
             else:
                 die("denying non-fast-forward (you should pull first)")