Refactor way of verifying the authentication type #1242

funkydev · 2021-03-21T11:58:44Z

export authentication interfaces
create enum with supported authentication types
simplify conditions for verifying authentication types

codecov · 2021-03-21T12:00:30Z

Codecov Report

Merging #1242 (75471ba) into master (f6e1681) will decrease coverage by 0.83%.
The diff coverage is 71.69%.

❗ Current head 75471ba differs from pull request most recent head 1043496. Consider uploading reports for the commit 1043496 to get more accurate results

@@            Coverage Diff             @@
##           master    #1242      +/-   ##
==========================================
- Coverage   81.52%   80.69%   -0.84%     
==========================================
  Files          85       93       +8     
  Lines        4536     4584      +48     
  Branches      846      844       -2     
==========================================
+ Hits         3698     3699       +1     
- Misses        574      632      +58     
+ Partials      264      253      -11

Impacted Files	Coverage Δ
src/connection.ts	`63.41% <11.76%> (-3.56%)`	⬇️
src/authentication/authentication-types.ts	`100.00% <100.00%> (ø)`
...hentication/azure-active-directory-access-token.ts	`100.00% <100.00%> (ø)`
...tication/azure-active-directory-msi-app-service.ts	`100.00% <100.00%> (ø)`
...rc/authentication/azure-active-directory-msi-vm.ts	`100.00% <100.00%> (ø)`
.../authentication/azure-active-directory-password.ts	`100.00% <100.00%> (ø)`
...azure-active-directory-service-principal-secret.ts	`100.00% <100.00%> (ø)`
src/authentication/default.ts	`100.00% <100.00%> (ø)`
src/authentication/ntlm.ts	`100.00% <100.00%> (ø)`
src/token/env-change-token-parser.ts	`55.81% <0.00%> (-23.26%)`	⬇️
... and 12 more

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 f6e1681...1043496. Read the comment docs.

funkydev · 2021-03-21T12:24:50Z

@arthurschreiber Could you look up to my refactor changes? I'm on the way to implement a new authentication type, but first, I'd like to clean up the code.

EDIT:
For more details, you can read my comment here: #1144 (comment)

src/connection.ts

- export authentication interfaces - create enum with authentication types - simplify conditions for verifying authentication types

funkydev · 2021-03-21T13:23:50Z

@arthurschreiber ready for re-review :D

arthurschreiber · 2021-03-21T14:23:04Z

@funkydev Here's a diff of the changes I'd propose:

diff --git a/src/connection.ts b/src/connection.ts
index 68eb02a6..61869a90 100644
--- a/src/connection.ts
+++ b/src/connection.ts
@@ -196,33 +196,49 @@ const DEFAULT_LANGUAGE = 'us_english';
  */
 const DEFAULT_DATEFORMAT = 'mdy';
 
