From 347896fa4718e1b724a2adcc2f046791cf11cb17 Mon Sep 17 00:00:00 2001 From: Magnus Hagander Date: Tue, 11 Aug 2020 16:32:57 +0200 Subject: [PATCH] Make references to users be foreign key instead of string matches We've previously done string matching on usernames which comes with all sorts of problems :/ Instead, do a proper foreign key to the users model. Our authentication system already (and for a long time) supports importing a user by email address from upstream if the user doesn't exist, so instead update the integration to do that when adding new users with permissions. NOTE! Prior to deploying this migration all users referenced in the permissions and ssh tables *must* exist in auth_users, otherwise the migration will fail. --- gitadmin/gitadmin/adm/forms.py | 40 +++++++++++- .../adm/migrations/0002_user_foreign_key.py | 61 +++++++++++++++++++ gitadmin/gitadmin/adm/models.py | 15 +++-- gitadmin/gitadmin/adm/templates/repoview.html | 5 ++ gitadmin/gitadmin/adm/util.py | 17 ++++++ gitadmin/gitadmin/adm/views.py | 35 +++++++---- gitdump.py | 8 +-- keysync.py | 2 +- pggit.py | 2 +- 9 files changed, 161 insertions(+), 24 deletions(-) create mode 100644 gitadmin/gitadmin/adm/migrations/0002_user_foreign_key.py create mode 100644 gitadmin/gitadmin/adm/util.py diff --git a/gitadmin/gitadmin/adm/forms.py b/gitadmin/gitadmin/adm/forms.py index 33e7aa8..9101e36 100644 --- a/gitadmin/gitadmin/adm/forms.py +++ b/gitadmin/gitadmin/adm/forms.py @@ -1,8 +1,10 @@ +from django.core.exceptions import ValidationError from django import forms from django.forms import ModelForm, Form -from gitadmin.adm.models import Repository +from gitadmin.adm.models import Repository, RepositoryPermission, PERMISSION_CHOICES +from gitadmin.adm.util import get_or_import_user class RepositoryForm(ModelForm): initialclone = forms.RegexField(r'^(git://.+/.+|[^:]+)$', max_length=256, required=False, @@ -11,8 +13,42 @@ class RepositoryForm(ModelForm): class Meta: model = Repository - exclude = ('repoid', 'name', ) + exclude = ('repoid', 'name', 'remoterepository') class ConfirmDeleteForm(Form): confirmed = forms.BooleanField(required=True, label="Confirm deleting the repository") + + +class RepositoryPermissionForm(forms.ModelForm): + username = forms.CharField(label="User") + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.fields['username'].widget = forms.widgets.TextInput(attrs={'readonly': 'readonly'}) + self.fields['username'].initial = "{} {} <{}>".format(self.instance.user.first_name, self.instance.user.last_name, self.instance.user.email) + + +class RepositoryPermissionsAddForm(forms.Form): + addemail = forms.EmailField(label='Add email', required=False) + addlevel = forms.ChoiceField(label='Permission', choices=PERMISSION_CHOICES) + + def __init__(self, repo, *args, **kwargs): + self.repo = repo + super().__init__(*args, **kwargs) + + def clean_addemail(self): + e = self.cleaned_data['addemail'] + if not e: + # Not mandatory! + return e + + # Find the user? + try: + u = get_or_import_user(e) + except Exception: + raise ValidationError("User with this email address not found") + + if RepositoryPermission.objects.filter(user=u, repository=self.repo).exists(): + raise ValidationError("This user already has a permissions entry on this repository") + + return e diff --git a/gitadmin/gitadmin/adm/migrations/0002_user_foreign_key.py b/gitadmin/gitadmin/adm/migrations/0002_user_foreign_key.py new file mode 100644 index 0000000..5b46572 --- /dev/null +++ b/gitadmin/gitadmin/adm/migrations/0002_user_foreign_key.py @@ -0,0 +1,61 @@ +# Generated by Django 2.2.11 on 2020-08-09 14:09 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('adm', '0001_initial'), + ] + + operations = [ + # Remove old primary key + migrations.AlterField( + model_name='gituser', + name='userid', + field=models.CharField(max_length=255, serialize=False), + ), + migrations.AddField( + model_name='gituser', + name='user', + field=models.OneToOneField(null=True, on_delete=django.db.models.deletion.CASCADE, serialize=False, to=settings.AUTH_USER_MODEL), + ), + migrations.RunSQL( + "UPDATE git_users SET user_id=(SELECT id FROM auth_user WHERE username=userid)", + ), + migrations.AlterField( + model_name='gituser', + name= 'user', + field=models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to=settings.AUTH_USER_MODEL), + ), + migrations.RemoveField( + model_name='gituser', + name='userid', + ), + + migrations.AddField( + model_name='repositorypermission', + name='user', + field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL), + ), + migrations.RunSQL( + "UPDATE repository_permissions SET user_id=(SELECT id FROM auth_user WHERE username=userid)", + ), + migrations.AlterField( + model_name='repositorypermission', + name='user', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL), + ), + migrations.AlterUniqueTogether( + name='repositorypermission', + unique_together={('repository', 'user')}, + ), + migrations.RemoveField( + model_name='repositorypermission', + name='userid', + ), + ] diff --git a/gitadmin/gitadmin/adm/models.py b/gitadmin/gitadmin/adm/models.py index 15ebffe..e20376a 100644 --- a/gitadmin/gitadmin/adm/models.py +++ b/gitadmin/gitadmin/adm/models.py @@ -1,3 +1,4 @@ +from django.contrib.auth.models import User from django.db import models import datetime @@ -45,7 +46,7 @@ class Repository(models.Model): verbose_name='Remote repository') def ValidateOwnerPermissions(self, user): - if not self.repositorypermission_set.filter(userid=user.username, level=2).exists(): + if not self.repositorypermission_set.filter(user=user, level=2).exists(): raise Exception('You need owner permissions to do that!') def __str__(self): @@ -58,7 +59,7 @@ class Repository(models.Model): class RepositoryPermission(models.Model): repository = models.ForeignKey(Repository, db_column='repository', on_delete=models.CASCADE) - userid = models.CharField(max_length=255, blank=False, verbose_name="User id") + user = models.ForeignKey(User, null=False, blank=False, on_delete=models.CASCADE) level = models.IntegerField(default=0, verbose_name='Permission', choices=PERMISSION_CHOICES) @property @@ -70,7 +71,7 @@ class RepositoryPermission(models.Model): return (self.level > 1) def __str__(self): - return "%s (%s)" % (self.userid, self.__permstr()) + return "%s (%s)" % (self.user.username, self.__permstr()) def __permstr(self): if self.level == 2: @@ -79,13 +80,17 @@ class RepositoryPermission(models.Model): return "Write" return "Read" + @property + def username(self): + return self.user.username + class Meta: db_table = 'repository_permissions' - unique_together = (('repository', 'userid'), ) + unique_together = (('repository', 'user'), ) class GitUser(models.Model): - userid = models.CharField(max_length=255, blank=False, primary_key=True) + user = models.OneToOneField(User, null=False, blank=False, primary_key=True, on_delete=models.CASCADE) sshkey = models.CharField(max_length=10240, blank=True) class Meta: diff --git a/gitadmin/gitadmin/adm/templates/repoview.html b/gitadmin/gitadmin/adm/templates/repoview.html index b1112e8..e0d567e 100644 --- a/gitadmin/gitadmin/adm/templates/repoview.html +++ b/gitadmin/gitadmin/adm/templates/repoview.html @@ -37,6 +37,11 @@ automatically upon creation. {%endfor%} {% endfor %} + + {%include "form_field.html" with field=addform.addemail%} + {%include "form_field.html" with field=addform.addlevel%} + + diff --git a/gitadmin/gitadmin/adm/util.py b/gitadmin/gitadmin/adm/util.py new file mode 100644 index 0000000..ed7ce83 --- /dev/null +++ b/gitadmin/gitadmin/adm/util.py @@ -0,0 +1,17 @@ +from django.contrib.auth.models import User + +from gitadmin.auth import user_search, user_import + +def get_or_import_user(email): + try: + return User.objects.get(email=email) + except User.DoesNotExist: + pass + + # If the user didn't exist, then try to import it. We have to start + # by doing an email search, and then later import it. + users = user_search(searchterm=email) + if len(users) != 1 or users[0]['e'] != email: + raise Exception("User not found") + + return user_import(users[0]['u']) diff --git a/gitadmin/gitadmin/adm/views.py b/gitadmin/gitadmin/adm/views.py index bfbd935..11cd4ca 100644 --- a/gitadmin/gitadmin/adm/views.py +++ b/gitadmin/gitadmin/adm/views.py @@ -24,7 +24,7 @@ def _MissingSshkey(user): if not user.is_authenticated: return False try: - gu = GitUser.objects.get(userid=user.username) + gu = GitUser.objects.get(user=user) if gu.sshkey != '': return False else: @@ -43,9 +43,9 @@ def context_add(request): @login_required def index(request): repos = Repository.objects.extra( - where=["remoterepository_id IS NULL AND repoid IN (SELECT repository FROM repository_permissions where userid=%s)"], - select={'perm': "SELECT CASE WHEN level>1 THEN 't'::boolean ELSE 'f'::boolean END FROM repository_permissions WHERE userid=%s AND repository_permissions.repository=repositories.repoid"}, - params=[request.user.username], select_params=[request.user.username]).order_by('name') + where=["remoterepository_id IS NULL AND repoid IN (SELECT repository FROM repository_permissions where user_id=%s)"], + select={'perm': "SELECT CASE WHEN level>1 THEN 't'::boolean ELSE 'f'::boolean END FROM repository_permissions WHERE user_id=%s AND repository_permissions.repository=repositories.repoid"}, + params=[request.user.id], select_params=[request.user.id]).order_by('name') return render(request, 'index.html', { 'repos': repos, }) @@ -62,17 +62,23 @@ def editrepo(request, repoid): repo.ValidateOwnerPermissions(request.user) form = None - formfactory = inlineformset_factory(Repository, RepositoryPermission, extra=1, fields=['userid', 'level']) + formfactory = inlineformset_factory( + Repository, + RepositoryPermission, + extra=0, + fields=['username', 'level'], + form=RepositoryPermissionForm, + ) if request.method == "POST": form = RepositoryForm(data=request.POST, instance=repo) formset = formfactory(data=request.POST, instance=repo) + addform = RepositoryPermissionsAddForm(repo=repo, data=request.POST) del form.fields['approved'] if repo.approved: del form.fields['initialclone'] - del form.fields['remoterepository'] - if form.is_valid() and formset.is_valid(): + if form.is_valid() and formset.is_valid() and addform.is_valid(): try: # Manually validate the repository entered if there is one to clone if 'initialclone' in form.cleaned_data and form.cleaned_data['initialclone']: @@ -94,25 +100,32 @@ def editrepo(request, repoid): form.save() formset.save() + if addform.cleaned_data['addemail']: + RepositoryPermission( + user=get_or_import_user(addform.cleaned_data['addemail']), + repository=repo, + level=addform.cleaned_data['addlevel'], + ).save() return HttpResponseRedirect("../../") except FormIsNotValid: # Just continue as if the form wasn't valid, expect the caller # to have set the required error fields pass - if not form or not form.errors: + else: form = RepositoryForm(instance=repo) del form.fields['approved'] if repo.approved: del form.fields['initialclone'] - del form.fields['remoterepository'] - formset = formfactory(instance=repo) + formset = formfactory(instance=repo) + addform = RepositoryPermissionsAddForm(repo=repo) perm = repo.repositorypermission_set.all() return render(request, 'repoview.html', { 'form': form, 'formset': formset, + 'addform': addform, 'repo': repo, 'repoperm': perm, }) @@ -150,7 +163,7 @@ def newrepo(request): repo = Repository(name=newname) repo.save() - perm = RepositoryPermission(userid=request.user.username, repository=repo, level=2) + perm = RepositoryPermission(user=request.user, repository=repo, level=2) perm.save() return HttpResponseRedirect('../repo/%s/' % repo.repoid) diff --git a/gitdump.py b/gitdump.py index b42bdb7..5b86d11 100644 --- a/gitdump.py +++ b/gitdump.py @@ -32,13 +32,13 @@ class AuthorizedKeysDumper(object): def dumpkeys(self): # FIXME: use a trigger to indicate if *anything at all* has changed curs = self.db.cursor() - curs.execute("SELECT userid,sshkey FROM git_users ORDER BY userid") + curs.execute("SELECT username,sshkey FROM git_users INNER JOIN auth_user on auth_user.id=git_users.user_id ORDER BY username") f = open("%s/.ssh/authorized_keys.tmp" % self.conf.get("paths", "githome"), "w") - for userid, sshkey in curs: + for username, sshkey in curs: for key in sshkey.split("\n"): key = key.strip() if key: - f.write("command=\"%s %s\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty %s\n" % (self.conf.get("paths", "pggit"), userid, key)) + f.write("command=\"%s %s\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty %s\n" % (self.conf.get("paths", "pggit"), username, key)) f.close() os.chmod("%s/.ssh/authorized_keys.tmp" % self.conf.get("paths", "githome"), 0o600) os.rename("%s/.ssh/authorized_keys.tmp" % self.conf.get("paths", "githome"), "%s/.ssh/authorized_keys" % self.conf.get("paths", "githome")) @@ -51,7 +51,7 @@ class AuthorizedKeysDumper(object): SELECT name,anonymous,web,description,initialclone,tabwidth, COALESCE( (SELECT min(first_name || ' ' || last_name) FROM repository_permissions AS rp - LEFT JOIN auth_user AS au ON au.username=rp.userid + LEFT JOIN auth_user AS au ON au.id=rp.user_id WHERE rp.level=2 AND rp.repository=r.repoid),''), CASE WHEN EXISTS (SELECT * FROM remoterepositories WHERE remoterepositories.id=r.remoterepository_id) diff --git a/keysync.py b/keysync.py index 62eb0e1..0c1db96 100644 --- a/keysync.py +++ b/keysync.py @@ -51,7 +51,7 @@ class KeySynchronizer(object): s = decryptor.decrypt(base64.b64decode(datas, "-_")).rstrip(b' ').decode('utf8') j = json.loads(s) for u in j: - curs.execute("INSERT INTO git_users (userid, sshkey) VALUES (%(userid)s, %(key)s) ON CONFLICT (userid) DO UPDATE SET sshkey=excluded.sshkey", { + curs.execute("INSERT INTO git_users (user_id, sshkey) SELECT id, %(key)s FROM auth_user WHERE username=%(userid)s ON CONFLICT (user_id) DO UPDATE SET sshkey=excluded.sshkey", { 'userid': u['u'], 'key': u['s'], }) diff --git a/pggit.py b/pggit.py index 68aad98..c0b2648 100755 --- a/pggit.py +++ b/pggit.py @@ -107,7 +107,7 @@ class PgGit(object): writeperm = False db = psycopg2.connect(self.cfg.get('database', 'db')) curs = db.cursor() - curs.execute("SELECT CASE WHEN remoterepository_id IS NULL THEN level ELSE 0 END FROM repository_permissions INNER JOIN repositories ON repoid=repository WHERE userid=%s AND name=%s", + curs.execute("SELECT CASE WHEN remoterepository_id IS NULL THEN level ELSE 0 END FROM repository_permissions INNER JOIN repositories ON repoid=repository INNER JOIN auth_user ON auth_user.id=user_id WHERE username=%s AND name=%s", (self.user, self.subpath)) try: -- 2.39.5