Refactor way of verifying the authentication type

[funkydev]

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

[codecov[bot]]

Codecov Report

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

:exclamation: 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%)	:arrow_down:
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%)	:arrow_down:
... 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]

@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: https://github.com/tediousjs/tedious/issues/1144#issuecomment-803571215

[funkydev]

@arthurschreiber ready for re-review :D

[arthurschreiber]

@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]

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

[funkydev]

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]

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]

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

[funkydev]

@arthurschreiber I have finished.

Summarizing:

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

[arthurschreiber]

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.