x/swiftonfile
Code Issues Proposed changes

gswauth: Fix 403 being returned instead of 401

Browse Source 
- 401(Unauthorized) is to be returned when user credentials are
  wrong where as 403(Forbidden) is to be returned when user
  credentials are correct but the user doesn't have the priveleges
  to carry out the operation.

- Also error messages displayed when using swauth-* command line
  utilities have been updated.

Change-Id: I485786896ad14d3263f4325d1857cacc93adab96
Signed-off-by: Prashanth Pai <ppai@redhat.com>
Reviewed-on: http://review.gluster.org/6336
Reviewed-by: Luis Pabon <lpabon@redhat.com>
Tested-by: Luis Pabon <lpabon@redhat.com>
Signed-off-by: Prashanth Pai <ppai@redhat.com>
Reviewed-on: http://review.gluster.org/6447
Reviewed-by: Thiago Da Silva <thiago@redhat.com>
Tested-by: Thiago Da Silva <thiago@redhat.com>
This commit is contained in:
Prashanth Pai 2013-11-22 12:13:09 +05:30 committed by Luis Pabon
parent b46b3dc7f2
commit a8c84cb2da
12 changed files with 146 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
gluster/swift/common/middleware/gswauth/bin/gswauth-add-account
View File

@ -66,4 +66,12 @@ if __name__ == '__main__':
ssl=(parsed.scheme == 'https'))
resp = conn.getresponse()
if resp.status // 100 != 2:
exit('Account creation failed: %s %s' % (resp.status, resp.reason))
if resp.status == 401:
exit('Account creation failed: %s %s: Invalid user/key provided' %
(resp.status, resp.reason))
elif resp.status == 403:
exit('Account creation failed: %s %s: Insufficient privileges' %
(resp.status, resp.reason))
else:
exit('Account creation failed: %s %s' %
(resp.status, resp.reason))

10
gluster/swift/common/middleware/gswauth/bin/gswauth-add-user
View File

@ -96,4 +96,12 @@ if __name__ == '__main__':
ssl=(parsed.scheme == 'https'))
resp = conn.getresponse()
if resp.status // 100 != 2:
exit('User creation failed: %s %s' % (resp.status, resp.reason))
if resp.status == 401:
exit('User creation failed: %s %s: Invalid user/key provided' %
(resp.status, resp.reason))
elif resp.status == 403:
exit('User creation failed: %s %s: Insufficient privileges' %
(resp.status, resp.reason))
else:
exit('User creation failed: %s %s' %
(resp.status, resp.reason))

2
gluster/swift/common/middleware/gswauth/bin/gswauth-cleanup-tokens
View File

@ -107,6 +107,8 @@ if __name__ == '__main__':
if e.http_status == 404:
exit('Container %s not found. gswauth-prep needs to be '
'rerun' % (container))
elif e.http_status == 401:
exit('Cleanup tokens failed: 401 Unauthorized: Invalid user/key provided')
else:
exit('Object listing on container %s failed with status '
'code %d' % (container, e.http_status))

15
gluster/swift/common/middleware/gswauth/bin/gswauth-delete-account
View File

@ -57,4 +57,17 @@ if __name__ == '__main__':
ssl=(parsed.scheme == 'https'))
resp = conn.getresponse()
if resp.status // 100 != 2:
exit('Account deletion failed: %s %s' % (resp.status, resp.reason))
if resp.status == 401:
exit('Delete account failed: %s %s: Invalid user/key provided' %
(resp.status, resp.reason))
elif resp.status == 403:
exit('Delete account failed: %s %s: Insufficient privileges' %
(resp.status, resp.reason))
elif resp.status == 404:
exit('Delete account failed: %s %s: Account %s does not exist' %
(resp.status, resp.reason, account))
elif resp.status == 409:
exit('Delete account failed: %s %s: Account %s contains active users. '
'Delete all users first.' % (resp.status, resp.reason, account))
else:
exit('Delete account failed: %s %s' % (resp.status, resp.reason))

12
gluster/swift/common/middleware/gswauth/bin/gswauth-delete-user
View File

@ -57,4 +57,14 @@ if __name__ == '__main__':
ssl=(parsed.scheme == 'https'))
resp = conn.getresponse()
if resp.status // 100 != 2:
exit('User deletion failed: %s %s' % (resp.status, resp.reason))
if resp.status == 401:
exit('Delete user failed: %s %s: Invalid user/key provided' %
(resp.status, resp.reason))
elif resp.status == 403:
exit('Delete user failed: %s %s: Insufficient privileges' %
(resp.status, resp.reason))
elif resp.status == 404:
exit('Delete user failed: %s %s: User %s does not exist' %
(resp.status, resp.reason, user))
else:
exit('Delete user failed: %s %s' % (resp.status, resp.reason))

9
gluster/swift/common/middleware/gswauth/bin/gswauth-list
View File

@ -82,7 +82,14 @@ If the [user] is '.groups', the active groups for the account will be listed.
resp = conn.getresponse()
body = resp.read()
if resp.status // 100 != 2:
exit('List failed: %s %s' % (resp.status, resp.reason))
if resp.status == 401:
exit('List failed: %s %s: Invalid user/key provided' %
(resp.status, resp.reason))
elif resp.status == 403:
exit('List failed: %s %s: Insufficient privileges' %
(resp.status, resp.reason))
else:
exit('List failed: %s %s' % (resp.status, resp.reason))
if options.plain_text:
info = json.loads(body)
for group in info[['accounts', 'users', 'groups'][len(args)]]:

7
gluster/swift/common/middleware/gswauth/bin/gswauth-prep
View File

@ -56,4 +56,9 @@ if __name__ == '__main__':
ssl=(parsed.scheme == 'https'))
resp = conn.getresponse()
if resp.status // 100 != 2:
exit('Auth subsystem prep failed: %s %s' % (resp.status, resp.reason))
if resp.status == 401:
exit('gswauth preparation failed: %s %s: Invalid user/key provided' %
(resp.status, resp.reason))
else:
exit('gswauth preparation failed: %s %s' %
(resp.status, resp.reason))

9
gluster/swift/common/middleware/gswauth/bin/gswauth-set-account-service
View File

@ -70,4 +70,11 @@ Example: %prog -K gswauthkey test storage local http://127.0.0.1:8080/v1/AUTH_01
conn.send(body)
resp = conn.getresponse()
if resp.status // 100 != 2:
exit('Service set failed: %s %s' % (resp.status, resp.reason))
if resp.status == 401:
exit('Service set failed: %s %s: Invalid user/key provided' %
(resp.status, resp.reason))
elif resp.status == 403:
exit('Service set failed: %s %s: Insufficient privileges' %
(resp.status, resp.reason))
else:
exit('Service set failed: %s %s' % (resp.status, resp.reason))

42
gluster/swift/common/middleware/gswauth/swauth/middleware.py
View File

@ -419,7 +419,9 @@ class Swauth(object):
Returns a standard WSGI response callable with the status of 403 or 401
depending on whether the REMOTE_USER is set or not.
"""
if req.remote_user:
if not hasattr(req, 'credentials_valid'):
req.credentials_valid = None
if req.remote_user or req.credentials_valid:
return HTTPForbidden(request=req)
else:
return HTTPUnauthorized(request=req)
@ -534,7 +536,7 @@ class Swauth(object):
:returns: swob.Response, 204 on success
"""
if not self.is_super_admin(req):
return HTTPForbidden(request=req)
return HTTPUnauthorized(request=req)
path = quote('/v1/%s/.account_id' % self.auth_account)
resp = self.make_pre_authed_request(
req.environ, 'PUT', path).get_response(self.app)
@ -568,7 +570,7 @@ class Swauth(object):
explained above.
"""
if not self.is_reseller_admin(req):
return HTTPForbidden(request=req)
return self.denied_response(req)
listing = []
marker = ''
while True:
@ -613,7 +615,7 @@ class Swauth(object):
if req.path_info or not account or account[0] == '.':
return HTTPBadRequest(request=req)
if not self.is_account_admin(req, account):
return HTTPForbidden(request=req)
return self.denied_response(req)
path = quote('/v1/%s/%s/.services' % (self.auth_account, account))
resp = self.make_pre_authed_request(
req.environ, 'GET', path).get_response(self.app)
@ -685,7 +687,7 @@ class Swauth(object):
dict as described above
"""
if not self.is_reseller_admin(req):
return HTTPForbidden(request=req)
return self.denied_response(req)
account = req.path_info_pop()
if req.path_info != '/.services' or not account or account[0] == '.':
return HTTPBadRequest(request=req)
@ -731,7 +733,7 @@ class Swauth(object):
:returns: swob.Response, 2xx on success.
"""
if not self.is_reseller_admin(req):
return HTTPForbidden(request=req)
return self.denied_response(req)
account = req.path_info_pop()
if req.path_info or not account or account[0] == '.':
return HTTPBadRequest(request=req)
@ -798,7 +800,7 @@ class Swauth(object):
:returns: swob.Response, 2xx on success.
"""
if not self.is_reseller_admin(req):
return HTTPForbidden(request=req)
return self.denied_response(req)
account = req.path_info_pop()
if req.path_info or not account or account[0] == '.':
return HTTPBadRequest(request=req)
@ -905,7 +907,7 @@ class Swauth(object):
(user[0] == '.' and user != '.groups'):
return HTTPBadRequest(request=req)
if not self.is_account_admin(req, account):
return HTTPForbidden(request=req)
return self.denied_response(req)
if user == '.groups':
# TODO: This could be very slow for accounts with a really large
# number of users. Speed could be improved by concurrently
@ -990,9 +992,9 @@ class Swauth(object):
return HTTPBadRequest(request=req)
if reseller_admin:
if not self.is_super_admin(req):
return HTTPForbidden(request=req)
return HTTPUnauthorized(request=req)
elif not self.is_account_admin(req, account):
return HTTPForbidden(request=req)
return self.denied_response(req)
path = quote('/v1/%s/%s' % (self.auth_account, account))
resp = self.make_pre_authed_request(
@ -1040,7 +1042,7 @@ class Swauth(object):
user[0] == '.':
return HTTPBadRequest(request=req)
if not self.is_account_admin(req, account):
return HTTPForbidden(request=req)
return self.denied_response(req)
# Delete the user's existing token, if any.
path = quote('/v1/%s/%s/%s' % (self.auth_account, account, user))
resp = self.make_pre_authed_request(
@ -1418,12 +1420,20 @@ class Swauth(object):
Returns True if the admin specified in the request represents a
.reseller_admin.
The variable req.credentials_valid is set to True if the credentials
match. This is used to distinguish between HTTPUnauthorized and
HTTPForbidden cases in denied_response method. HTTPUnauthorized is
returned when the credentials(username and key) do not match. A
HTTPForbidden is returned when the credentials match, but the user does
not have necessary permission to perform the requested action.
:param req: The swob.Request to check.
:param admin_detail: The previously retrieved dict from
:func:`get_admin_detail` or None for this function
to retrieve the admin_detail itself.
:param returns: True if .reseller_admin.
"""
req.credentials_valid = False
if self.is_super_admin(req):
return True
if not admin_detail:
@ -1431,6 +1441,7 @@ class Swauth(object):
if not self.credentials_match(admin_detail,
req.headers.get('x-auth-admin-key')):
return False
req.credentials_valid = True
return '.reseller_admin' in (g['name'] for g in admin_detail['groups'])
def is_account_admin(self, req, account):
@ -1438,10 +1449,18 @@ class Swauth(object):
Returns True if the admin specified in the request represents a .admin
for the account specified.
The variable req.credentials_valid is set to True if the credentials
match. This is used to distinguish between HTTPUnauthorized and
HTTPForbidden cases in denied_response method. HTTPUnauthorized is
returned when the credentials(username and key) do not match. A
HTTPForbidden is returned when the credentials match, but the user does
not have necessary permission to perform the requested action.
:param req: The swob.Request to check.
:param account: The account to check for .admin against.
:param returns: True if .admin.
"""
req.credentials_valid = False
if self.is_super_admin(req):
return True
admin_detail = self.get_admin_detail(req)
@ -1451,6 +1470,7 @@ class Swauth(object):
if not self.credentials_match(admin_detail,
req.headers.get('x-auth-admin-key')):
return False
req.credentials_valid = True
return admin_detail and admin_detail['account'] == account and \
'.admin' in (g['name'] for g in admin_detail['groups'])
return False

4
test/functional_auth/gswauth/test_gswauth.py
View File

@ -159,7 +159,7 @@ class TestGSWauth(unittest.TestCase):
conn = http_connect(config['auth_host'], config['auth_port'], 'PUT',
path, headers)
resp = conn.getresponse()
self.assertTrue(resp.status == 403)
self.assertTrue(resp.status == 401)
def test_change_user_password(self):
# check and register account
@ -235,7 +235,7 @@ class TestGSWauth(unittest.TestCase):
conn = http_connect(config['auth_host'], config['auth_port'], 'PUT',
path, headers)
resp = conn.getresponse()
self.assertTrue(resp.status == 403)
self.assertTrue(resp.status == 401)
finally:
try:

42
test/functional_auth/gswauth/test_gswauth_cli.py
View File

@ -81,8 +81,8 @@ class TestSwauthPrep(unittest.TestCase):
(status,output)=Utils.swauthPrep(key='notavalidkey')
self.assertNotEqual(status, 0, 'Invalid swauth-prep request accepted(wrong key provided):'+output)
#TODO:In place of this error message 'Auth subsystem prep failed: 403 Forbidden, Invalid user/key' would be good to have
self.assertEqual('Auth subsystem prep failed: 403 Forbidden' in output,True, 'Invalid swauth-prep request accepted: '+output)
self.assertEqual('gswauth preparation failed: 401 Unauthorized: Invalid user/key provided' in output,True, 'Invalid\
swauth-prep request accepted: '+output)
#TODO:More cases for invalid url and admin user
@ -128,7 +128,11 @@ class TestAccount(unittest.TestCase):
(status,output)=Utils.addAccount('testinvalidkey',key='invalidkey')
#self.assertEqual(status, 0, 'account creation failed std err was: '+output)
#assert for better error message 403 Forbidden, Invalid user/key would be good to have
self.assertEqual('403 Forbidden' in output,True, 'Invalid account creation request accepted: '+output)
self.assertEqual('Account creation failed: 401 Unauthorized: Invalid user/key provided' in output,True, 'Invalid account creation request accepted: '+output)
(status,output) = Utils.addUser('test','tester','testing')
(status,output)=Utils.addAccount('test2',user='test:tester',key='testing')
self.assertEqual('Account creation failed: 403 Forbidden: Insufficient privileges' in output,True, 'Invalid account creation request accepted: '+output)
#TODO:more cases?
def testDeleteAccount(self):
@ -141,8 +145,8 @@ class TestAccount(unittest.TestCase):
#Invalid request to delete an account with users
(status,output)=Utils.deleteAccount('test2')
self.assertNotEqual(status, 0, 'account deletion failed for test2 account'+output)
#TODO:decide on expected behavior 'there are active users,users needs to be deleted first'?
self.assertEqual('Conflict' in output,True, 'account deletion failed for test account'+output)
self.assertEqual('Delete account failed: 409 Conflict: Account test2 contains active users. Delete all users first.' in output,True,
'account deletion failed for test account'+output)
#delete all users in above account and then try again
(status,output) = Utils.deleteUser('test2','tester')
@ -154,12 +158,20 @@ class TestAccount(unittest.TestCase):
(status,output) = Utils.deleteUser('test2','tester3')
self.assertEqual(status, 0, 'setTestDeleteAccountEnv'+output)
(status,output) = Utils.addUser('test','tester','testing')
(status,output) = Utils.deleteAccount('test2',user='test:tester',key='testing')
self.assertEqual('Delete account failed: 403 Forbidden: Insufficient privileges' in output,True, 'account deletion failed for test2 account'+output)
(status,output) = Utils.deleteAccount('test2',key='invalidkey')
self.assertEqual('Delete account failed: 401 Unauthorized: Invalid user/key provided' in output,True, 'account deletion failed for test2 account'+output)
(status,output)=Utils.deleteAccount('test2')
self.assertEqual(status, 0, 'account deletion failed for test2 account'+output)
(status,output)=Utils.deleteAccount('accountdoesnotexist')
#TODO:decide on expected behavior
self.assertNotEqual(status, 0, 'account deletion failed for accountdoesnotexist'+output)
self.assertEqual('Delete account failed: 404 Not Found: Account accountdoesnotexist does not exist' in output,True, 'account deletion failed for test\
account'+output)
#TODO:more cases
def testListAcounts(self):
@ -225,12 +237,24 @@ class TestUser(unittest.TestCase):
(status,output) = Utils.addAdminUser('accountdoesnotexist', 'testcli', 'testcli')
#TODO: decide on behavior,below is just place holder, right now it accepts this request and create both user and account
self.assertEqual(status, 0, 'Invalid user creation request accepted,accountdoesnotexist: '+output)
(status,output) = Utils.addUser('test','testuser2','testuser2',user='test:testuser',key='testuser')
self.assertEqual('User creation failed: 403 Forbidden: Insufficient privileges' in output, True, 'user addition failed'+output)
(status,output) = Utils.addUser('test','testuser2','testuser2',user='test:testadminuser',key='invalidkey')
self.assertEqual('User creation failed: 401 Unauthorized: Invalid user/key provided' in output, True, 'user addition failed'+output)
#TODO: more test cases?
def testDeleteUser(self):
#set the env for test
self.setTestDeleteUserEnv()
(status,output) = Utils.deleteUser('test','testadminuser',user='test:testuser',key='testuser')
self.assertEqual('Delete user failed: 403 Forbidden: Insufficient privileges' in output, True, 'user deletion failed'+output)
(status,output) = Utils.deleteUser('test','testuser',key='invalidkey')
self.assertEqual('Delete user failed: 401 Unauthorized: Invalid user/key provided' in output, True, 'user deletion failed'+output)
(status,output) = Utils.deleteUser('test','testadminuser')
self.assertEqual(status, 0, 'valid user deletion failed:'+output)
@ -247,12 +271,10 @@ class TestUser(unittest.TestCase):
self.assertEqual('Usage:' in output, True, 'Invalid user deletion request accepted : '+output)
(status,output) = Utils.deleteUser('test', 'userdoesnotexist')
self.assertNotEqual(status, 0, 'Invalid user deletion request accepted,userdoesnotexist:'+output)
#TODO:decide on expected behavior,current is '404 Not Found'
self.assertEqual('Delete user failed: 404 Not Found: User userdoesnotexist does not exist' in output, True, 'user deletion failed'+output)
(status,output) = Utils.deleteUser('accountisnothere', 'testcli')
self.assertNotEqual(status, 0, 'Invalid user deletion request accepted, accountdoesnotexist:'+output)
#TODO:decide on expected behavior,current is '404 Not Found'
self.assertEqual('Delete user failed: 404 Not Found: User testcli does not exist' in output, True, 'user deletion failed'+output)
#TODO:more testcases?

28
test/unit/common/middleware/gswauth/swauth/test_middleware.py
View File

@ -1287,7 +1287,7 @@ class TestAuth(unittest.TestCase):
'super_admin',
'X-Auth-Admin-Key': 'supertest'}
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
resp = Request.blank('/auth/v2/.prep',
environ={
'REQUEST_METHOD': 'POST'},
@ -1296,25 +1296,25 @@ class TestAuth(unittest.TestCase):
'.super_admin',
'X-Auth-Admin-Key': 'upertest'}
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
resp = Request.blank('/auth/v2/.prep',
environ={
'REQUEST_METHOD': 'POST'},
headers={
'X-Auth-Admin-User': '.super_admin'}
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
resp = Request.blank('/auth/v2/.prep',
environ={
'REQUEST_METHOD': 'POST'},
headers={
'X-Auth-Admin-Key': 'supertest'}
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
resp = Request.blank(
'/auth/v2/.prep',
environ={'REQUEST_METHOD': 'POST'}).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
def test_prep_fail_account_create(self):
self.test_auth.app = FakeApp(iter([
@ -1424,7 +1424,7 @@ class TestAuth(unittest.TestCase):
'super:admin',
'X-Auth-Admin-Key': 'supertest'}
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
self.assertEquals(self.test_auth.app.calls, 1)
self.test_auth.app = FakeApp(iter([
@ -1598,7 +1598,7 @@ class TestAuth(unittest.TestCase):
'super:admin',
'X-Auth-Admin-Key': 'supertest'}
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
self.assertEquals(self.test_auth.app.calls, 1)
self.test_auth.app = FakeApp(iter([
@ -1813,7 +1813,7 @@ class TestAuth(unittest.TestCase):
body=json.dumps(
{'storage': {'local': 'new_value'}})
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
self.assertEquals(self.test_auth.app.calls, 1)
self.test_auth.app = FakeApp(iter([
@ -2046,7 +2046,7 @@ class TestAuth(unittest.TestCase):
headers={'X-Auth-Admin-User': 'super:admin',
'X-Auth-Admin-Key': 'supertest'},).get_response(
self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
self.assertEquals(self.test_auth.app.calls, 1)
self.test_auth.app = FakeApp(iter([
@ -2391,7 +2391,7 @@ class TestAuth(unittest.TestCase):
'super:admin',
'X-Auth-Admin-Key': 'supertest'},
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
self.assertEquals(self.test_auth.app.calls, 1)
self.test_auth.app = FakeApp(iter([
@ -2990,7 +2990,7 @@ class TestAuth(unittest.TestCase):
'super:admin',
'X-Auth-Admin-Key': 'supertest'},
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
self.assertEquals(self.test_auth.app.calls, 1)
self.test_auth.app = FakeApp(iter([
@ -3260,7 +3260,7 @@ class TestAuth(unittest.TestCase):
'key',
'X-Auth-User-Reseller-Admin': 'true'}
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
self.assertEquals(self.test_auth.app.calls, 0)
self.test_auth.app = FakeApp(iter([
@ -3282,7 +3282,7 @@ class TestAuth(unittest.TestCase):
'key',
'X-Auth-User-Reseller-Admin': 'true'}
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
self.assertEquals(self.test_auth.app.calls, 0)
self.test_auth.app = FakeApp(iter([
@ -3303,7 +3303,7 @@ class TestAuth(unittest.TestCase):
'key',
'X-Auth-User-Reseller-Admin': 'true'}
).get_response(self.test_auth)
self.assertEquals(resp.status_int, 403)
self.assertEquals(resp.status_int, 401)
self.assertEquals(self.test_auth.app.calls, 0)
def test_put_user_account_admin_fail_bad_creds(self):