Merge pull request #534 from bheesham/use-aal-from-matched-authz-rule

fix(access): Use AAL from the matched authz rule

Author: bheesham

tf/actions/accessRules.js

@@ -200,19 +200,32 @@ exports.onExecutePostLogin = async (event, api) => {
     };
   };
 
-  // Process the access cache decision
+  // Process the access cache decision.
+  // Note that applications may be defined multiple times in the access rules.
+  //
+  // When access is granted, the AAL from the rule is used, meaning different
+  // groups can be subjected to different MFA requirements.
+  //
+  // The one exception is that: if any apps say _no_ users nor groups should
+  // have access, then we bail early.
   const access_decision = (groups, access_rules, access_file_conf) => {
     // This is used for authorized user/groups
     let authorized = false;
 
     // Defaut app requested aal to MEDIUM for all apps which do not have
     // this set in access file
-    let required_aal = "MEDIUM";
+    const default_aal = "MEDIUM";
 
-    const apps = access_rules.filter(
-      (a) =>
-        (a.application.client_id ?? "").indexOf(event.client.client_id) >= 0
-    );
+    // The AAL from the matched authorization rule is used.
+    let required_aal;
+
+    // Only look at rules which match our client_id.
+    const apps = access_rules
+      .filter(
+        (a) =>
+          (a.application.client_id ?? "").indexOf(event.client.client_id) >= 0
+      )
+      .map((a) => a.application);
 
     // Default deny for apps we don't define in
     // https://github.com/mozilla-iam/sso-dashboard-configuration/blob/master/apps.yml
@@ -221,68 +234,66 @@ exports.onExecutePostLogin = async (event, api) => {
       return deny("notingroup");
     }
 
-    // Check users and groups.
-    for (let i = 0; i < apps.length; i++) {
-      let app = apps[i].application;
+    // XXX This needs to be fixed in the dashboard first. Empty users
+    // or groups (length == 0) means no access in the dashboard
+    // apps.yml world.
+    const deny_all =
+      apps.find(
+        (a) =>
+          a.authorized_users.length === 0 && a.authorized_groups.length === 0
+      ) !== undefined;
+    if (deny_all) {
+      console.log(
+        `Access denied to ${event.client.client_id} for user ` +
+          `${event.user.email} (${event.user.user_id})` +
+          ` - this app denies ALL users and ALL groups")`
+      );
+      return deny("notingroup");
+    }
 
+    // Check users and groups.
+    for (const app of apps) {
       //Handy for quick testing in dev (overrides access rules)
       //var app = {'client_id': 'pCGEHXW0VQNrQKURDcGi0tghh7NwWGhW', // This is testrp social-ldap-pwless
       //           'authorized_users': ['gdestuynder@mozilla.com'],
       //           'authorized_groups': ['okta_mfa'],
       //           'aal': 'LOW'
       //          };
 
-      if (app.client_id && app.client_id.indexOf(event.client.client_id) >= 0) {
-        // If there are multiple applications in apps.yml with the same
-        // client_id then this expiration of access check will only run
-        // against the first one encountered. This matters if there are
-        // multiple applications, using the same client_id, and asserting
-        // different expire_access_when_unused_after values.
-
-        // Set app AAL (AA level) if present
-        required_aal = app.AAL || required_aal;
-
-        // AUTHORIZED_{GROUPS,USERS}
-        //
-        // XXX this authorized_users SHOULD BE REMOVED as it's unsafe (too
-        // easy to make mistakes). USE GROUPS.
-        //
-        // XXX This needs to be fixed in the dashboard first. Empty users
-        // or groups (length == 0) means no access in the dashboard
-        // apps.yml world.
-        if (
-          app.authorized_users.length === 0 &&
-          app.authorized_groups.length === 0
-        ) {
-          const msg =
-            `Access denied to ${event.client.client_id} for user ${event.user.email} (${event.user.user_id})` +
-            ` - this app denies ALL users and ALL groups")`;
-          console.log(msg);
-          return deny("notingroup");
-        }
+      // AUTHORIZED_{GROUPS,USERS}
+      //
+      // XXX this authorized_users SHOULD BE REMOVED as it's unsafe (too
+      // easy to make mistakes). USE GROUPS.
 
-        // Check if the user is authorized to access
-        // A user is authorized if they are a member of any authorized_groups or if they are one of the authorized_users
-        if (
-          app.authorized_users.length > 0 &&
-          app.authorized_users.indexOf(event.user.email) >= 0
-        ) {
-          authorized ||= true;
-          // Same dance as above, but for groups
-        } else if (
-          app.authorized_groups.length > 0 &&
-          hasCommonElements(app.authorized_groups, groups)
-        ) {
-          authorized ||= true;
-        }
-      } // correct client id / we matched the current RP
+      // Check if the user is authorized to access.
+      // A user is authorized if they are a member of any authorized_groups
+      // or if they are one of the authorized_users.
+      if (
+        app.authorized_users.length > 0 &&
+        app.authorized_users.indexOf(event.user.email) >= 0
+      ) {
+        console.log(`${event.user.user_id} was in authorized_users`);
+        required_aal = app.AAL || default_aal;
+        authorized = true;
+        break;
+        // Same dance as above, but for groups
+      } else if (
+        app.authorized_groups.length > 0 &&
+        hasCommonElements(app.authorized_groups, groups)
+      ) {
+        console.log(`${event.user.user_id} was in authorized_groups`);
+        required_aal = app.AAL || default_aal;
+        authorized = true;
+        break;
+      }
     } // for loop / next rule in apps.yml
 
     if (!authorized) {
-      const msg =
-        `Access denied to ${event.client.client_id} for user ${event.user.email} (${event.user.user_id})` +
-        ` - not in authorized group or not an authorized user`;
-      console.log(msg);
+      console.log(
+        `Access denied to ${event.client.client_id} for user ` +
+          `${event.user.email} (${event.user.user_id}) - not in ` +
+          "authorized group or not an authorized user"
+      );
       return deny("notingroup");
     }
 

tf/tests/accessRules.test.js

@@ -627,27 +627,32 @@ describe("Client is defined in apps.yml as client00000000000000000000000008", ()
 });
 
 describe("Client is defined multiple times in apps.yml as client00000000000000000000000009", () => {
-  test("User in restricted_group_1; expect allowed", async () => {
+  test("User in restricted_group_1 without 2FA; expect allowed", async () => {
+    // This entry in apps.yml does not require MFA.
     _event.client.client_id = "client00000000000000000000000009";
     _event.connection.name = "google-oauth2";
     _event.user.groups = ["restricted_group_1"];
     _event.user.ldap_groups = [];
     _event.user.app_metadata.groups = [];
     await onExecutePostLogin(_event, api);
+    expect(api.multifactor.enable).not.toHaveBeenCalled();
     expect(_event.transaction.redirect_uri).toEqual(undefined);
   });
-  test("User in restricted_group_2; expect allowed", async () => {
+  test("User in restricted_group_2 with 2FA; expect allowed", async () => {
     _event.client.client_id = "client00000000000000000000000009";
-    _event.connection.name = "google-oauth2";
+    _event.connection.strategy = "ad";
+    _event.user.multifactor = ["duo"];
     _event.user.groups = ["restricted_group_2"];
     _event.user.ldap_groups = [];
     _event.user.app_metadata.groups = [];
     await onExecutePostLogin(_event, api);
+    expect(api.multifactor.enable).toHaveBeenCalled();
     expect(_event.transaction.redirect_uri).toEqual(undefined);
   });
-  test("User in restricted_group_3; expect denied", async () => {
+  test("User no allowed groups with 2FA; expect denied", async () => {
     _event.client.client_id = "client00000000000000000000000009";
-    _event.connection.name = "google-oauth2";
+    _event.connection.strategy = "ad";
+    _event.user.multifactor = ["duo"];
     _event.user.groups = ["restricted_group_3"];
     _event.user.ldap_groups = [];
     _event.user.app_metadata.groups = [];

tf/tests/modules/apps.yml.js

@@ -63,11 +63,13 @@ apps:
     - team_mofo
     authorized_users: []
 - application:
+    AAL: LOW
     authorized_groups:
     - restricted_group_1
     authorized_users: []
     client_id: client00000000000000000000000009
 - application:
+    AAL: MEDIUM
     authorized_groups:
     - restricted_group_2
     authorized_users: []