| Message ID | 20170721173926.6327-1-eschwartz@archlinux.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | None | expand |
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
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!
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)")
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(-)