gswauth: Fix 403 being returned instead of 401

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

9 files changed, 98 insertions, 18 deletions

diff --git a/gluster/swift/common/middleware/gswauth/bin/gswauth-add-account b/gluster/swift/common/middleware/gswauth/bin/gswauth-add-account
index 9ac5919..0f72f06 100755
--- a/gluster/swift/common/middleware/gswauth/bin/gswauth-add-account
+++ b/gluster/swift/common/middleware/gswauth/bin/gswauth-add-account
@@ -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))
diff --git a/gluster/swift/common/middleware/gswauth/bin/gswauth-add-user b/gluster/swift/common/middleware/gswauth/bin/gswauth-add-user
index 0b57f6f..b9588ef 100755
--- a/gluster/swift/common/middleware/gswauth/bin/gswauth-add-user
+++ b/gluster/swift/common/middleware/gswauth/bin/gswauth-add-user
@@ -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))
diff --git a/gluster/swift/common/middleware/gswauth/bin/gswauth-cleanup-tokens b/gluster/swift/common/middleware/gswauth/bin/gswauth-cleanup-tokens
index a295b13..621124e 100755
--- a/gluster/swift/common/middleware/gswauth/bin/gswauth-cleanup-tokens
+++ b/gluster/swift/common/middleware/gswauth/bin/gswauth-cleanup-tokens
@@ -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))
diff --git a/gluster/swift/common/middleware/gswauth/bin/gswauth-delete-account b/gluster/swift/common/middleware/gswauth/bin/gswauth-delete-account
index 854a5d4..be8ace8 100755
--- a/gluster/swift/common/middleware/gswauth/bin/gswauth-delete-account
+++ b/gluster/swift/common/middleware/gswauth/bin/gswauth-delete-account
@@ -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))
diff --git a/gluster/swift/common/middleware/gswauth/bin/gswauth-delete-user b/gluster/swift/common/middleware/gswauth/bin/gswauth-delete-user
index a56dea1..2b4f4fe 100755
--- a/gluster/swift/common/middleware/gswauth/bin/gswauth-delete-user
+++ b/gluster/swift/common/middleware/gswauth/bin/gswauth-delete-user
@@ -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))
diff --git a/gluster/swift/common/middleware/gswauth/bin/gswauth-list b/gluster/swift/common/middleware/gswauth/bin/gswauth-list
index 7a38f77..23175e9 100755
--- a/gluster/swift/common/middleware/gswauth/bin/gswauth-list
+++ b/gluster/swift/common/middleware/gswauth/bin/gswauth-list
@@ -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)]]:
diff --git a/gluster/swift/common/middleware/gswauth/bin/gswauth-prep b/gluster/swift/common/middleware/gswauth/bin/gswauth-prep
index cfe75af..74071d5 100755
--- a/gluster/swift/common/middleware/gswauth/bin/gswauth-prep
+++ b/gluster/swift/common/middleware/gswauth/bin/gswauth-prep
@@ -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))
diff --git a/gluster/swift/common/middleware/gswauth/bin/gswauth-set-account-service b/gluster/swift/common/middleware/gswauth/bin/gswauth-set-account-service
index 1b5c354..b78524d 100755
--- a/gluster/swift/common/middleware/gswauth/bin/gswauth-set-account-service
+++ b/gluster/swift/common/middleware/gswauth/bin/gswauth-set-account-service
@@ -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))
diff --git a/gluster/swift/common/middleware/gswauth/swauth/middleware.py b/gluster/swift/common/middleware/gswauth/swauth/middleware.py
index bc5d085..ac1b295 100644
--- a/gluster/swift/common/middleware/gswauth/swauth/middleware.py
+++ b/gluster/swift/common/middleware/gswauth/swauth/middleware.py
@@ -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