Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Superadmin Implementation #168

Merged
merged 18 commits into from
Jan 27, 2017
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 40 additions & 4 deletions dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -557,9 +557,7 @@ def insert_user(self, user):

return self.users_col.insert(user.dump(context='db'))

# throws invalidRegionsException, which is okay, as this is only used by a
# script
def create_user(self, username, password, regions):
def create_user(self, username, password, regions, perm='REGION'):
valid_regions = [
region.id for region in Dao.get_all_regions(self.mongo_client)]

Expand All @@ -576,7 +574,8 @@ def create_user(self, username, password, regions):
admin_regions=regions,
username=username,
salt=salt,
hashed_password=hashed_password)
hashed_password=hashed_password,
admin_level=perm)

return self.insert_user(the_user)

Expand Down Expand Up @@ -619,6 +618,25 @@ def get_user_by_session_id_or_none(self, session_id):
def get_user_by_region(self, regions):
pass

def get_is_superadmin(self, user_id):
user = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if self.users_col.find_one({'_id': user_id}):
user = M.User.load(self.users_col.find_one({'_id': user_id}))
return user.admin_level == 'SUPER'
else:
return False

def set_user_admin_level(self, user_id, admin_level):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if type(admin_level) is not M.AdminLevels:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

raise Exception('Submitted admin level is not of correct type')
if self.users_col.find_one({'_id': user_id}):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')).

self.user_col.update({'_id': user_id},
{'$set':
{
'admin_level': admin_level.name
}
})

#### FOR INTERNAL USE ONLY ####
#XXX: this method must NEVER be publicly routeable, or you have session-hijacking
def get_session_id_by_user_or_none(self, User):
Expand All @@ -645,6 +663,24 @@ def check_creds_and_get_session_id_or_none(self, username, password):
else:
return None

def check_creds(self, username, password):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 verify_password(password, user.salt, user.hashed_password)

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

def update_session_id_for_user(self, user_id, session_id):
# lets force people to have only one session at a time
self.sessions_col.remove({"user_id": user_id})
Expand Down
7 changes: 5 additions & 2 deletions model.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import orm

SOURCE_TYPE_CHOICES = ('tio', 'challonge', 'smashgg', 'other')

ADMIN_LEVEL_CHOICES = ('REGION', 'SUPER')
# Embedded documents

class AliasMapping(orm.Document):
Expand Down Expand Up @@ -348,7 +348,10 @@ class User(orm.Document):
('username', orm.StringField(required=True)),
('salt', orm.StringField(required=True)),
('hashed_password', orm.StringField(required=True)),
('admin_regions', orm.ListField(orm.StringField()))]
('admin_regions', orm.ListField(orm.StringField())),
('admin_level', orm.StringField(required=True, default='REGION',
validators=[orm.validate_choices(ADMIN_LEVEL_CHOICES)])
)]


class Merge(orm.Document):
Expand Down
44 changes: 40 additions & 4 deletions server.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
TYPEAHEAD_PLAYER_LIMIT = 20
BASE_REGION = 'newjersey'


# parse config file
config = Config()

Expand Down Expand Up @@ -118,6 +119,10 @@
admin_functions_parser.add_argument('new_user_permissions', location='json', type=str)
admin_functions_parser.add_argument('new_user_regions', location='json', type=list)

user_parser = reqparse.RequestParser()
user_parser.add_argument('old_pass', location='json', type=str)
user_parser.add_argument('new_pass', location='json', type=str)

#TODO: major refactor to move auth code to a decorator


Expand All @@ -137,6 +142,8 @@ def get_user_from_request(request, dao):


def is_user_admin_for_region(user, region):
if user.admin_level == 'SUPER':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@BrandonCookeDev BrandonCookeDev Jan 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return True
if not region:
return False
if not user.admin_regions:
Expand All @@ -150,7 +157,9 @@ def is_user_admin_for_regions(user, regions):
'''
returns true is user is an admin for ANY of the regions
'''
if len(set(regions).intersection(user.admin_regions)) == 0:
if user.admin_level == 'SUPER':
return True
elif len(set(regions).intersection(user.admin_regions)) == 0:
return False
else:
return True
Expand Down Expand Up @@ -1163,6 +1172,28 @@ def get(self):

return return_dict

class UserResource(restful.Resource):
def put(self):
dao = Dao(None, mongo_client=mongo_client)

if not dao:
return 'Dao not found', 404
user = get_user_from_request(request, dao)
if not user:
return 'Permission denied', 403

args = user_parser.parse_args()
old_pass = args['old_pass']
new_pass = args['new_pass']

try:
if dao.check_creds(user.username, old_pass):
dao.change_passwd(user.username, new_pass)
return 200
else: return 'Bad password', 403
except Exception as ex:
return 'Password change not successful', 400


