Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for scopes based on AD group IDs + reject user if required scopes aren't added #145

Merged
merged 14 commits into from May 20, 2024

Conversation

alexluckett
Copy link
Contributor

@alexluckett alexluckett commented May 8, 2024

Scope support

  • Reads AD groups and maps them to scopes in ./designer/server/src/common/helpers/auth/azure-oidc.js. This is then propogated to the cookie-based session and validated on /auth/callback.
  • As a temporary measure until more fine-grained scope support is added around the app, if no scopes are assigned to the user they will be rejected from the login process and their session will be dropped. This means we can easily support extra scopes in the future (e.g. superadmins, guest readers, etc) while having the short term goal met of rejecting users with insufficient privileges.

UI changes

  • A new sign-in screen is the new landing page, replacing the previous "you are signed out" screen
  • If a user has insufficient scopes, an error message is displayed on the sign-in screen. The user will never see that they are logged in, as we log them in, check their scopes, and immediately drop their session if they don't have permission.

image

@alexluckett alexluckett force-pushed the feature/303643-unsuccessful-user-auth branch from 4507dbd to 5aff5b2 Compare May 8, 2024 20:14
designer/package.json Outdated Show resolved Hide resolved
designer/server/src/common/constants/rbac.js Outdated Show resolved Hide resolved
idTokenPayload.groups ?? []
)

credentials.scope = assignedScopes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting an error that credentials.scope isn't typed

Should we be using the scope(request) handler instead?

In the deleted code there seemed to be some nice auth.access.scope examples, should we not try to keep them maintained?

  options: {
    auth: {
      access: {
        scope: ['+mockScope']
      },
      mode: 'required'
    }
  },

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. That function is per request, rather than when the token is received. We only need to grab their groups when the user is signing in or is refreshed. My understanding of the scope handler is where you want to elevate the user's access based on a specific request, which wouldn't be the case here.

credentials.scope is something I'm still trying to work out. It's definitely the right way to do it, I'm a bit puzzled though at the type errors. The docs indicate it's a property of credentials, as does a few guides I found online.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably Bell using its own bespoke request types?

You'll see in #145 (comment) that credentials.scope is typed correctly there?

Be good to move the code into createUserSession() so the compiler can check our homework

      // Create user session
      const credentials = await createUserSession(request)

      if (!credentials?.user) {
        return Boom.unauthorized()
      }
      
+     if (!credentials?.scope) {
+       // Do the flashy auth failed thing
+     }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the deleted code there seemed to be some nice auth.access.scope examples, should we not try to keep them maintained?

Looking at this now. It's a CDP thing but we can use it, might be a good way to keep the route permissions centralised somewhere.

designer/server/src/routes/account/auth.js Outdated Show resolved Hide resolved
designer/server/src/routes/account/auth.test.js Outdated Show resolved Hide resolved
designer/server/src/views/account/sign-in.njk Outdated Show resolved Hide resolved
designer/server/src/models/account/auth.js Outdated Show resolved Hide resolved
designer/server/src/models/account/auth.js Outdated Show resolved Hide resolved
designer/server/src/models/account/auth.js Outdated Show resolved Hide resolved
designer/server/src/common/helpers/auth/azure-oidc.js Outdated Show resolved Hide resolved
@alexluckett alexluckett force-pushed the feature/303643-unsuccessful-user-auth branch from fe0804d to 5d4f3c2 Compare May 9, 2024 13:31
@alexluckett alexluckett marked this pull request as ready for review May 13, 2024 12:45
@alexluckett alexluckett force-pushed the feature/303643-unsuccessful-user-auth branch from 761d01e to 614720b Compare May 13, 2024 14:23
@alexluckett alexluckett force-pushed the feature/303643-unsuccessful-user-auth branch from 41041c2 to 521cac3 Compare May 14, 2024 08:55
designer/server/jest.setup.cjs Outdated Show resolved Hide resolved
designer/server/src/common/constants/scopes.js Outdated Show resolved Hide resolved
@alexluckett alexluckett force-pushed the feature/303643-unsuccessful-user-auth branch 3 times, most recently from 0d62471 to dcc3091 Compare May 16, 2024 15:28
@colinrotherham colinrotherham force-pushed the feature/303643-unsuccessful-user-auth branch from 52ee05d to 56e4c43 Compare May 20, 2024 16:42
@colinrotherham colinrotherham changed the base branch from main to error-list-headings May 20, 2024 16:42
@colinrotherham colinrotherham self-requested a review May 20, 2024 16:45
Base automatically changed from error-list-headings to main May 20, 2024 16:57
@colinrotherham colinrotherham force-pushed the feature/303643-unsuccessful-user-auth branch from 56e4c43 to 46779ee Compare May 20, 2024 17:05
@colinrotherham colinrotherham merged commit 11a3c03 into main May 20, 2024
6 of 8 checks passed
@colinrotherham colinrotherham deleted the feature/303643-unsuccessful-user-auth branch May 20, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants