Allauth: User settings & GitHub linking

[gdude2002]

This is a tracking issue, just for an overview of the functionality needed and a note that I'm working on it.

Now that Allauth is implemented and users can login with Discord, we need to provide a way for users to manage their accounts. The proposal is simple: A user settings page.

This page should include the following, ideally:

• A method for linking/unlinking a GitHub account
  • Particular attention needs to be given here to prevent users from actually signing up with a GitHub account
• An overview of the user's roles groups and any other debugging data
• Some way to remove their Django user account (although we need to hold on to stuff like infractions)
• Let's stick a cog to the side of the logout option, if we can.
• Prevent signups from users that aren't Developers and on the server

[scragly]

A possible idea for having is_staff applied correctly is to apply it at user_logged_in and adding model methods to Role and User:

Role

class Role(ModelReprMixin, models.Model):
    ...
    staff_roles = ("Owners", "Admins", "Moderators")
    ...
    @property
    def is_staff(self):
        return self.name in self.staff_roles

User

class User(ModelReprMixin, models.Model):
    ...
    @property
    def is_staff(self):
        return any(r.is_staff for r in self.roles.all())

AllauthSignalListener

    def user_logged_in(self, sender: Type[DjangoUser], **kwargs) -> None:
        ...
        if discord_user.is_staff and not user.is_staff:
            user.is_staff = True
            user.save()
        elif not discord_user.is_staff and user.is_staff:
            user.is_staff = False
            user.save()

        self._apply_groups(discord_user, account)

[scragly]

This section in signals.py needs to be dedented:

site/pydis_site/apps/home/signals.py

Lines 254 to 260 in 03d5a4c

	account.user.groups.add(
	*[group for group in new_groups if group not in current_groups]
	)
	account.user.groups.remove(
	*[mapping.group for mapping in mappings if mapping.group not in new_groups]
	)

[scragly]

While SocialAccounts are identified as unique by their provider connection UUID, the Auth User that's mapped to the user attribute of it is the actual login user with the groups, permissions, etc, and is limited to username being unique. Due to this, we will need to consider ensuring either the username#disc is assigned instead, or the discord user id, as usernames in discord are not unique and will cause issues when a second person logs in with the same username, potentially granting a staff permissions to a standard member simply by having the same name.

[gdude2002]

Thanks for the responses!

A possible idea for having is_staff applied correctly is to apply it at user_logged_in and adding model methods to Role and User...

I quite like this approach, but I think for the sake of keeping things configurable, roles should be able to have is_staff toggled in the Django Admin.

While SocialAccounts are identified as unique by their provider connection UUID, the Auth User that's mapped to the user attribute of it is the actual login user with the groups, permissions, etc, and is limited to username being unique...

This does need to be fixed, but it's worth noting that SocialAccounts don't key Django Users by username - the primary key is an ID, so there isn't a security issue in the current implementation.

[scragly]

SocialAccounts don't key Django Users by username - the primary key is an ID

Security issue then aside, since we're clear that won't occur thankfully, the username is a unique constraint still so yeah, I guess it'll just raise an exception and fail to create an auth user entirely. Wonder how that looks when it breaks.

[scragly]

roles should be able to have is_staff toggled in the Django Admin.

Sounds like a good idea to me. Just means it'll have to be added as a new column, but I'm sure that's fine.

[gdude2002]

Wonder how that looks when it breaks.

Allauth doesn't seem to have any handling of it, so I suspect users would be provided with a nasty error 500 page.

Just means it'll have to be added as a new column, but I'm sure that's fine.

🖖

[gdude2002]

OK, as far as I can see, all of the extra stuff not specifically in my TODO list in the first comment should now be present in the PR.