class AdminFunctionsResource(restful.Resource):
def get(self):
Expand All @@ -1176,8 +1207,8 @@ def put(self):
user = get_user_from_request(request, dao)
if not user:
return 'Permission denied', 403
#if not is_user_admin_for_region(user, region='*'):
# return 'Permission denied', 403
if not user.admin_level == 'SUPER':
return 'Permission denied', 403

args = admin_functions_parser.parse_args()

Expand All @@ -1196,9 +1227,12 @@ def put(self):
uperm = args['new_user_permissions']
uregions = args['new_user_regions']

perm = None
if uperm is not 'REGION' and uperm is not 'SUPER': return 'Invalid permission selection!', 403

#Execute user addition
dao = Dao(None, mongo_client)
if dao.create_user(uname, upass, uregions):
if dao.create_user(uname, upass, uregions, uperm):
print("user created:" + uname)

@api.representation('text/plain')
Expand Down Expand Up @@ -1273,6 +1307,8 @@ def add_cors(resp):

api.add_resource(SessionResource, '/users/session')

api.add_resource(UserResource, '/user')

api.add_resource(LoaderIOTokenResource,
'/{}/'.format(config.get_loaderio_token()))

Expand Down
39 changes: 33 additions & 6 deletions test/test_dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,8 @@ def setUp(self):
admin_regions=self.user_admin_regions_1,
username='user1',
salt='nacl',
hashed_password='browns')
hashed_password='browns',
admin_level='REGION')

self.user_id_2 = 'asdfasdf'
self.user_full_name_2 = 'Full Name'
Expand All @@ -202,9 +203,22 @@ def setUp(self):
admin_regions=self.user_admin_regions_2,
username=self.user_full_name_2,
salt='nacl',
hashed_password='browns')

self.users = [self.user_1, self.user_2]
hashed_password='browns',
admin_level='REGION')

self.superadmin_id = '123456'
self.superadmin_full_name = 'superadmin'
self.superadmin_admin_regions = []
self.superadmin = User(
id=self.superadmin_id,
user_admin_regions=self.superadmin_admin_regions,
username=self.superadmin_full_name,
salt='nacl',
hashed_password='browns',
admin_level='SUPER'
)

self.users = [self.user_1, self.user_2, self.superadmin]

self.region_1 = Region(id='norcal', display_name='Norcal')
self.region_2 = Region(id='texas', display_name='Texas')
Expand Down Expand Up @@ -233,6 +247,8 @@ def setUp(self):
for user in self.users:
self.norcal_dao.insert_user(user)



def tearDown(self):
self.mongo_client.drop_database(DATABASE_NAME)

Expand Down Expand Up @@ -785,7 +801,7 @@ def test_get_latest_ranking(self):

def test_get_all_users(self):
users = self.norcal_dao.get_all_users()
self.assertEquals(len(users), 2)
self.assertEquals(len(users), 3)

user = users[0]
self.assertEquals(user.id, self.user_id_1)
Expand All @@ -797,6 +813,11 @@ def test_get_all_users(self):
self.assertEquals(user.username, self.user_full_name_2)
self.assertEquals(user.admin_regions, self.user_admin_regions_2)

user = users[2]
self.assertEqual(user.id, self.superadmin_id)
self.assertEqual(user.username, self.superadmin_full_name)
self.assertEqual(user.admin_regions, self.superadmin_admin_regions)

def test_create_user(self):
username = 'abra'
password = 'cadabra'
Expand All @@ -805,7 +826,7 @@ def test_create_user(self):
self.norcal_dao.create_user(username, password, regions)

users = self.norcal_dao.get_all_users()
self.assertEquals(len(users), 3)
self.assertEquals(len(users), 4)

user = users[-1]
self.assertEquals(user.username, username)
Expand Down Expand Up @@ -938,3 +959,9 @@ def test_get_nonexistent_merge(self):
dao = self.norcal_dao
self.assertIsNone(dao.get_merge(
ObjectId("420f53650181b84aaaa01051"))) # mlg1337noscope

def test_get_is_superadmin(self):
dao = self.norcal_dao
self.assertEqual(dao.get_is_superadmin(self.user_1.id), False)
self.assertEqual(dao.get_is_superadmin(self.user_2.id), False)
self.assertEqual(dao.get_is_superadmin(self.superadmin.id), True)
7 changes: 5 additions & 2 deletions test/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -610,18 +610,21 @@ def setUp(self):
self.username = 'ASDF fdsa'
self.salt = 'nacl'
self.hashed_password = 'browns'
self.admin_level = 'REGION'
self.user = User(id=self.id,
admin_regions=self.admin_regions,
username=self.username,
salt=self.salt,
hashed_password=self.hashed_password)
hashed_password=self.hashed_password,
admin_level=self.admin_level)

self.user_json_dict = {
'_id': self.id,
'username': self.username,
'admin_regions': self.admin_regions,
'salt': self.salt,
'hashed_password': self.hashed_password
'hashed_password': self.hashed_password,
'admin_level': self.admin_level
}

def test_dump(self):
Expand Down
Loading