-export enum AuthenticationTypes {
-  AzureActiveDirectoryMsiAppService = 'azure-active-directory-msi-app-service',
-  AzureActiveDirectoryMsiVm = 'azure-active-directory-msi-vm',
-  AzureActiveDirectoryAccessToken = 'azure-active-directory-access-token',
-  AzureActiveDirectoryPassword = 'azure-active-directory-password',
-  AzureActiveDirectoryServicePrincipalSecret = 'azure-active-directory-service-principal-secret',
-  Ntlm = 'ntlm',
-  Default = 'default',
+export const AuthenticationTypes = Object.freeze({
+  AzureActiveDirectoryMsiAppService: 'azure-active-directory-msi-app-service',
+  AzureActiveDirectoryMsiVm: 'azure-active-directory-msi-vm',
+  AzureActiveDirectoryAccessToken: 'azure-active-directory-access-token',
+  AzureActiveDirectoryPassword: 'azure-active-directory-password',
+  AzureActiveDirectoryServicePrincipalSecret: 'azure-active-directory-service-principal-secret',
+  Ntlm: 'ntlm',
+  Default: 'default',
+} as const);
+
+type FedAuthAuthenticationType =
+  typeof AuthenticationTypes.AzureActiveDirectoryPassword |
+  typeof AuthenticationTypes.AzureActiveDirectoryMsiVm |
+  typeof AuthenticationTypes.AzureActiveDirectoryMsiAppService |
+  typeof AuthenticationTypes.AzureActiveDirectoryServicePrincipalSecret;
+
+type AADAuthenticationType = typeof AuthenticationTypes.AzureActiveDirectoryAccessToken | FedAuthAuthenticationType;
+
+function isFedAuthAuthenticationType(type: AuthenticationType): type is FedAuthAuthenticationType {
+  switch (type) {
+    case AuthenticationTypes.AzureActiveDirectoryAccessToken:
+    case AuthenticationTypes.AzureActiveDirectoryMsiVm:
+    case AuthenticationTypes.AzureActiveDirectoryMsiAppService:
+    case AuthenticationTypes.AzureActiveDirectoryServicePrincipalSecret:
+      return true;
+
+    default:
+      return false;
+  }
 }
 
-const AADAuthenticationTypes = [
-  AuthenticationTypes.AzureActiveDirectoryPassword,
-  AuthenticationTypes.AzureActiveDirectoryAccessToken,
-  AuthenticationTypes.AzureActiveDirectoryMsiVm,
-  AuthenticationTypes.AzureActiveDirectoryMsiAppService,
-  AuthenticationTypes.AzureActiveDirectoryServicePrincipalSecret,
-];
-
-const FedAuthAuthentications = [
-  AuthenticationTypes.AzureActiveDirectoryPassword,
-  AuthenticationTypes.AzureActiveDirectoryMsiVm,
-  AuthenticationTypes.AzureActiveDirectoryMsiAppService,
-  AuthenticationTypes.AzureActiveDirectoryServicePrincipalSecret,
-];
+function isAADAuthenticationType(type: AuthenticationType): type is AADAuthenticationType {
+  switch (type) {
+    case AuthenticationTypes.AzureActiveDirectoryAccessToken:
+      return true;
+
+    default:
+      return isFedAuthAuthenticationType(type);
+  }
+}
 
 export interface AzureActiveDirectoryMsiAppServiceAuthentication {
-  type: AuthenticationTypes.AzureActiveDirectoryMsiAppService;
+  type: typeof AuthenticationTypes.AzureActiveDirectoryMsiAppService;
   options: {
     /**
      * If you user want to connect to an Azure app service using a specific client account
@@ -243,7 +259,7 @@ export interface AzureActiveDirectoryMsiAppServiceAuthentication {
 }
 
 export interface AzureActiveDirectoryMsiVmAuthentication {
-  type: AuthenticationTypes.AzureActiveDirectoryMsiVm;
+  type: typeof AuthenticationTypes.AzureActiveDirectoryMsiVm;
   options: {
     /**
      * If you user want to connect to an Azure app service using a specific client account
@@ -260,7 +276,7 @@ export interface AzureActiveDirectoryMsiVmAuthentication {
 }
 
 export interface AzureActiveDirectoryAccessTokenAuthentication {
-  type: AuthenticationTypes.AzureActiveDirectoryAccessToken;
+  type: typeof AuthenticationTypes.AzureActiveDirectoryAccessToken;
   options: {
     /**
      * A user need to provide `token` which they retrived else where
@@ -271,7 +287,7 @@ export interface AzureActiveDirectoryAccessTokenAuthentication {
 }
 
 export interface AzureActiveDirectoryPasswordAuthentication {
-  type: AuthenticationTypes.AzureActiveDirectoryPassword;
+  type: typeof AuthenticationTypes.AzureActiveDirectoryPassword;
   options: {
     /**
      * A user need to provide `userName` asscoiate to their account.
@@ -290,7 +306,7 @@ export interface AzureActiveDirectoryPasswordAuthentication {
 }
 
 export interface AzureActiveDirectoryServicePrincipalSecret {
-  type: AuthenticationTypes.AzureActiveDirectoryServicePrincipalSecret;
+  type: typeof AuthenticationTypes.AzureActiveDirectoryServicePrincipalSecret;
   options: {
     /**
      * Application (`client`) ID from your registered Azure application
@@ -308,7 +324,7 @@ export interface AzureActiveDirectoryServicePrincipalSecret {
 }
 
 export interface NtlmAuthentication {
-  type: AuthenticationTypes.Ntlm;
+  type: typeof AuthenticationTypes.Ntlm;
   options: {
     /**
      * User name from your windows account.
@@ -328,7 +344,7 @@ export interface NtlmAuthentication {
 }
 
 export interface DefaultAuthentication {
-  type: AuthenticationTypes.Default;
+  type: typeof AuthenticationTypes.Default;
   options: {
     /**
      * User name to use for sql server login.
@@ -3365,7 +3381,7 @@ Connection.prototype.STATE = {
 
             const { authentication } = this.config;
 
-            if (FedAuthAuthentications.includes(authentication.type)) {
+            if (isFedAuthAuthenticationType(authentication.type)) {
               this.transitionTo(this.STATE.SENT_LOGIN7_WITH_FEDAUTH);
             } else if (authentication.type === AuthenticationTypes.Ntlm) {
               this.transitionTo(this.STATE.SENT_LOGIN7_WITH_NTLM);
@@ -3389,7 +3405,7 @@ Connection.prototype.STATE = {
       featureExtAck: function(token) {
         const { authentication } = this.config;
 
-        if (AADAuthenticationTypes.includes(authentication.type)) {
+        if (isAADAuthenticationType(authentication.type)) {
           if (token.fedAuth === undefined) {
             this.loginError = ConnectionError('Did not receive Active Directory authentication acknowledgement');
             this.loggedIn = false;

It changes AuthenticationTypes back to an object, and uses the typeof operator to get the correct type in the places where a property is used and we want to keep the type intact.

It also replaces the AADAuthenticationTypes and FedAuthAuthentications arrays I proposed with two functions that allow checking the same condition, but without having an array defined and with type propagation. What do you think?

arthurschreiber · 2021-03-21T14:24:52Z

One other thing I'm wondering - do we even want to export AuthenticationTypes? 🤔

funkydev · 2021-03-21T14:45:19Z

Let me check that. I saw some castings, and I need to check if your proposition covers all of them.

One other thing I'm wondering - do we even want to export AuthenticationTypes? 🤔

Because we export the interfaces of authentication types (and we should do that), so we should do the same with authentication types. That would allow developers to use tedious internal constants instead of hardcoding strings.

Additionally we could also move authentication types (constants and interfaces) to a separate folder to organize code and decrease lines in that huge file.

funkydev · 2021-03-21T14:49:46Z

That would allow to make the following files per authentication type:

// Contents of /src/authentication/azure-active-directory-msi-app-service.ts

export const AzureActiveDirectoryMsiAppServiceType = 'azure-active-directory-msi-app-service';

export interface AzureActiveDirectoryMsiAppServiceAuthentication {
  type: typeof AzureActiveDirectoryMsiAppServiceType;
  options: {
    /**
     * If you user want to connect to an Azure app service using a specific client account
     * they need to provide `clientId` asscoiate to their created idnetity.
     *
     * This is optional for retrieve token from azure web app service
     */
    clientId?: string;
    /**
     * A msi app service environment need to provide `msiEndpoint` for retriving the accesstoken.
     */
    msiEndpoint?: string;
    /**
     * A msi app service environment need to provide `msiSecret` for retriving the accesstoken.
     */
    msiSecret?: string;
  };
}

funkydev · 2021-03-21T14:56:40Z

Finally, it would allow keeping the validation methods of each type in dedicated files instead of one huge file.

- Cleanup imports - Refactor Authentication type - Add isSupportedAuthenticationType function for validating supported authentication types - Extract authentication types validation to separate functions

…thentication type

funkydev · 2021-03-21T17:19:25Z

@arthurschreiber I have finished.

Summarizing:

There is a SupportedAuthenticationTypes that is used for storing supported authentication types. It is a constant, as you asked.
I created FedAuthAuthenticationType, AADAuthenticationType, AuthenticationType types;
I created isSupportedAuthenticationType, isFedAuthAuthenticationType, isAADAuthenticationType methods for validating authentication types
I extracted all authentications to their files, and I created methods for validating and parsing options depends on the authentication type
I added tests for authentications 😎

…onType as default

arthurschreiber · 2021-03-25T09:41:11Z

Thanks for preparing this PR! I haven't gotten around to reviewing this again, but I'll try to find some time over the coming days.

funkydev mentioned this pull request Mar 21, 2021

azure-active-directory-access-token REFRESH conn automatically on expiring #1144

Open

arthurschreiber reviewed Mar 21, 2021

View reviewed changes

src/connection.ts Outdated Show resolved Hide resolved

arthurschreiber reviewed Mar 21, 2021

View reviewed changes

src/connection.ts Outdated Show resolved Hide resolved

funkydev force-pushed the feature/azure-identity branch from 1161148 to ec73059 Compare March 21, 2021 12:54

arthurschreiber reviewed Mar 21, 2021

View reviewed changes

src/connection.ts Outdated Show resolved Hide resolved

funkydev force-pushed the feature/azure-identity branch from ec73059 to 4c26744 Compare March 21, 2021 13:11

refactor: refactor authentication types

cd73e5a

- export authentication interfaces - create enum with authentication types - simplify conditions for verifying authentication types

funkydev force-pushed the feature/azure-identity branch from 4c26744 to cd73e5a Compare March 21, 2021 13:13

refactor: rename FedAuthAuthentications to FedAuthAuthenticationTypes

d7ed8d3

refactor: extract authentication to separate folder

7c0baa4

- Cleanup imports - Refactor Authentication type - Add isSupportedAuthenticationType function for validating supported authentication types - Extract authentication types validation to separate functions

funkydev force-pushed the feature/azure-identity branch from 5c55781 to cba905e Compare March 21, 2021 16:19

funkydev marked this pull request as draft March 21, 2021 16:19

feat: add validation for domain in azure-active-directory-password au…

9a765ba

…thentication type

funkydev force-pushed the feature/azure-identity branch from cba905e to 33a613e Compare March 21, 2021 17:09

funkydev marked this pull request as ready for review March 21, 2021 17:10

test: test authentication and authentication types methods

97cd35c

funkydev force-pushed the feature/azure-identity branch from 33a613e to 97cd35c Compare March 21, 2021 17:14

refactor: update isAADAuthenticationType to use isFedAuthAuthenticati…

a0174a5

…onType as default

funkydev force-pushed the feature/azure-identity branch from 6244d71 to a0174a5 Compare March 21, 2021 17:45

funkydev added 2 commits March 21, 2021 19:57

feat: replace skipped ntlm raw usages

e83977d

test: run test/unit/authentication on test or test-all

1043496

architshukla mentioned this pull request Jul 10, 2021

Authentication with Azure CLI #1284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor way of verifying the authentication type #1242

Refactor way of verifying the authentication type #1242

funkydev commented Mar 21, 2021

codecov bot commented Mar 21, 2021 •

edited

Loading

funkydev commented Mar 21, 2021 •

edited

Loading

funkydev commented Mar 21, 2021

arthurschreiber commented Mar 21, 2021

arthurschreiber commented Mar 21, 2021

funkydev commented Mar 21, 2021

funkydev commented Mar 21, 2021 •

edited

Loading

funkydev commented Mar 21, 2021

funkydev commented Mar 21, 2021

arthurschreiber commented Mar 25, 2021

Refactor way of verifying the authentication type #1242

Are you sure you want to change the base?

Refactor way of verifying the authentication type #1242

Conversation

funkydev commented Mar 21, 2021

codecov bot commented Mar 21, 2021 • edited Loading

Codecov Report

funkydev commented Mar 21, 2021 • edited Loading

funkydev commented Mar 21, 2021

arthurschreiber commented Mar 21, 2021

arthurschreiber commented Mar 21, 2021

funkydev commented Mar 21, 2021

funkydev commented Mar 21, 2021 • edited Loading

funkydev commented Mar 21, 2021

funkydev commented Mar 21, 2021

arthurschreiber commented Mar 25, 2021

codecov bot commented Mar 21, 2021 •

edited

Loading

funkydev commented Mar 21, 2021 •

edited

Loading

funkydev commented Mar 21, 2021 •

edited

Loading