8000 WIP: Group assignment from JWT claims by EternalDeiwos · Pull Request #2568 · outline/outline · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

WIP: Group assignment from JWT claims #2568

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

WIP: Group assignment from JWT claims #2568

wants to merge 3 commits into from

Conversation

EternalDeiwos
Copy link
Contributor

This is a natural follow-on from #2388 and was initially floated in discussions #2205 and #2206. This PR is a WIP with some pretty obviously missing pieces, specifically error handling, .env.sample examples, and tests, which I will gladly add but wanted to be sure that how I'm going about this is acceptable before adding the polish.

Additions

  1. The provider passes a configurable groups claim to the accountProvisioner. If no claim is present then there is no effect.
  2. The claim name is configurable by specifying an environment variable. This accounts for multiple IdPs that might use a claim named roles instead of groups or anything similar.
  3. The claim content can either be a JSON array, or a string of comma-separated group names. This, once again, accounts for the majority of ways identity providers will provide roles/group names.
  4. The accountProvisioner will pass the groups to a new groupCreator file in /commands.
  5. The groupCreator first ensures that the group exists, and if not will create it.
  6. The groupCreator will then ensure that the user is a part of the group, and if not will add them.
  7. The accountProvisioner will then remove any GroupUser records that are not present in the current list of groups passed to it; currently this includes groups that were assigned manually, however if no groups are passed to the accountProvisioner then it will not remove any group assignments, and therefore should not interfere with existing setups. It will also not delete any group, even if there are no members.

Configuration

  • OIDC_GROUPS_CLAIM — The name of the claim provided that contains the list of groups a user belongs to. Currently defaults to groups although we may wish to have it default to a disabled state to guarantee that existing deployments will not be affected by stray configuration.

Further configuration will be necessary from the IdP side; as this likely requires additional scopes that will add the required claim referenced in the env variable above. For example, on Keycloak this would require creating a "mapper" that has that effect.

@EternalDeiwos EternalDeiwos marked this pull request as draft September 16, 2021 14:15
@dmezh dmezh mentioned this pull request Oct 3, 2021
Closed
@almereyda almereyda mentioned this pull request Nov 16, 2021
Closed
@almereyda
Copy link

This seems to be mostly working.

Similar to the experience described in #2555 (comment), there is a reproducible timeout that happens at some point during the groupCreator flow, where it seems some database request is not handled properly. It now happened again in another environment:

ConnectionAcquireTimeoutError [SequelizeConnectionAcquireTimeoutError]: Operation timeout
    at ConnectionManager.getConnection (/opt/outline/node_modules/sequelize/lib/dialects/abstract/connection-manager.js:288:48)
    at runMicrotasks (<anonymous>)
    at runNextTicks (internal/process/task_queues.js:60:5)
    at listOnTimeout (internal/timers.js:526:9)
    at processTimers (internal/timers.js:500:7)
    at async Transaction.prepareEnvironment (/opt/outline/node_modules/sequelize/lib/transaction.js:119:24)
    at async Sequelize.transaction (/opt/outline/node_modules/sequelize/lib/sequelize.js:1082:7)
    at async groupCreator (/opt/outline/build/server/commands/groupCreator.js:33:23)
    at async Promise.all (index 9)
    at async accountProvisioner (/opt/outline/build/server/commands/accountProvisioner.js:132:14)
    at async OAuth2Strategy._verify (/opt/outline/build/server/routes/auth/providers/oidc.js:100:22) {
  parent: TimeoutError: Operation timeout
      at Timeout.<anonymous> (/opt/outline/node_modules/sequelize-pool/lib/Deferred.js:17:25)
      at listOnTimeout (internal/timers.js:557:17)
      at processTimers (internal/timers.js:500:7),
  original: TimeoutError: Operation timeout
      at Timeout.<anonymous> (/opt/outline/node_modules/sequelize-pool/lib/Deferred.js:17:25)
      at listOnTimeout (internal/timers.js:557:17)
      at processTimers (internal/timers.js:500:7)
}
TypeError: Cannot read property 'map' of undefined
    at accountProvisioner (/opt/outline/build/server/commands/accountProvisioner.js:151:32)
    at runMicrotasks (<anonymous>)
    at runNextTicks (internal/process/task_queues.js:60:5)
    at listOnTimeout (internal/timers.js:526:9)
    at processTimers (internal/timers.js:500:7)
    at async OAuth2Strategy._verify (/opt/outline/build/server/routes/auth/providers/oidc.js:100:22)

And also, as in the previous test, groups are being created, but it is not clear to me how the ones created are chosen, or why there are only three groups from many that make it to Outline. My best quess is the groups field in the response contains the groups without any specific order, and the code here only treats the first three occurencies.

It's already good work that we find here, because it works partly as expected, and I would like to offer my assistance, in case we wanted to bring this draft to a successful merger.

@stale
Copy link
stale bot commented Mar 16, 2022

Hey! The issue has been automatically marked as stale because it has not had recent activity. It will be closed soon if no further activity occurs. Please reply here if you wish for the issue to be kept open.

@stale stale bot added the stale label Mar 16, 2022
@stale stale bot closed this Mar 31, 2022
@NexZhu
Copy link
NexZhu commented Apr 15, 2022

Hope this will still land

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EternalDeiwos @almereyda @NexZhu
0