AAD password on non windows by xiangyushawn · Pull Request #146 · microsoft/mssql-jdbc

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

-                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

            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

-                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

+            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

+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

-        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

Conversation

xiangyushawn commented Feb 14, 2017

Uh oh!

codecov-io commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

v-nisidh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangyushawn Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-nisidh Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-nisidh left a comment

Choose a reason for hiding this comment

Uh oh!

v-nisidh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Feb 14, 2017 •

edited

Loading

xiangyushawn Feb 15, 2017 •

edited

Loading

v-nisidh Feb 15, 2017 •

edited

Loading