AAD password on non windows #146

xiangyushawn · 2017-02-14T21:04:58Z

No description provided.

codecov-io · 2017-02-14T21:14:32Z

Codecov Report

Merging #146 into dev will decrease coverage by -0.03%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##                dev     #146      +/-   ##
============================================
- Coverage     27.32%   27.29%   -0.03%     
+ Complexity     1142     1140       -2     
============================================
  Files            95       97       +2     
  Lines         23279    23303      +24     
  Branches       3870     3871       +1     
============================================
+ Hits           6360     6361       +1     
- Misses        15641    15661      +20     
- Partials       1278     1281       +3

Flag	Coverage Δ	Complexity Δ
#JDBC41	`27.18% <ø> (-0.06%)`	`1135 <ø> (-3)`
#JDBC42	`27.24% <ø> (-0.02%)`	`1139 <ø> (-1)`

Impacted Files	Coverage Δ	Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerResource.java	`100% <ø> (ø)`	`4 <ø> (ø)`	❌
.../microsoft/sqlserver/jdbc/SQLServerConnection.java	`37.59% <ø> (+0.06%)`	`179 <ø> (ø)`	❌
...microsoft/sqlserver/jdbc/SQLServerADAL4JUtils.java	`0% <ø> (ø)`	`0 <ø> (?)`
.../com/microsoft/sqlserver/jdbc/SqlFedAuthToken.java	`0% <ø> (ø)`	`0 <ø> (?)`
src/main/java/microsoft/sql/DateTimeOffset.java	`37.14% <ø> (-2.86%)`	`8% <ø> (-2%)`
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	`45.81% <ø> (-0.22%)`	`182% <ø> (-1%)`
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	`31.05% <ø> (+0.13%)`	`0% <ø> (ø)`	❌
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java	`39.7% <ø> (+1.47%)`	`10% <ø> (+1%)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3733637...4878161. Read the comment docs.

v-nisidh

Approved with comments.

v-nisidh · 2017-02-14T23:33:12Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java

-                if (authenticationString.trim().equalsIgnoreCase(SqlAuthentication.ActiveDirectoryPassword.toString())) {
-                    dllInfo = AuthenticationJNI.getAccessToken(user, password, fedAuthInfo.stsurl, fedAuthInfo.spn, clientConnectionId.toString(),
-                            ActiveDirectoryAuthentication.jdbcFedauthClientId, expirationFileTime);
+            if (authenticationString.trim().equalsIgnoreCase(SqlAuthentication.ActiveDirectoryPassword.toString())) {


Better to have constant on left side while comparing.

I totally agree, but there are a lot of places to change in the driver, maybe we can have a separate PR for this?

v-nisidh · 2017-02-14T23:36:42Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java

@@ -1270,8 +1285,8 @@ Connection connectInternal(Properties propsIn,
            }

            if ((!System.getProperty("os.name").toLowerCase().startsWith("windows"))
-                    && (!authenticationString.equalsIgnoreCase(SqlAuthentication.NotSpecified.toString()))) {
-                throw new SQLServerException(SQLServerException.getErrString("R_FedAuthOnNonWindows"), null);
+                    && (authenticationString.equalsIgnoreCase(SqlAuthentication.ActiveDirectoryIntegrated.toString()))) {


in getFedAuthToken(..) we have similar check of ActiveDirectoryPassword. There we used authenticationString.trim(). Is there any possibility that we are getting authenticationString as null?

no, the default is NotSpecified, it cannot be null. you can search for this line in the file and you will see:
sPropValue = SQLServerDriverStringProperty.AUTHENTICATION.getDefaultValue();

v-nisidh · 2017-02-14T23:38:28Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java

-                assert null != dllInfo.accessTokenBytes;
+                    // the cause error message uses \\n\\r which does not give correct format
+                    // change it to \r\n to provide correct format
+                    String correctedErrorMessage = e.getCause().getMessage().replaceAll("\\\\r\\\\n", "\r\n");


Do we have any Negative JUnit Test cases ? Which will expect some exception / message / cause.

yes, we have tests for AAD error messages

v-nisidh · 2017-02-15T00:02:32Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java

+            if (authenticationString.trim().equalsIgnoreCase(SqlAuthentication.ActiveDirectoryPassword.toString())) {
+                ExecutorService executorService = Executors.newFixedThreadPool(1);
+                try {
+                    AuthenticationContext context = new AuthenticationContext(fedAuthInfo.stsurl, false, executorService);


SUGGESTION: May be we can have ADALAuth.getAccesstoken() in which we can create AuthenctioanContext and acquireToken.

good suggestion. It's done now, I have used modular approach now

v-nisidh · 2017-02-15T19:53:03Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerADAL4JUtils.java

+import com.microsoft.sqlserver.jdbc.SQLServerConnection.ActiveDirectoryAuthentication;
+import com.microsoft.sqlserver.jdbc.SQLServerConnection.SqlFedAuthInfo;
+
+class SQLServerADAL4JUtils {


Are you sure you do not want public class? By default this will use package level visibility.

why would we want it to be public?

v-nisidh · 2017-02-15T19:56:20Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java

-        private static final int GetAccessTokenTansisentError = 2;
-        private static final int GetAccessTokenOtherError = 3;
+    class ActiveDirectoryAuthentication {
+        static final String jdbcFedauthClientId = "7f98cb04-cd1e-40df-9140-3bf7e2cea4db";


final static instance variables should be in CAPS

v-nisidh

Approved. Having 2 review comments.

v-nisidh

Approved.

xiangyushawn added 5 commits February 10, 2017 13:50

basic logic for AAD Password using ADAL4J is done

397eca0

adding adal4j dependency in pom file

48e166a

worked on error handling

7ba7ff7

match the exception tree before error message correction

a467b94

only AAD integrated is restricted to Windows OS

1a80f96

ajlam changed the title ~~AAD password on non wiondows~~ AAD password on non windows Feb 14, 2017

v-nisidh approved these changes Feb 15, 2017

View reviewed changes

use modular approach for AAD Password with ADAL4J

2c33841

v-nisidh reviewed Feb 15, 2017

View reviewed changes

v-nisidh approved these changes Feb 15, 2017

View reviewed changes

final static instance variables should be in CAPS

4878161

v-nisidh approved these changes Feb 15, 2017

View reviewed changes

xiangyushawn merged commit d6f1000 into microsoft:dev Feb 15, 2017

xiangyushawn deleted the AAD-Password-on-non-Wiondows branch February 15, 2017 21:06

v-nisidh added this to the 6.1.4 milestone Feb 15, 2017

ajlam mentioned this pull request Mar 30, 2017

What is up with the Azure dependency? #227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AAD password on non windows #146

AAD password on non windows #146

xiangyushawn commented Feb 14, 2017

codecov-io commented Feb 14, 2017 •

edited

Loading

v-nisidh left a comment

v-nisidh Feb 14, 2017

xiangyushawn Feb 15, 2017

v-nisidh Feb 14, 2017

xiangyushawn Feb 15, 2017 •

edited

Loading

v-nisidh Feb 14, 2017

xiangyushawn Feb 15, 2017

v-nisidh Feb 15, 2017

xiangyushawn Feb 15, 2017

v-nisidh Feb 15, 2017

xiangyushawn Feb 15, 2017

v-nisidh Feb 15, 2017 •

edited

Loading

xiangyushawn Feb 15, 2017

v-nisidh left a comment

v-nisidh left a comment

AAD password on non windows #146

AAD password on non windows #146

Conversation

xiangyushawn commented Feb 14, 2017

codecov-io commented Feb 14, 2017 • edited Loading

Codecov Report

v-nisidh left a 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

xiangyushawn Feb 15, 2017 • 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

v-nisidh Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v-nisidh left a comment

Choose a reason for hiding this comment

v-nisidh left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 14, 2017 •

edited

Loading

xiangyushawn Feb 15, 2017 •

edited

Loading

v-nisidh Feb 15, 2017 •

edited

Loading