[draft] Update OAuthSampleApp.java, Added refresh token api to get new access… #19

rvpanchal1202 · 2024-04-17T04:08:01Z

… token

using the refresh token, we can get the new access token in case of a wrong access token or an expired access token.

Added API to get the new access_token using refresh_token

note: When we request for new access_token using refresh token, This request will give us both the tokens. which we can save later to original tokens and start using new Tokens instead of old tokens

… token using the refresh token, we can get the new access token in case of a wrong access token or an expired access token. Added API to get the new access_token using refresh_token note: When we request for new access_token using refresh token, This request will give us both the tokens. which we can save later to original tokens and start using new Tokens instead of old tokens

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

DMickens · 2024-04-19T13:01:21Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+    private static String OAUTH_SERVER = System.getenv("OAUTH_SERVER"); 
+    private static String OAUTH_HTTP_PORT =  System.getenv("OAUTH_HTTP_PORT");
+
+    private static String getTokenUrl()


getTokenUrl, getHttpsTokenUrl, getIssuerUri, and getDiscoveryUrl are all unneeded. The only ones being used is the token/https token url and if we just have that be provided through the new environment variable then we don't need the get function

These unneccessary methods are removed

williammatherjones

These changes are reasonable in isolation, but the task was to align the JDBC OAuth sample app with the ADO.NET implementation. We took a lot of time and effort to develop the ADO sample. My suggestion is to start with a port of the structure of the ADO.NET sample app.

The user should configure it to run by setting values in a properties file with the same settings as are in the ADO.NET sample
The app should check and set the values of the OAUTH_SAMPLE_ACCESS_TOKEN and OAUTH_SAMPLE_REFRESH_TOKEN environment variables.
You should be able to run both sample apps interchangeably. Running the Java sample should set the access and refresh tokens that can be used by the ADO sample and visa versa.
When reading the code, it should have the same overall logic and naming. If someone understands the one app, they should have an immediate familiarity with the other.

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

DMickens · 2024-04-19T13:51:48Z

Here is a description of the flow that I suggest using. The ADO sample does essentially the same thing, and that's really what we want - to align the samples with a common recommended flow. Here are the steps:

set up database
set a local boolean variable (haveRequestedAccessToken or something like that) to show if we have programmatically gotten a new access token yet
check for an access token in the environment.
if we have one, great. Set haveRequestedAccessToken to false.
if we don't have one, then check for a refresh token in the environment. If we have a refresh token, use refresh token grant to obtain access and refresh tokens from the IDP, update the environment variables with the new tokens, and set haveRequestedAccessToken to true. If we don't have a refresh token either, then we use a password grant type instead to obtain the tokens, update the token environment variables, and set haveRequestedAccessToken to true.
Attempt to connect to database. Only pass the driver an access token.
If the connection attempt fails due to possible invalid access token (token introspection failure from the server) then check the haveRequestedAccessToken variable. If it is true, meaning we already got a new one from the IDP, then exit and report an error. If it is false, then it means we may have just been given an invalid access token. In this case we do token refresh with the refresh token grant to make sure we have a valid access token and then try to connect again. If it still fails we would exit and report an error.

One difference is that the ADO sample uses a configuration file instead of environment variables for the initial set of properties, excluding access token and refresh token. If we can do that for this sample as well that would be a good change.

Changes in the client application done as per comments and discussion. SetUpDbForOAuth() -> Adding authentication record in Vertica database is having issues. It's dropping the user and authentication method, but while setting the Authentication record, there is some error because of which Token authentication is failing and Database connection with access token is unable to establish. Without SetUpDbForOAuth() method call, Application is woking good.

Commented SetUpDbForOAuth() and TearDown() call.

williammatherjones

There are some issues that must be addressed before this can be approved. See comments.

williammatherjones · 2024-04-30T20:50:06Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+
+    private static void TearDown() throws Exception
+    {
+		System.out.println("TearDown");


I don't think we want the println here, right?

Removed unnecessary System - printlns.

williammatherjones · 2024-04-30T20:51:17Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+		System.out.println("TearDown");
+    	Statement st = vConnection.createStatement(); 
+    	String USER = prop.getProperty("User");
+    //    st.execute("DROP USER IF EXISTS " + USER + " CASCADE");


Don't we want to drop the user and auth method? This is the ADO.NET sample code:

public static async Task TearDown() { string USER = getSetting("User"); VerticaCommand cmd = new VerticaCommand("DROP USER IF EXISTS " + USER + " CASCADE", vConnection); cmd.ExecuteNonQuery(); cmd.CommandText = "DROP AUTHENTICATION IF EXISTS adooauth CASCADE"; cmd.ExecuteNonQuery(); vConnection.Close(); }

Hi @williammatherjones,

Yes, We need to drop this user and auth method.
But As of now, there is an issue with adding/creating the Authentication record using sample app code. Same statements are able to execute via VSQL and it's working fine with this app.

But, if we use the same statements from outh sample app is not work. Need to check and resolve this issue. there after this methods we will uncomment.

Issue resolved for the SetUpDbForOAuth and TearDown Method.

williammatherjones · 2024-04-30T20:54:07Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

-        return DriverManager.getConnection(
-                "jdbc:vertica://" + host + ":" + port + "/" + dbName, jdbcOptions);
+		return DriverManager.getConnection(
+                "jdbc:vertica://" + "172.17.0.45" + ":" + "5433" + "/" + "verticadb21451", jdbcOptions);


Why is the server, port and database hard coded? This should not be in the PR.

Added the method to get this data from config.properties vars.

williammatherjones · 2024-04-30T20:56:17Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+                "jdbc:vertica://" + "172.17.0.45" + ":" + "5433" + "/" + "verticadb21451", jdbcOptions);
+    }
+
+    private static void ConnectToDatabase() throws SQLException {


The overall formatting looks off. Please be sure to use standard source formatting and indentation.

The formating of code was done as per tab stop 4 and git had it as tab stop 8, updated that. Also made changes for indentation in git view.

williammatherjones · 2024-04-30T20:58:38Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

-            ResultSet rs = executeQuery(conn);
-            printResults(rs);
-            conn.close();
+       		//SetUpDbForOAuth();


Should this skip SetUpDbForOAuth? That seems pretty important.

Hi @williammatherjones : There are some issues in SetUpDbForOAuth(){}
it's not able to set up the Oauth configurations in the Vertica database.
Note: Same Query statements are working fine if we execute using vsql and run this application. Need to analyze more to resolve this issue.

The issue with SetUpDbForOAuth method is resolved.

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

kevinkarch88 · 2024-05-01T14:51:35Z

I think it would be good to take a closer look at the logic in how the ADO.NET OAuth sample connects. Right now, I don't think the logic to retry on failure is there.

@commandline

-> Removed all @commandline and picoli imports -> Removed all the blank lines from the file -> Indentation issue resolved. Note:Please adjust the tab stop as 4 to view the file content properly indented. -> Removed if else for first element in the map for framing the connectionString. -> All the Start braces and end braces are aligned and made the same in every place -> Added null check for an access token and refresh token after getting it from token URL

kevinkarch88 · 2024-05-03T14:12:52Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

-@Command(name = "OAuthSampleApp", mixinStandardHelpOptions = true, version = "OAuth sample app 1.0", description = "Connects to a Vertica database using OAuth")
-public class OAuthSampleApp implements Callable<Integer> {
-
+public class OAuthSampleApp implements Callable<Integer> 


There should always be a line between the start of the class and imports. There's also places in this file where there is no space between methods.

Removed the space or empty lines from the file as per suggestions from @sandeep Rawat.
Added the line between class and imports.

There are still no spaces between functions.

Added the spaces between the Functions.

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

Added config.properties file which would be required for running the sample app. User have to update the values as per configuration. Note: Given Sample example for the Connection string. Because Connection string will be accepted in this format. <KeyName>=<Value>;<KeyName>=<Value>;......

-> Added functions for the redundant code for copying data to the buffered output stream. -> removed unnecessary imports.

Removed ClientID, ClientSecret, Tokenurl from getconnection Call.

Added Line between imports and class

Debug prints removed

Deprecated method use JsonParser().parse(res); removed. used JsonParser.parseString(res);

kevinkarch88 · 2024-05-06T14:27:12Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+	private static String OAUTH_REFRESH_TOKEN_VAR_STRING = "OAUTH_SAMPLE_REFRESH_TOKEN";
+	private static Connection vConnection;
+	// Get jdbc connection string from provided configuration
+	private static String getConnectionString(String args) 


You can simplify this a lot by removing getConnectionString and getParam. Just put a sample connection string in the config properties like jdbc:vertica://localhost:5433/VMart or whatever you use.

kevinkarch88 · 2024-05-06T14:38:12Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+				// set Tokens as System Properties - new values to access_token and refresh_token
+	            String accessToken = jObject.has("access_token") ? jObject.get("access_token").getAsString() : null;
+	            String refreshToken = jObject.has("refresh_token") ? jObject.get("refresh_token").getAsString() : null;           
+	            if (null == accessToken || null == refreshToken)


It is possible that an IDP will return an access token but not a refresh token. If the refresh token is null, that could be OK, it just shouldn't be set in the environment variable.

Yes, It's possible. As in our application, We are showing the use of access and refresh tokens, So let's have both tokens. This sample app shows how we can use the access and refresh token. If any User wants to have code without a refresh token, They can remove the code and use it as it's open-source code. Adding multiple scenarios like this will increase the complexity of the sample app.

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

kevinkarch88 · 2024-05-06T14:50:50Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+					throw new Exception("Access Token is not available.");
+        		}else
+        		{
+        			Connection conn = connectToDB(accessToken);


It would be useful for these steps to print things for the user like "Attempting to connect with connection string: " + connectionString. Or "Executing query: " + query. Things like that.

Added the print msges as per suggestions

kevinkarch88 · 2024-05-06T14:51:47Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+            		break;
+            	}
+            	try {
+					ex.printStackTrace();


I don't think we should be printing the stack trace here or on line 148. At this point in the flow, we are just refreshing the tokens, nothing 'wrong' has heppened.

During compilation, It's giving an error for this code that it has an unhandled exception at this line. Due to this it needs to handle the exception and need to avoid the use of the "System.out.println" (console output) in the code as per discussion with Sandeep. Because of that whenever we need error details, I have used printStackTrace.

When an access token is expired and you get a refresh token, is the stack trace printed out? That would be confusing to the user and what we want to avoid.

Removed printing of stack traces while getting tokens in connect to database.

kevinkarch88 · 2024-05-06T14:53:13Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+	private static ResultSet executeQuery(Connection conn) throws SQLException 
+	{
+		Statement stmt = conn.createStatement();
+		return stmt.executeQuery("SELECT user_id, user_name FROM USERS ORDER BY user_id");


We should print something like "Connected Successfully. The current user is: " + the query result.

Here all the users available in the USERS table are getting printed from database table USERS. Added print to specify "executing query :" + query

- Added Fix for setup of Authentication record to Database. - Uncommented DBSetup and Teardown for Authentication setup before OAuth sample test run

Added DiscoveryUrl to be provided in config.properties file for OAuth Authentication

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

Added RefreshToken check and If both the tokens are available, then it will use those tokens, Or else it will get new tokens using password grant.

Check if access token is not available, Get the tokens. else if a refresh token is not available, Get the tokens. else set both the tokens to properties.

Indentation issue resolved

Indentation resolved

Comment indentation resolved

rvpanchal1202 · 2024-05-08T06:56:05Z

there

Added the logic to retry on failure as per ADO.Net Sample APP

kevinkarch88 · 2024-05-08T13:27:20Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+	// uses the password grant flow
+	private static void EnsureAccessToken() throws Exception 
+	{
+		try 


The added logic in this function is a little off. It should go like this:
If there is an access token, then it needs to be set, else
If there is not an access token and there is no refresh token, then get new tokens by password grant.
If there is not an access token but there is a refresh token, get new tokens by refresh grant.
If that refresh token has expired, then catch the exception and get tokens by password grant.

This logic is in ADO.NET sample application.

-> Code changes as per suggestion are completed.
-> EnsureAccesstoken and connectToDatabase code changed as per the ADO.Net Example.

rvpanchal1202 · 2024-05-10T09:49:29Z

New PR created due to commit issue with current PR 19.
Recent commit is not showing and not updating in the previous PR - #19. I have checked most of the options to do that from the Web but was unable to make it work. Due to some authentication being disabled, I am not able to use git bash, so all the commits and push are done from git-web. Generally, Whenever we Commit to a branch, It should auto-reflect in PR also. All the commits done before today, are auto-reflected other than today's commit.
New PR Link
#23

DMickens reviewed Apr 19, 2024

View reviewed changes

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java Outdated Show resolved Hide resolved

DMickens reviewed Apr 19, 2024

View reviewed changes

williammatherjones requested review from williammatherjones and kevinkarch88 April 19, 2024 13:03

williammatherjones requested changes Apr 19, 2024

View reviewed changes

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java Outdated Show resolved Hide resolved

rvpanchal1202 added 2 commits April 26, 2024 00:17

Update OAuthSampleApp.java

c49ab5d

Commented SetUpDbForOAuth() and TearDown() call.

williammatherjones requested changes Apr 30, 2024

View reviewed changes

kevinkarch88 reviewed May 1, 2024

View reviewed changes

rvpanchal1202 changed the title ~~Update OAuthSampleApp.java, Added refresh token api to get new access…~~ [draft] Update OAuthSampleApp.java, Added refresh token api to get new access… May 3, 2024

kevinkarch88 reviewed May 3, 2024

View reviewed changes

rvpanchal1202 added 6 commits May 6, 2024 11:28

Update OAuthSampleApp.java

cb2a573

-> Added functions for the redundant code for copying data to the buffered output stream. -> removed unnecessary imports.

Update OAuthSampleApp.java

9be665f

Removed ClientID, ClientSecret, Tokenurl from getconnection Call.

Update OAuthSampleApp.java

43faac9

Added Line between imports and class

Update OAuthSampleApp.java

7f9f3fe

Debug prints removed

Update OAuthSampleApp.java

1f1dd1e

Deprecated method use JsonParser().parse(res); removed. used JsonParser.parseString(res);

kevinkarch88 reviewed May 6, 2024

View reviewed changes

rvpanchal1202 added 2 commits May 7, 2024 15:22

Update OAuthSampleApp.java

43569a9

- Added Fix for setup of Authentication record to Database. - Uncommented DBSetup and Teardown for Authentication setup before OAuth sample test run

Update config.properties

e3fb283

Added DiscoveryUrl to be provided in config.properties file for OAuth Authentication

kevinkarch88 reviewed May 7, 2024

View reviewed changes

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java Show resolved Hide resolved

rvpanchal1202 added 5 commits May 8, 2024 09:29

Update OAuthSampleApp.java

24678f1

Added RefreshToken check and If both the tokens are available, then it will use those tokens, Or else it will get new tokens using password grant.

Update OAuthSampleApp.java

e2b9639

Check if access token is not available, Get the tokens. else if a refresh token is not available, Get the tokens. else set both the tokens to properties.

Update OAuthSampleApp.java

bc955dd

Indentation issue resolved

Update OAuthSampleApp.java

222e8f2

Indentation resolved

Update OAuthSampleApp.java

d467034

Comment indentation resolved

rvpanchal1202 closed this May 8, 2024

kevinkarch88 reviewed May 8, 2024

View reviewed changes

[draft] Update OAuthSampleApp.java, Added refresh token api to get new access… #19

[draft] Update OAuthSampleApp.java, Added refresh token api to get new access… #19

Conversation

rvpanchal1202 commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williammatherjones left a comment

Choose a reason for hiding this comment

DMickens commented Apr 19, 2024

williammatherjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvpanchal1202 May 6, 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

rvpanchal1202 May 6, 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

kevinkarch88 commented May 1, 2024

Choose a reason for hiding this comment

rvpanchal1202 May 6, 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

Choose a reason for hiding this comment

rvpanchal1202 commented May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvpanchal1202 commented May 10, 2024

rvpanchal1202 May 6, 2024 •

edited

Loading

rvpanchal1202 May 6, 2024 •

edited

Loading

rvpanchal1202 May 6, 2024 •

edited

Loading

rvpanchal1202 commented May 8, 2024 •

edited

Loading