Superadmin Implementation

[BrandonCookeDev]

This PR implements:

• Superadmin vs Region Admin privileges
• New UI on the Admin Functions that shows every user some information and allows them to change password

[BrandonCookeDev]

I still need to implement a unit test or two before we PR this, but the functionality is there.

[BrandonCookeDev]

Tests written and this PR is ready to be reviewed

[jschnei]

just tested it out a bunch on stage -- looks good!

mostly minor things. the biggest thing is whether we actually want to use an Enum for the admin_levels (and if we do, we should be consistent and use the enum constants everywhere instead of string constants).

in addition to these review comments, i'm also having issues as a superadmin editing default rating criteria for a region that's not in my regions list... probably another check is needed there for superadmins?

[jschnei]

since these guys should come from an Enum, probably it is a good idea to add a choices validator so the ORM will automatically check for this. (take a look at how tournament source types work on line 129).

[BrandonCookeDev]

So I've gone with the String Only method, but I'm not really sure how to validate on the ORM. can you point me in the right direction?

[jschnei]

so, take a look at line 129 to see how it's done for tournament source_types (challonge, tio, smashgg).

basically, you should do something like

              ('admin_level', orm.StringField(
                  required=True,
                  validators=[orm.validate_choices(ADMIN_LEVEL_CHOICES)]))

and define a list/tuple ADMIN_LEVEL_CHOICES somewhere.

[jschnei]

probably makes most sense to move this up to the top, next to the SOURCE_TYPE_CHOICES "enum".

For that matter, I'm still not sure if we gain anything by making this an explicit enum instead of just writing a tuple of choices (as in SOURCE_TYPE_CHOICES) and using the validator. It would make more sense I feel if we were mapping human-readable enum values to non-human readable int values (this is what a lot of enums are for), but since we are mapping them to string values, it feels a bit redundant (also, in server.py, it looks like we actually just use the explicit string constants).

Anyway tl;dr either way (enums/lists of strings) is okay, but if we decide to go the Enum route we should probably also convert SOURCE_TYPE_CHOICES to an Enum for consistency.

[BrandonCookeDev]

Going with the string method so no need for this

[jschnei]

if we want to stick with the Enum method, these 'SUPER's and everything should be M.AdminLevels.SUPERs.

[BrandonCookeDev]

Going with the String method. so no need to change this

[jschnei]

hmm if we have the user object, can't we just check if user.admin_level=='SUPER'? This should save a mongo lookup.

[jschnei]

let's remove this comment, it's no longer true with the introduction of the admin dashboard.

[jschnei]

we should try not to repeat code (this is basically part of check_creds_and_get_session_id_or_none). maybe let's make check_creds return a user object, and refactor check_creds_and_get_session_id_or_none to call check_creds first?

(this could also be postponed to a later PR, but we should make a note).

[BrandonCookeDev]

OK so. This one is interesting. I tried making its own function via the following:

def get_and_verify_user_by_username(self, username):
        result = self.users_col.find({"username": username})
        if result.count() == 0:
            return None
        assert result.count() == 1, "WE HAVE DUPLICATE USERNAMES IN THE DB"
        user = M.User.load(result[0], context='db')
        assert user, "mongo has stopped being consistent, abort ship"
        return user

However, when we run this function, I get problems with this function:

def check_creds_and_get_session_id_or_none(self, username, password):
        user = self.get_and_verify_user_by_username(username)

        # timing oracle on this... good luck
        if verify_password(password, user.salt, user.hashed_password):
            session_id = base64.b64encode(os.urandom(128))
            self.update_session_id_for_user(user.id, session_id)
            return session_id
        else:
            return None

The error I get is that it can't get user.salt for NoneType

[BrandonCookeDev]

Oh looking a little closer it might be because it finds no user in the db and then returns None...

[jschnei]

i'm confused, it looks there are now 3 functions which repeat a lot of code?:

1. check_creds(username, password) which returns whether (username, password) are valid credentials.
2. check_creds_and_get_session_id_or_none(username, password), which checks if (username, password) are valid credentials, and tries to return the current session
3. get_and_verify_user_by_username, which seems to be a duplicate of get_user_by_username.

Right now 1 and 2 duplicate a lot of the same code. I recommend removing get_and_verify_user_by_username and refactoring 1 and 2 so that check_creds returns a user object (not just a bool), and so that get_session_id calls check_creds to get the user object.

[jschnei]

this can be used for any admin_level, so let's call it something like set_user_admin_level or update_user_admin_level? (it might also make more sense to just make a general update_user function in the same vein as update_player and update_tournament).

[jschnei]

for consistency, let's make the values of the permissions on the frontend match the string values of the enums on the backends (i.e. 'SUPER' and 'REGION')

[jschnei]

can we put value='REGION' and value='SUPER' here to pass 'REGION' and 'SUPER' to the backend? (not a big deal, but it's nice when values are consistent between frontend and backend)

[jschnei]

I think M.AdminLevels does not exist anymore, so this should error (is it being tested)?
In any case, the ORM should take care of this check.

[jschnei]

I think it is probably better to use get_user_by_id_or_none here instead of rewriting the Mongo queries (so something like user = get_user_by_id_or_none(user_id).

Actually, do we use this function (get_is_super_admin) anywhere else in the code? If not, I think it's fine to remove this function (it is easy enough to just check whether user.admin_level=='SUPER' whenever we have a user object).

[jschnei]

do we actually use this function anywhere else in the code yet? if not, I think it would be fine to just replace it with a simple update_user function, in the same style as update_player and update_region.

[jschnei]

right now this way of updating bypasses the ORM completely and won't trigger any of the ORMs validators. the correct way to do this sort of thing is 1. read the user data into a User object (as is done in get_user_by_id_or_none), 2. update whatever properties of the User object, 3. then write the User object back into the database using self.users_col.update({'_id': user.id}, user.dump(context='db')).

[jschnei]

i'm confused, it looks there are now 3 functions which repeat a lot of code?:

1. check_creds(username, password) which returns whether (username, password) are valid credentials.
2. check_creds_and_get_session_id_or_none(username, password), which checks if (username, password) are valid credentials, and tries to return the current session
3. get_and_verify_user_by_username, which seems to be a duplicate of get_user_by_username.

Right now 1 and 2 duplicate a lot of the same code. I recommend removing get_and_verify_user_by_username and refactoring 1 and 2 so that check_creds returns a user object (not just a bool), and so that get_session_id calls check_creds to get the user object.

[BrandonCookeDev]

Jon, I made the changes. Notably I deleted the third check_creds thing I had written. I left it in to try to fix whatever bug was in it, but now I have no time.
Also I rewrote the set_admin_level function, and commented it out. Since it isn't implemented yet, and we may want it in the future, I felt this was the best course of action

Additionally, the third function for check_creds (that seemingly duplicate code) I wrote to try to get the user and return that data (the first half of both check_creds functions). However, it wasn't working properly. So I left it in there to fix another day. But instead I just deleted it in the last commit. We can do a refactor on it later; right now I'm just lacking free time.

[jschnei]

I fixed a bug with creating new users; I think it works now.

Also, while fixing this I noticed some of the frontend code is probably less angular-y than it should be; there's a lot of places where you're manipulating the DOM directly (I think the angular philosophy is that this should basically always be done through angular directives). Fixing this can probably wait for a later PR though; there is a bunch of frontend refactoring that still needs to be done.