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

Ability to map exisitng LDAP SSO groups to teams #4768

Closed
hardillb opened this issue Nov 13, 2024 · 6 comments · Fixed by #4902
Closed

Ability to map exisitng LDAP SSO groups to teams #4768

hardillb opened this issue Nov 13, 2024 · 6 comments · Fixed by #4902
Assignees
Labels
customer request requested by customer feature-request New feature or request that needs to be turned into Epic/Story details needs-triage Needs looking at to decide what to do size:M - 3 Sizing estimation point
Milestone

Comments

@hardillb
Copy link
Contributor

hardillb commented Nov 13, 2024

Description

Customer has hard rules on LDAP group names, not allowed to match ff-[team-slug]-[role] group names.

Would like a way to explicitly set group name for a given team

Which customers would this be available to

Enterprise Tier Only (EE)

Have you provided an initial effort estimate for this issue?

I have provided an initial effort estimate

@hardillb hardillb added feature-request New feature or request that needs to be turned into Epic/Story details needs-triage Needs looking at to decide what to do size:M - 3 Sizing estimation point labels Nov 13, 2024
@knolleary knolleary added the customer request requested by customer label Nov 13, 2024
@hardillb
Copy link
Contributor Author

Suggestion from customer

If we could predefine the length of the prefix and postfix for the AD group in FlowFuse, you could probably add a process to ignore them in your system. This should be an application-level configuration, of course, which means it should be configured once for your organization and then followed.

For example,
FlowFuse group name: ff-team1-owner
Prefix length: 8
postfix length: 0
AD group name: abc_grt_ff-team1-owner
or
Prefix length: 4
postfix length: 2
AD group name: abc_ff-team1-owner_1

@joepavitt joepavitt added this to the 2.12 milestone Nov 19, 2024
@joepavitt joepavitt moved this to Scheduled in ☁️ Product Planning Nov 19, 2024
@joepavitt joepavitt moved this to Todo in 🛠 Development Nov 19, 2024
@hardillb hardillb moved this from Todo to In Design in 🛠 Development Dec 11, 2024
@hardillb hardillb self-assigned this Dec 11, 2024
@hardillb
Copy link
Contributor Author

hardillb commented Dec 11, 2024

I think the prefix/postfix option is actually a most practical solution.

It means we don't need to keep a table of group to team/role mappings

Something like this in

async function updateTeamMembership (samlUser, user, providerOpts) {

            groupAssertions.forEach(ga => {
                // Trim prefix/postfix from group name
                let shortGA = ga.slice(providerOpts.groupPrefixLength, (providerOpts.groupPostfixLength * -1))
                // Parse the group name - format: 'ff-SLUG-ROLE'
                // Generate a slug->role object (desiredTeamMemberships)
                const match = /^ff-(.+)-([^-]+)$/.exec(shortGA)
                if (match) {
                    const teamSlug = match[1]
                    const teamRoleName = match[2]
                    const teamRole = Roles[teamRoleName]
                    // Check this role is a valid team role

Or the same in LDAP code

async function updateTeamMembershipLDAP (adminClient, user, userDN, providerOpts) {

        app.log.debug(`LDAP Groups for ${user.username} ${JSON.stringify(searchEntries)}`)
        const promises = []
        let adminGroup = false
        const desiredTeamMemberships = {}
        const groupRegEx = /^ff-(.+)-([^-]+)$/
        for (const i in searchEntries) {
            const shortCN = searchEntries[i].cn.slice(providerOpts.groupPrefixLength, (providerOpts.PostfixLength * -1))
            const match = groupRegEx.exec(shortCN)
            if (match) {
                app.log.debug(`Found group ${searchEntries[i].cn} for user ${user.username}`)
                const teamSlug = match[1]
                const teamRoleName = match[2]
                const teamRole = Roles[teamRoleName]
                // Check this role is a valid team role
                if (TeamRoles.includes(teamRole)) {

@hardillb
Copy link
Contributor Author

Will need to make sure groupPrefixLength and groupPostfixLength default to 0

@knolleary if you get a second, can you comment on this suggestion (before I run off and build the UI for it)

@knolleary
Copy link
Member

Feels like a sensible quick solution that is most flexible. An alternative would be to require the actual pre/post strings to be provided - not as flexible, although perhaps more deterministic. Stick with lengths for now.

@hardillb
Copy link
Contributor Author

OK, got the changes done, will add some extra groups to my SSO test rig after lunch

@hardillb
Copy link
Contributor Author

Just thinking that using the length does mean it's possible to match other groups...

e.g. prefix 5 matches both

  • test_ff-dev-owner
  • fooo_ff-tev-owner

I don't think this is a problem as only LDAP/SAML admins should be able to create groups?

@hardillb hardillb moved this from In Design to In Progress in 🛠 Development Dec 11, 2024
@hardillb hardillb moved this from In Progress to Review in 🛠 Development Dec 11, 2024
@github-project-automation github-project-automation bot moved this from Scheduled to Closed / Done in ☁️ Product Planning Dec 13, 2024
@github-project-automation github-project-automation bot moved this from Review to Done in 🛠 Development Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer request requested by customer feature-request New feature or request that needs to be turned into Epic/Story details needs-triage Needs looking at to decide what to do size:M - 3 Sizing estimation point
Projects
Status: Closed / Done
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants