Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Add LDAP group-based role mapping for automatic user role assignment #3269

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

Open
Copilot wants to merge 4 commits into develop
base: develop
Choose a base branch
Loading
from copilot/fix-3226

Conversation

Copy link
Contributor

@Copilot Copilot AI commented Sep 2, 2025 •
edited
Loading

This PR implements automatic role assignment for LDAP users based on their Active Directory/LDAP group memberships, addressing the common issue where LDAP users can authenticate but have no permissions in Semaphore.

Problem

Users reported that while LDAP authentication worked correctly, authenticated users had no rights or roles assigned, requiring manual intervention for each user. The request was specifically for automatic role assignment based on Active Directory groups.

Solution

New Features

Automatic Admin Role Assignment: Users belonging to configured LDAP groups automatically receive admin privileges upon login.

Group-Based Role Mapping Framework: Infrastructure for mapping LDAP groups to project roles (currently logs mappings, ready for future project-specific assignment).

Enhanced Group Processing: Retrieves and processes multiple group memberships from LDAP, handling various formats (single group, multiple groups, multi-value attributes).

Configuration

Three new environment variables enable group-based functionality:

# Configure which LDAP attribute contains group memberships
SEMAPHORE_LDAP_MAPPING_MEMBEROF=memberOf
# Define groups that grant admin access
SEMAPHORE_LDAP_ADMIN_GROUPS='["CN=Semaphore_Admins,OU=Groups,DC=company,DC=com"]'
# Map groups to roles for future project assignment
SEMAPHORE_LDAP_GROUP_MAPPINGS='{"CN=Semaphore_Users,OU=Groups,DC=company,DC=com": "manager"}'

Example Usage

For a typical Active Directory setup:

version: '3.8'
services:
 semaphore:
 environment:
 SEMAPHORE_LDAP_ENABLE: "true"
 SEMAPHORE_LDAP_SERVER: "ldap.company.com:389"
 SEMAPHORE_LDAP_ADMIN_GROUPS: '["CN=Semaphore_Admins,OU=Security Groups,DC=company,DC=local"]'
 SEMAPHORE_LDAP_GROUP_MAPPINGS: '{"CN=Semaphore_Users,OU=Security Groups,DC=company,DC=local": "task_runner"}'

Implementation Details

  • Enhanced LDAP Search: Modified tryFindLDAPUser to retrieve group membership attributes alongside user information
  • Group Processing: Added extractGroupsFromClaims, checkIfUserIsAdmin, and assignRolesBasedOnGroups functions
  • Backwards Compatibility: All existing LDAP configurations continue working without changes
  • Multi-value Support: Handles LDAP attributes with single or multiple values
  • Comprehensive Testing: Added test coverage for all new group processing functions

Quality Assurance

  • All existing tests pass
  • New test coverage for group processing functionality
  • Manual validation confirms server functionality
  • Backwards compatibility verified
  • Comprehensive documentation added

The implementation provides immediate value for admin role assignment while establishing a foundation for future project-specific role mapping capabilities.

Fixes #3226.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

ShadowJonathan reacted with eyes emoji
Copilot AI and others added 3 commits September 2, 2025 20:21 
...tation
Co-authored-by: fiftin <914224+fiftin@users.noreply.github.com>
Co-authored-by: fiftin <914224+fiftin@users.noreply.github.com>
Co-authored-by: fiftin <914224+fiftin@users.noreply.github.com>
@Copilot Copilot AI changed the title (削除) [WIP] Problem: Mapping roles to users from LDAP (削除ここまで) (追記) Add LDAP group-based role mapping for automatic user role assignment (追記ここまで) Sep 2, 2025
@Copilot Copilot AI requested a review from fiftin September 2, 2025 20:38
Copilot finished work on behalf of fiftin September 2, 2025 20:38
@fiftin fiftin marked this pull request as ready for review September 21, 2025 15:48
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

Comment on lines +323 to 369
func loginByLDAP(store db.Store, ldapUserData LdapUserData) (user db.User, err error) {
ldapUser := ldapUserData.User
user, err = store.GetUserByLoginOrEmail(ldapUser.Username, ldapUser.Email)

if errors.Is(err, db.ErrNotFound) {
user, err = store.CreateUserWithoutPassword(ldapUser)
if err != nil {
return
}

// For new users, assign roles based on group membership
if len(util.Config.LdapGroupMappings) > 0 {
err = assignRolesBasedOnGroups(store, user.ID, ldapUserData.Groups, util.Config.LdapGroupMappings)
if err != nil {
log.Error("Failed to assign roles based on LDAP groups: " + err.Error())
// Don't fail the login, just log the error
}
}
} else if err == nil {
// For existing users, update admin status if needed
if user.Admin != ldapUser.Admin {
user.Admin = ldapUser.Admin
// Update the user in the database using UserWithPwd structure
userWithPwd := db.UserWithPwd{
User: user,
Pwd: "", // Empty password since this is an LDAP user
}
err = store.UpdateUser(userWithPwd)
if err != nil {
log.Error("Failed to update user admin status: " + err.Error())
// Continue with login even if update fails
err = nil
}
}

// For existing users, update roles based on current group membership if configured
if len(util.Config.LdapGroupMappings) > 0 {
err = assignRolesBasedOnGroups(store, user.ID, ldapUserData.Groups, util.Config.LdapGroupMappings)
if err != nil {
log.Error("Failed to update roles based on LDAP groups: " + err.Error())
// Don't fail the login, just log the error
err = nil
}
}
}

if err != nil {
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Sep 21, 2025

Choose a reason for hiding this comment

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

[P0] Update local user roles before verifying LDAP user is external

The new loginByLDAP logic updates an existing user’s Admin flag (and potentially other role mappings) before checking whether the matched account is marked as external. If a local database user shares the same username or email as an LDAP account, an LDAP login attempt will execute store.UpdateUser and can flip the local user’s admin status based on LDAP group membership, even though the function later rejects the login with db.ErrNotFound because user.External is false. This allows an authenticated LDAP user to elevate a local account’s privileges without actually logging in, which is a serious regression compared to the previous version where the external check occurred before any updates.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

@fiftin fiftin Awaiting requested review from fiftin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Problem: Mapping roles to users from LDAP

2 participants

AltStyle によって変換されたページ (->オリジナル) /