Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 135 additions & 10 deletions api/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,30 @@ import (
"golang.org/x/oauth2"
)

// LdapUserData holds LDAP user information including groups
type LdapUserData struct {
User db.User
Groups []string
}

func convertEntryToMap(entity *ldap.Entry) map[string]any {
res := map[string]any{}
for _, attr := range entity.Attributes {
if len(attr.Values) == 0 {
continue
}
res[attr.Name] = attr.Values[0]
// For group membership attributes, store all values as an array
if len(attr.Values) > 1 {
res[attr.Name] = attr.Values
} else {
res[attr.Name] = attr.Values[0]
}
}

return res
}

func tryFindLDAPUser(username, password string) (*db.User, error) {
func tryFindLDAPUser(username, password string) (*LdapUserData, error) {
if !util.Config.LdapEnable {
return nil, fmt.Errorf("LDAP not configured")
}
Expand Down Expand Up @@ -101,12 +112,12 @@ func tryFindLDAPUser(username, password string) (*db.User, error) {
return nil, err
}

// Get user info
// Get user info including group memberships
searchRequest = ldap.NewSearchRequest(
util.Config.LdapSearchDN,
ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false,
fmt.Sprintf(util.Config.LdapSearchFilter, username),
[]string{util.Config.LdapMappings.DN, util.Config.LdapMappings.Mail, util.Config.LdapMappings.UID, util.Config.LdapMappings.CN},
[]string{util.Config.LdapMappings.DN, util.Config.LdapMappings.Mail, util.Config.LdapMappings.UID, util.Config.LdapMappings.CN, util.Config.LdapMappings.MemberOf},
nil,
)

Expand All @@ -128,13 +139,20 @@ func tryFindLDAPUser(username, password string) (*db.User, error) {
return nil, err
}

// Extract group memberships from LDAP claims
userGroups := extractGroupsFromClaims(entry, util.Config.LdapMappings.MemberOf)

// Check if user should be admin based on group membership
isAdmin := checkIfUserIsAdmin(userGroups, util.Config.LdapAdminGroups)

ldapUser := db.User{
Username: strings.ToLower(claims.username),
Created: tz.Now(),
Name: claims.name,
Email: claims.email,
External: true,
Alert: false,
Admin: isAdmin,
}

err = db.ValidateUser(ldapUser)
Expand All @@ -145,7 +163,75 @@ func tryFindLDAPUser(username, password string) (*db.User, error) {
}

log.Info("User " + ldapUser.Name + " with email " + ldapUser.Email + " authorized via LDAP correctly")
return &ldapUser, nil
return &LdapUserData{
User: ldapUser,
Groups: userGroups,
}, nil
}

// extractGroupsFromClaims extracts group memberships from LDAP claims
func extractGroupsFromClaims(claims map[string]any, groupAttribute string) []string {
var groups []string

if groupsValue, exists := claims[groupAttribute]; exists {
switch v := groupsValue.(type) {
case string:
groups = append(groups, v)
case []string:
groups = v
case []any:
for _, item := range v {
if str, ok := item.(string); ok {
groups = append(groups, str)
}
}
}
}

return groups
}

// checkIfUserIsAdmin determines if user should be admin based on group membership
func checkIfUserIsAdmin(userGroups []string, adminGroups []string) bool {
if len(adminGroups) == 0 {
return false
}

for _, userGroup := range userGroups {
for _, adminGroup := range adminGroups {
if userGroup == adminGroup {
return true
}
}
}

return false
}

// assignRolesBasedOnGroups assigns project roles based on group mappings
// Note: This is a simplified version that doesn't support project-specific roles
// The group mapping format should be: "groupName:role" where role is one of: owner, manager, task_runner, guest
func assignRolesBasedOnGroups(store db.Store, userID int, userGroups []string, groupMappings map[string]string) error {
if len(groupMappings) == 0 {
return nil
}

// For now, log the group mappings that would be applied
// In a future version, this could be extended to support project-specific role assignment
for _, userGroup := range userGroups {
if roleStr, exists := groupMappings[userGroup]; exists {
// Validate that the role is recognized
switch strings.ToLower(roleStr) {
case "owner", "manager", "task_runner", "guest":
log.Info(fmt.Sprintf("LDAP user %d has group %s mapped to role %s (project assignment not yet implemented)", userID, userGroup, roleStr))
// TODO: Implement project assignment when GetProjectByName or similar method is available
default:
log.Warn("Unknown role in LDAP group mapping: " + roleStr)
}
}
}

return nil
}

// createSession creates session for passed user and stores session details
Expand Down Expand Up @@ -234,11 +320,50 @@ func loginByPassword(store db.Store, login string, password string) (user db.Use
return
}

func loginByLDAP(store db.Store, ldapUser db.User) (user db.User, err error) {
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 {
Comment on lines +323 to 369

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 👍 / 👎.

Expand Down Expand Up @@ -334,10 +459,10 @@ func login(w http.ResponseWriter, r *http.Request) {

var err error

var ldapUser *db.User
var ldapUserData *LdapUserData

if util.Config.LdapEnable {
ldapUser, err = tryFindLDAPUser(login.Auth, login.Password)
ldapUserData, err = tryFindLDAPUser(login.Auth, login.Password)
if err != nil {
log.Warn(err.Error())
w.WriteHeader(http.StatusInternalServerError)
Expand All @@ -347,10 +472,10 @@ func login(w http.ResponseWriter, r *http.Request) {

var user db.User

if ldapUser == nil {
if ldapUserData == nil {
user, err = loginByPassword(helpers.Store(r), login.Auth, login.Password)
} else {
user, err = loginByLDAP(helpers.Store(r), *ldapUser)
user, err = loginByLDAP(helpers.Store(r), *ldapUserData)
}

if err != nil {
Expand Down
64 changes: 64 additions & 0 deletions api/login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,67 @@ func TestParseClaim5(t *testing.T) {
t.Fatalf("Expected: %v, Got: %v", "123456757343", res)
}
}

func TestExtractGroupsFromClaims(t *testing.T) {
// Test with string array
claims := map[string]any{
"memberOf": []string{"group1", "group2", "group3"},
}
groups := extractGroupsFromClaims(claims, "memberOf")
if len(groups) != 3 {
t.Fatalf("expected 3 groups, got %d", len(groups))
}
if groups[0] != "group1" || groups[1] != "group2" || groups[2] != "group3" {
t.Fatalf("unexpected groups: %v", groups)
}

// Test with single string
claims2 := map[string]any{
"memberOf": "single-group",
}
groups2 := extractGroupsFromClaims(claims2, "memberOf")
if len(groups2) != 1 {
t.Fatalf("expected 1 group, got %d", len(groups2))
}
if groups2[0] != "single-group" {
t.Fatalf("unexpected group: %v", groups2)
}

// Test with interface{} array
claims3 := map[string]any{
"memberOf": []any{"group1", "group2"},
}
groups3 := extractGroupsFromClaims(claims3, "memberOf")
if len(groups3) != 2 {
t.Fatalf("expected 2 groups, got %d", len(groups3))
}

// Test with missing attribute
claims4 := map[string]any{}
groups4 := extractGroupsFromClaims(claims4, "memberOf")
if len(groups4) != 0 {
t.Fatalf("expected 0 groups, got %d", len(groups4))
}
}

func TestCheckIfUserIsAdmin(t *testing.T) {
adminGroups := []string{"admin-group", "super-admin"}

// Test user in admin group
userGroups1 := []string{"user-group", "admin-group", "other-group"}
if !checkIfUserIsAdmin(userGroups1, adminGroups) {
t.Fatal("expected user to be admin")
}

// Test user not in admin group
userGroups2 := []string{"user-group", "other-group"}
if checkIfUserIsAdmin(userGroups2, adminGroups) {
t.Fatal("expected user to not be admin")
}

// Test with empty admin groups
userGroups3 := []string{"admin-group"}
if checkIfUserIsAdmin(userGroups3, []string{}) {
t.Fatal("expected user to not be admin when no admin groups configured")
}
}
3 changes: 3 additions & 0 deletions deployment/docker/server/server-wrapper
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ export SEMAPHORE_LDAP_DN_SEARCH="${SEMAPHORE_LDAP_DN_SEARCH:-}"
export SEMAPHORE_LDAP_MAPPING_USERNAME="${SEMAPHORE_LDAP_MAPPING_USERNAME:-uid}"
export SEMAPHORE_LDAP_MAPPING_FULLNAME="${SEMAPHORE_LDAP_MAPPING_FULLNAME:-cn}"
export SEMAPHORE_LDAP_MAPPING_EMAIL="${SEMAPHORE_LDAP_MAPPING_EMAIL:-mail}"
export SEMAPHORE_LDAP_MAPPING_MEMBEROF="${SEMAPHORE_LDAP_MAPPING_MEMBEROF:-memberOf}"
export SEMAPHORE_LDAP_GROUP_MAPPINGS="${SEMAPHORE_LDAP_GROUP_MAPPINGS:-}"
export SEMAPHORE_LDAP_ADMIN_GROUPS="${SEMAPHORE_LDAP_ADMIN_GROUPS:-}"
file_env 'SEMAPHORE_ACCESS_KEY_ENCRYPTION'


Expand Down
Loading
Loading