From b1eea48ace9b4d88e08d4df70d53e61f9835997d Mon Sep 17 00:00:00 2001 From: Anon Ray Date: Mon, 13 May 2019 15:17:37 +0530 Subject: [PATCH 1/6] fix non-200 response for authorization errors on /v1/graphql --- server/src-lib/Hasura/Server/App.hs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/server/src-lib/Hasura/Server/App.hs b/server/src-lib/Hasura/Server/App.hs index 2b838e319ff04..9b296c0b139f8 100644 --- a/server/src-lib/Hasura/Server/App.hs +++ b/server/src-lib/Hasura/Server/App.hs @@ -131,17 +131,17 @@ withSCUpdate scr logger action = do data ServerCtx = ServerCtx - { scPGExecCtx :: PGExecCtx - , scConnInfo :: Q.ConnInfo - , scLogger :: L.Logger - , scCacheRef :: SchemaCacheRef - , scAuthMode :: AuthMode - , scManager :: HTTP.Manager - , scSQLGenCtx :: SQLGenCtx - , scEnabledAPIs :: S.HashSet API - , scInstanceId :: InstanceId - , scPlanCache :: E.PlanCache - , scLQState :: EL.LiveQueriesState + { scPGExecCtx :: PGExecCtx + , scConnInfo :: Q.ConnInfo + , scLogger :: L.Logger + , scCacheRef :: SchemaCacheRef + , scAuthMode :: AuthMode + , scManager :: HTTP.Manager + , scSQLGenCtx :: SQLGenCtx + , scEnabledAPIs :: S.HashSet API + , scInstanceId :: InstanceId + , scPlanCache :: E.PlanCache + , scLQState :: EL.LiveQueriesState } data HandlerCtx @@ -232,7 +232,9 @@ mkSpockAction qErrEncoder qErrModifier serverCtx handler = do manager = scManager serverCtx userInfoE <- liftIO $ runExceptT $ getUserInfo logger manager headers authMode - userInfo <- either (logAndThrow req reqBody False) return userInfoE + -- apply the error modifier + let modUserInfoE = fmapL qErrModifier userInfoE + userInfo <- either (logAndThrow req reqBody False) return modUserInfoE let handlerState = HandlerCtx serverCtx reqBody userInfo headers From 05dde339a43161fdf0fe18168062ea1426530512 Mon Sep 17 00:00:00 2001 From: Anon Ray Date: Mon, 13 May 2019 16:06:48 +0530 Subject: [PATCH 2/6] fix review comment --- server/src-lib/Hasura/Server/App.hs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/server/src-lib/Hasura/Server/App.hs b/server/src-lib/Hasura/Server/App.hs index 9b296c0b139f8..87b6c51dc98db 100644 --- a/server/src-lib/Hasura/Server/App.hs +++ b/server/src-lib/Hasura/Server/App.hs @@ -232,9 +232,7 @@ mkSpockAction qErrEncoder qErrModifier serverCtx handler = do manager = scManager serverCtx userInfoE <- liftIO $ runExceptT $ getUserInfo logger manager headers authMode - -- apply the error modifier - let modUserInfoE = fmapL qErrModifier userInfoE - userInfo <- either (logAndThrow req reqBody False) return modUserInfoE + userInfo <- either (logAndThrow req reqBody False . qErrModifier) return userInfoE let handlerState = HandlerCtx serverCtx reqBody userInfo headers From 340a62a2c310f04f2cc34ce6c5592ef803b4b61a Mon Sep 17 00:00:00 2001 From: Anon Ray Date: Mon, 13 May 2019 20:34:44 +0530 Subject: [PATCH 3/6] add tests - TODO: add authorization error test for v1alphaq/graphql --- server/tests-py/test_v1alpha1_endpoint.py | 34 +++++++++++++++++++++++ server/tests-py/validate.py | 23 +++++++++++++-- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/server/tests-py/test_v1alpha1_endpoint.py b/server/tests-py/test_v1alpha1_endpoint.py index 1744f9ab39022..a83b7faaadd39 100644 --- a/server/tests-py/test_v1alpha1_endpoint.py +++ b/server/tests-py/test_v1alpha1_endpoint.py @@ -1,5 +1,6 @@ import yaml import pytest +#from validate import check_query, test_forbidden_when_admin_secret_reqd, test_forbidden_webhook from validate import check_query from super_classes import DefaultTestSelectQueries from context import GQLWsClient @@ -11,6 +12,39 @@ class TestV1Alpha1GraphQLErrors(DefaultTestSelectQueries): def dir(cls): return 'queries/graphql_query/v1alpha1/errors' + def test_v1alpha1_authorization_error(self, hge_ctx): + gql_query = """ + query { + author { + id + name + } + } + """ + http_conf = { + 'url': '/v1alpha1/graphql', + 'status': 200, + 'query': {'query': gql_query}, + } + print('ok') + + # if hge_ctx.hge_key is not None and hge_ctx.hge_webhook is None and hge_ctx.hge_jwt_key is None: + # # Test whether it is forbidden when incorrect/no admin_secret is specified + # print(' I AM HERE !!') + # test_forbidden_when_admin_secret_reqd(hge_ctx, http_conf) + # assert True + + # elif hge_ctx.hge_webhook is not None: + # print(' I AM HERE NOWWWW !!') + # if not hge_ctx.webhook_insecure: + # # Check whether the output is also forbidden when webhook returns forbidden + # test_forbidden_webhook(hge_ctx, http_conf) + # assert True + # else: + # print(' AND HERE ........ !! ') + # pass + + @pytest.mark.parametrize('transport', ['http', 'websocket']) def test_v1alpha1_validation_error(self, hge_ctx, transport): gql_query = """ diff --git a/server/tests-py/validate.py b/server/tests-py/validate.py index 661ae377f56cf..fb48f8c9ac77e 100644 --- a/server/tests-py/validate.py +++ b/server/tests-py/validate.py @@ -59,13 +59,21 @@ def check_event(hge_ctx, evts_webhook, trig_name, table, operation, exp_ev_data, def test_forbidden_when_admin_secret_reqd(hge_ctx, conf): + if conf['url'] == '/v1alpha1/graphql': + status = [401, 404] + elif conf['url'] == '/v1/graphql': + status = [200] + else: + raise ValueError('unknown endpoint ' + conf['url']) + headers = {} if 'headers' in conf: headers = conf['headers'] # Test without admin secret code, resp = hge_ctx.anyq(conf['url'], conf['query'], headers) - assert code in [401,404], "\n" + yaml.dump({ + #assert code in [401,404], "\n" + yaml.dump({ + assert code in status, "\n" + yaml.dump({ "expected": "Should be access denied as admin secret is not provided", "actual": { "code": code, @@ -76,7 +84,8 @@ def test_forbidden_when_admin_secret_reqd(hge_ctx, conf): # Test with random admin secret headers['X-Hasura-Admin-Secret'] = base64.b64encode(os.urandom(30)) code, resp = hge_ctx.anyq(conf['url'], conf['query'], headers) - assert code in [401,404], "\n" + yaml.dump({ + #assert code in [401,404], "\n" + yaml.dump({ + assert code in status, "\n" + yaml.dump({ "expected": "Should be access denied as an incorrect admin secret is provided", "actual": { "code": code, @@ -86,9 +95,17 @@ def test_forbidden_when_admin_secret_reqd(hge_ctx, conf): def test_forbidden_webhook(hge_ctx, conf): + if conf['url'] == '/v1alpha1/graphql': + status = [401, 404] + elif conf['url'] == '/v1/graphql': + status = [200] + else: + raise ValueError('unknown endpoint ' + conf['url']) + h = {'Authorization': 'Bearer ' + base64.b64encode(base64.b64encode(os.urandom(30))).decode('utf-8')} code, resp = hge_ctx.anyq(conf['url'], conf['query'], h) - assert code in [401,404], "\n" + yaml.dump({ + #assert code in [401,404], "\n" + yaml.dump({ + assert code in status, "\n" + yaml.dump({ "expected": "Should be access denied as it is denied from webhook", "actual": { "code": code, From 2e93b9db1b366118330102d38f50b8b2ca74e752 Mon Sep 17 00:00:00 2001 From: Anon Ray Date: Tue, 14 May 2019 10:37:48 +0530 Subject: [PATCH 4/6] fix tests --- server/tests-py/test_jwt.py | 54 +++++++++++++++++++++++++++---------- server/tests-py/validate.py | 17 ++++++------ 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/server/tests-py/test_jwt.py b/server/tests-py/test_jwt.py index 811bd925b2f5f..9c34c7b66ac46 100644 --- a/server/tests-py/test_jwt.py +++ b/server/tests-py/test_jwt.py @@ -35,9 +35,10 @@ def mk_claims(conf, claims): else: return claims -class TestJWTBasic: +@pytest.mark.parametrize('endpoint', ['/v1/graphql', '/v1alpha1/graphql']) +class TestJWTBasic(): - def test_jwt_valid_claims_success(self, hge_ctx): + def test_jwt_valid_claims_success(self, hge_ctx, endpoint): self.claims['https://hasura.io/jwt/claims'] = mk_claims(hge_ctx.hge_jwt_conf, { 'x-hasura-user-id': '1', 'x-hasura-allowed-roles': ['user', 'editor'], @@ -45,10 +46,11 @@ def test_jwt_valid_claims_success(self, hge_ctx): }) token = jwt.encode(self.claims, hge_ctx.hge_jwt_key, algorithm='RS512').decode('utf-8') self.conf['headers']['Authorization'] = 'Bearer ' + token + self.conf['url'] = endpoint self.conf['status'] = 200 check_query(hge_ctx, self.conf, add_auth=False) - def test_jwt_invalid_role_in_request_header(self, hge_ctx): + def test_jwt_invalid_role_in_request_header(self, hge_ctx, endpoint): self.claims['https://hasura.io/jwt/claims'] = mk_claims(hge_ctx.hge_jwt_conf, { 'x-hasura-user-id': '1', 'x-hasura-allowed-roles': ['contractor', 'editor'], @@ -65,10 +67,14 @@ def test_jwt_invalid_role_in_request_header(self, hge_ctx): 'message': 'Your current role is not in allowed roles' }] } - self.conf['status'] = 400 + self.conf['url'] = endpoint + if endpoint == '/v1/graphql': + self.conf['status'] = 200 + if endpoint == '/v1alpha1/graphql': + self.conf['status'] = 400 check_query(hge_ctx, self.conf, add_auth=False) - def test_jwt_no_allowed_roles_in_claim(self, hge_ctx): + def test_jwt_no_allowed_roles_in_claim(self, hge_ctx, endpoint): self.claims['https://hasura.io/jwt/claims'] = mk_claims(hge_ctx.hge_jwt_conf, { 'x-hasura-user-id': '1', 'x-hasura-default-role': 'user' @@ -84,10 +90,14 @@ def test_jwt_no_allowed_roles_in_claim(self, hge_ctx): 'message': 'JWT claim does not contain x-hasura-allowed-roles' }] } - self.conf['status'] = 400 + self.conf['url'] = endpoint + if endpoint == '/v1/graphql': + self.conf['status'] = 200 + if endpoint == '/v1alpha1/graphql': + self.conf['status'] = 400 check_query(hge_ctx, self.conf, add_auth=False) - def test_jwt_invalid_allowed_roles_in_claim(self, hge_ctx): + def test_jwt_invalid_allowed_roles_in_claim(self, hge_ctx, endpoint): self.claims['https://hasura.io/jwt/claims'] = mk_claims(hge_ctx.hge_jwt_conf, { 'x-hasura-user-id': '1', 'x-hasura-allowed-roles': 'user', @@ -104,10 +114,14 @@ def test_jwt_invalid_allowed_roles_in_claim(self, hge_ctx): 'message': 'invalid x-hasura-allowed-roles; should be a list of roles' }] } - self.conf['status'] = 400 + self.conf['url'] = endpoint + if endpoint == '/v1/graphql': + self.conf['status'] = 200 + if endpoint == '/v1alpha1/graphql': + self.conf['status'] = 400 check_query(hge_ctx, self.conf, add_auth=False) - def test_jwt_no_default_role(self, hge_ctx): + def test_jwt_no_default_role(self, hge_ctx, endpoint): self.claims['https://hasura.io/jwt/claims'] = mk_claims(hge_ctx.hge_jwt_conf, { 'x-hasura-user-id': '1', 'x-hasura-allowed-roles': ['user'], @@ -123,10 +137,14 @@ def test_jwt_no_default_role(self, hge_ctx): 'message': 'JWT claim does not contain x-hasura-default-role' }] } - self.conf['status'] = 400 + self.conf['url'] = endpoint + if endpoint == '/v1/graphql': + self.conf['status'] = 200 + if endpoint == '/v1alpha1/graphql': + self.conf['status'] = 400 check_query(hge_ctx, self.conf, add_auth=False) - def test_jwt_expired(self, hge_ctx): + def test_jwt_expired(self, hge_ctx, endpoint): self.claims['https://hasura.io/jwt/claims'] = mk_claims(hge_ctx.hge_jwt_conf, { 'x-hasura-user-id': '1', 'x-hasura-default-role': 'user', @@ -146,10 +164,14 @@ def test_jwt_expired(self, hge_ctx): 'message': 'Could not verify JWT: JWTExpired' }] } - self.conf['status'] = 400 + self.conf['url'] = endpoint + if endpoint == '/v1/graphql': + self.conf['status'] = 200 + if endpoint == '/v1alpha1/graphql': + self.conf['status'] = 400 check_query(hge_ctx, self.conf, add_auth=False) - def test_jwt_invalid_signature(self, hge_ctx): + def test_jwt_invalid_signature(self, hge_ctx, endpoint): self.claims['https://hasura.io/jwt/claims'] = mk_claims(hge_ctx.hge_jwt_conf, { 'x-hasura-user-id': '1', 'x-hasura-default-role': 'user', @@ -168,7 +190,11 @@ def test_jwt_invalid_signature(self, hge_ctx): 'message': 'Could not verify JWT: JWSError JWSInvalidSignature' }] } - self.conf['status'] = 400 + self.conf['url'] = endpoint + if endpoint == '/v1/graphql': + self.conf['status'] = 200 + if endpoint == '/v1alpha1/graphql': + self.conf['status'] = 400 check_query(hge_ctx, self.conf, add_auth=False) @pytest.fixture(autouse=True) diff --git a/server/tests-py/validate.py b/server/tests-py/validate.py index fb48f8c9ac77e..46e9367cf7c88 100644 --- a/server/tests-py/validate.py +++ b/server/tests-py/validate.py @@ -59,12 +59,13 @@ def check_event(hge_ctx, evts_webhook, trig_name, table, operation, exp_ev_data, def test_forbidden_when_admin_secret_reqd(hge_ctx, conf): - if conf['url'] == '/v1alpha1/graphql': - status = [401, 404] - elif conf['url'] == '/v1/graphql': - status = [200] + if conf['url'] == '/v1/graphql': + if conf['status'] == 404: + status = [404] + else: + status = [200] else: - raise ValueError('unknown endpoint ' + conf['url']) + status = [401, 404] headers = {} if 'headers' in conf: @@ -95,12 +96,10 @@ def test_forbidden_when_admin_secret_reqd(hge_ctx, conf): def test_forbidden_webhook(hge_ctx, conf): - if conf['url'] == '/v1alpha1/graphql': - status = [401, 404] - elif conf['url'] == '/v1/graphql': + if conf['url'] == '/v1/graphql': status = [200] else: - raise ValueError('unknown endpoint ' + conf['url']) + status = [401, 404] h = {'Authorization': 'Bearer ' + base64.b64encode(base64.b64encode(os.urandom(30))).decode('utf-8')} code, resp = hge_ctx.anyq(conf['url'], conf['query'], h) From 6dfe12e63f85e87d84df36c2c9dffe646c9aa985 Mon Sep 17 00:00:00 2001 From: Anon Ray Date: Tue, 14 May 2019 13:00:39 +0530 Subject: [PATCH 5/6] add url logic in validate.py --- server/tests-py/validate.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/tests-py/validate.py b/server/tests-py/validate.py index 46e9367cf7c88..c080c5902658f 100644 --- a/server/tests-py/validate.py +++ b/server/tests-py/validate.py @@ -97,7 +97,10 @@ def test_forbidden_when_admin_secret_reqd(hge_ctx, conf): def test_forbidden_webhook(hge_ctx, conf): if conf['url'] == '/v1/graphql': - status = [200] + if conf['status'] == 404: + status = [404] + else: + status = [200] else: status = [401, 404] From 968e975a7de7e05ba1395a838928824d4598f037 Mon Sep 17 00:00:00 2001 From: Anon Ray Date: Tue, 14 May 2019 14:54:42 +0530 Subject: [PATCH 6/6] add authorization test for v1alpha1 endpoint --- server/tests-py/test_v1alpha1_endpoint.py | 25 +++++++++-------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/server/tests-py/test_v1alpha1_endpoint.py b/server/tests-py/test_v1alpha1_endpoint.py index a83b7faaadd39..52c756ba41792 100644 --- a/server/tests-py/test_v1alpha1_endpoint.py +++ b/server/tests-py/test_v1alpha1_endpoint.py @@ -2,6 +2,7 @@ import pytest #from validate import check_query, test_forbidden_when_admin_secret_reqd, test_forbidden_webhook from validate import check_query +import validate from super_classes import DefaultTestSelectQueries from context import GQLWsClient @@ -26,23 +27,17 @@ def test_v1alpha1_authorization_error(self, hge_ctx): 'status': 200, 'query': {'query': gql_query}, } - print('ok') - # if hge_ctx.hge_key is not None and hge_ctx.hge_webhook is None and hge_ctx.hge_jwt_key is None: - # # Test whether it is forbidden when incorrect/no admin_secret is specified - # print(' I AM HERE !!') - # test_forbidden_when_admin_secret_reqd(hge_ctx, http_conf) - # assert True + if hge_ctx.hge_key is not None and hge_ctx.hge_webhook is None and hge_ctx.hge_jwt_key is None: + # Test whether it is forbidden when incorrect/no admin_secret is specified + validate.test_forbidden_when_admin_secret_reqd(hge_ctx, http_conf) - # elif hge_ctx.hge_webhook is not None: - # print(' I AM HERE NOWWWW !!') - # if not hge_ctx.webhook_insecure: - # # Check whether the output is also forbidden when webhook returns forbidden - # test_forbidden_webhook(hge_ctx, http_conf) - # assert True - # else: - # print(' AND HERE ........ !! ') - # pass + elif hge_ctx.hge_webhook is not None: + if not hge_ctx.webhook_insecure: + # Check whether the output is also forbidden when webhook returns forbidden + validate.test_forbidden_webhook(hge_ctx, http_conf) + else: + assert True @pytest.mark.parametrize('transport', ['http', 'websocket'])