Gml 1660 support jwt token auth in py tiger graph #224

luzhoutg · 2024-05-10T21:25:22Z

No description provided.

parkererickson-tg

Version should not be a connection parameter, we should be able to use the function to get it dynamically.

luzhoutg · 2024-05-10T22:14:14Z

Yes, just switch to getVer() to get the version dynamically.

billshitg

Have we tested the jwt token authentication with restpp?

parkererickson-tg · 2024-05-13T13:32:44Z

pyTigerGraph/pyTigerGraphBase.py

@@ -124,6 +126,14 @@ def __init__(self, host: str = "http://127.0.0.1", graphname: str = "MyGraph",
        else:
            self.authHeader = {"Authorization": "Basic {0}".format(self.base64_credential)}

+        # If JWT token is provided, set authMode to "token", and overwrite authMode = "pwd" for GSQL authentication as well if version is newer than 4.1.0
+        if jwtToken:
+            dbVersion = self.getVer()


This version check will fail since version relies on token auth, and the auth header has not yet been defined.

parkererickson-tg · 2024-05-13T13:33:14Z

pyTigerGraph/pyTigerGraphBase.py

+        if jwtToken:
+            dbVersion = self.getVer()
+            if StrictVersion(dbVersion) >= StrictVersion("4.1.0"):
+                self.authMode = "token"


This self.authMode variable isn't used anywhere, why are we setting it?

luzhoutg · 2024-05-13T15:55:38Z

Thanks Parker for the comments. I will modify the PR and test it today.

luzhoutg · 2024-05-24T20:37:16Z

move getVersion() and getVer() from pyTigerGraphUtils to pyTigerGraphBase
use getVer() and _get() schema of a graph in pyTigerGraphBase to check restpp and gsql authentication respectively
tested on 3.10.1 using jwt token for restpp authentication successfully
waiting for patch to test gsql authentication

parkererickson-tg · 2024-05-24T21:01:23Z

tests/test_jwt.py

This needs to be disabled such that our other tests don't fail... @billshitg, do you have thoughts on how to support this test in the future?

parkererickson-tg · 2024-05-24T21:01:53Z

tests/pyTigerGraphUnitTest.py

@@ -42,6 +43,7 @@ def make_connection(graphname: str = None):
        certPath=server_config["certPath"],
        sslPort=server_config["sslPort"],
        gcp=server_config["gcp"],
+        jwtToken=server_config["jwtToken"]


Can we support this in our current testing pipeline?

Yeah, we need to modify the config file to add the jwtToken field but set it to empty str to avoid actually using it now.

Meanwhile, we need to figure out with QE team how to config the DB to use jwtToken

Just double check here: does server_config contains this key "jwtToken"? I don't see the config file modified.

Sure. I just add the jwtToken to the config file and set it to empty.

Just double check here: does server_config contains this key "jwtToken"? I don't see the config file modified.

billshitg · 2024-05-24T20:53:57Z

pyTigerGraph/pyTigerGraphBase.py

+
+        # TODO Remove apiToken parameter
+        # if apiToken:
+        #     warnings.warn(


Why do we remove this deprecation warning but keep the apiToken parameter

was trying to clean up the code. There are several deprecation warnings other than "apiToken" parameter. I can add the deprecation warning back.

The apiToken parameter is used for CoPilot

billshitg · 2024-05-24T20:59:34Z

pyTigerGraph/pyTigerGraphBase.py

+        try:
+            logger.debug(f"Attempting to get schema with URL: {self.gsUrl + '/gsqlserver/gsql/schema?graph=' + self.graphname}")
+            logger.debug(f"Using auth header: {self.authHeader}") 
+            self._get(self.gsUrl + "/gsqlserver/gsql/schema?graph=" + self.graphname, authMode="token")


Do we have to use graphname here? Might cause issue when there is no graph in the database

The graphname is a required field accoroding to https://docs.tigergraph.com/tigergraph-server/current/api/built-in-endpoints#_show_graph_schema_metadata. We don't have a package to test on the GSQL auth yet. So if there no graph in the database, it could cause issue. might need to change to another function for validation after testing on.

billshitg · 2024-05-24T21:02:44Z

pyTigerGraph/pyTigerGraphBase.py

-            self.authHeader = {'Authorization': 'Basic {0}'.format(self.base64_credential)}
-            _headers = self.authHeader
-            authMode = 'pwd'
+        # If JWT token is provided, always use jwtToken as token


Is jwtToken also used when authMode is pwd?

currently no, still use pwd. in the init, if the db doesn't support jwt, it will raise an error asking users to use username and password.

billshitg · 2024-05-24T21:07:46Z

tests/test_jwt.py

Not sure how we can test this file in our CICD pipeline. Will need to talk to our QE team to find out how we can configure the DB to use the JWT token auth.

parkererickson · 2024-05-24T21:10:35Z

We need to keep the apiToken parameter, I don’t know why we were deprecating it in the first place. Get Outlook for iOS<https://aka.ms/o0ukef>

________________________________ From: billshitg ***@***.***> Sent: Friday, May 24, 2024 2:08:25 PM To: tigergraph/pyTigerGraph ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [tigergraph/pyTigerGraph] Gml 1660 support jwt token auth in py tiger graph (PR #224) @billshitg commented on this pull request.

________________________________ In pyTigerGraph/pyTigerGraphBase.py<#224 (comment)>:

self.apiToken = apiToken

+ self.base64_credential = base64.b64encode( + "{0}:{1}".format(self.username, self.password).encode("utf-8")).decode("utf-8") + + self.authHeader = self._set_auth_header() + + # TODO Remove apiToken parameter + # if apiToken: + # warnings.warn( Why do we remove this deprecation warning but keep the apiToken parameter

________________________________ In pyTigerGraph/pyTigerGraphBase.py<#224 (comment)>:

+ if e.response.status_code == 403:

+ logger.error(f"Unauthorized error: {e}. The JWT token might be invalid or expired.") + else: + logger.error(f"HTTP error occurred: {e}") + # logger.error("The DB version using doesn't support JWT token for RestPP. Please switch to API token or username/password.") + raise + except Exception as e: + logger.error(f"Error occurred: {e}. The DB version using doesn't support JWT token for RestPP.") + logger.error("Please switch to API token or username/password.") + raise + + # Check JWT support for GSQL server + try: + logger.debug(f"Attempting to get schema with URL: {self.gsUrl + '/gsqlserver/gsql/schema?graph=' + self.graphname}") + logger.debug(f"Using auth header: {self.authHeader}") + self._get(self.gsUrl + "/gsqlserver/gsql/schema?graph=" + self.graphname, authMode="token") Do we have to use graphname here? Might cause issue when there is no graph in the database

________________________________ In pyTigerGraph/pyTigerGraphBase.py<#224 (comment)>:

@@ -257,20 +317,27 @@ def _req(self, method: str, url: str, authMode: str = "token", headers: dict = N

if logger.level == logging.DEBUG: logger.debug("params: " + self._locals(locals())) - if authMode == "token" and str(self.apiToken) != "": - if isinstance(self.apiToken, tuple): - self.apiToken = self.apiToken[0] - self.authHeader = {'Authorization': "Bearer " + self.apiToken} - _headers = self.authHeader - else: - self.authHeader = {'Authorization': 'Basic {0}'.format(self.base64_credential)} - _headers = self.authHeader - authMode = 'pwd' + # If JWT token is provided, always use jwtToken as token Is jwtToken also used when authMode is pwd?

…

________________________________ On tests/test_jwt.py<#224 (comment)>: Not sure how we can test this file in our CICD pipeline. Will need to talk to our QE team to find out how we can configure the DB to use the JWT token auth. — Reply to this email directly, view it on GitHub<#224 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACJLWK7L2H24UZCF7CGVOPDZD6T4TAVCNFSM6AAAAABHRIYCC6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZXHE4TSNJUGM>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

…r-graph

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1062/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1063/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1066/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1067/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1068/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1070/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1072/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1075/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1076/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1077/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1078/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1079/

qe-tigergraph

Unit Test: FAILURE, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1080/

qe-tigergraph

Unit Test: SUCCESS, e2e Test: SKIPPED, Jenkins_job:http://192.168.99.101:30080/job/mlwb_build/1081/

qe-tigergraph

QE Approved

parkererickson-tg · 2024-06-12T15:14:16Z

tests/test_jwtAuth.py

+
+    def _requestJWTToken(self):
+        # Define the URL
+        url = f"{self.conn.host}:{self.conn.gsPort}/gsqlserver/requestjwttoken"


Should we make the jwt request endpoint as a utility function?

Good point. Maybe we should add it to the GSQL integration plan?

That is a good idea. I will create another PR for this.

billshitg · 2024-06-12T16:28:46Z

tests/pyTigerGraphUnitTest.py

@@ -42,6 +43,7 @@ def make_connection(graphname: str = None):
        certPath=server_config["certPath"],
        sslPort=server_config["sslPort"],
        gcp=server_config["gcp"],
+        jwtToken=server_config["jwtToken"]


Just double check here: does server_config contains this key "jwtToken"? I don't see the config file modified.

Lu Zhou added 2 commits May 10, 2024 09:59

GML-1660 add jwttoken support for authentication for restpp and gsql

e3aac00

GML-1660 add dbversion to set authMode

5537745

luzhoutg added the enhancement New feature or request label May 10, 2024

luzhoutg requested review from billshitg and parkererickson-tg May 10, 2024 21:25

luzhoutg requested a review from qe-tigergraph as a code owner May 10, 2024 21:25

parkererickson-tg requested changes May 10, 2024

View reviewed changes

Lu Zhou added 2 commits May 10, 2024 15:08

GML-1660 use the gerVer() to get dbversion

b584eb2

GML-1660 use the gerVer() to get dbversion

b12759c

luzhoutg requested a review from parkererickson-tg May 10, 2024 22:13

billshitg reviewed May 10, 2024

View reviewed changes

parkererickson-tg requested changes May 13, 2024

View reviewed changes

Lu Zhou added 2 commits May 24, 2024 13:16

GML-1660 add logic to check if supporting JWT on RestPP and GSQL

030656c

GML-1660 fix the version configuration

80f0503

luzhoutg requested review from billshitg and parkererickson-tg May 24, 2024 20:37

parkererickson-tg reviewed May 24, 2024

View reviewed changes

billshitg reviewed May 24, 2024

View reviewed changes

Lu Zhou and others added 3 commits June 5, 2024 17:25

add unit test

ce0d746

Merge branch 'master' into GML-1660-support-jwt-token-auth-in-py-tige…

3fef8fc

…r-graph

GML-1660 add unit test

1ad4209

luzhoutg requested review from billshitg and parkererickson-tg June 7, 2024 06:02

qe-tigergraph requested changes Jun 7, 2024

View reviewed changes

GML-1660 correct error message for test

1cf1615

qe-tigergraph requested changes Jun 7, 2024

View reviewed changes

qe-tigergraph requested changes Jun 9, 2024

View reviewed changes

GML-1660 create tests graph for jwtauth

6fabbc5

qe-tigergraph requested changes Jun 10, 2024

View reviewed changes

GML-1660 move graph creation

0d20920

qe-tigergraph requested changes Jun 10, 2024

View reviewed changes

Lu Zhou added 2 commits June 10, 2024 12:29

GML-1660 create new graph and generate token

34f05cb

GML-1660 create jwttoken graph first and generate token on it

185d1d4

qe-tigergraph requested changes Jun 10, 2024

View reviewed changes

GML-1660 make connection()

3476060

qe-tigergraph requested changes Jun 10, 2024

View reviewed changes

GML-1660 swithc to graph Cora

163236a

qe-tigergraph requested changes Jun 11, 2024

View reviewed changes

GML-1660 setToken is True

5bc4f3e

qe-tigergraph requested changes Jun 11, 2024

View reviewed changes

Lu Zhou added 2 commits June 11, 2024 09:04

GML-1660 change authheader to token

089165b

GML-1660 change authheader to token

c55cca4

qe-tigergraph requested changes Jun 11, 2024

View reviewed changes

GML-1660 test getVer()

eebf27e

qe-tigergraph requested changes Jun 11, 2024

View reviewed changes

GML-1660 change the importing make_connection

3403e3c

qe-tigergraph reviewed Jun 12, 2024

View reviewed changes

qe-tigergraph approved these changes Jun 12, 2024

View reviewed changes

GML-1660 remove print

f5ec141

parkererickson-tg approved these changes Jun 12, 2024

View reviewed changes

billshitg approved these changes Jun 12, 2024

View reviewed changes

Lu Zhou added 2 commits June 12, 2024 10:09

GML-1660 add jwtToken in config file and set it to empty.

e69505b

GML-1660 add jwtToken in config file and set it to empty.

39bca3a

luzhoutg merged commit 26919bd into master Jun 12, 2024

Gml 1660 support jwt token auth in py tiger graph #224

Gml 1660 support jwt token auth in py tiger graph #224

Conversation

luzhoutg commented May 10, 2024

parkererickson-tg left a comment

Choose a reason for hiding this comment

luzhoutg commented May 10, 2024

billshitg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luzhoutg commented May 13, 2024

luzhoutg commented May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkererickson commented May 24, 2024 via email

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

qe-tigergraph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luzhoutg Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luzhoutg commented May 24, 2024 •

edited

Loading

luzhoutg Jun 12, 2024 •

edited

Loading