Implement LDAP by arktronic-sep · Pull Request #38 · sep/sama

arktronic-sep · 2026-02-17T15:29:43Z

No description provided.

Copilot

Pull request overview

This PR implements LDAP authentication support for SAMA, allowing users to authenticate against LDAP/Active Directory servers. The implementation includes JIT (Just-In-Time) user provisioning, group-based role mappings, and workspace access management based on LDAP group memberships.

Changes:

Added LDAP authentication service with support for direct bind and service account modes
Implemented group-based workspace and role mappings for automatic access control
Created admin UI for LDAP configuration and group mapping management
Reorganized settings pages into a tabbed layout (General, Import/Export, LDAP, Group Mappings)

Reviewed changes

Copilot reviewed 53 out of 53 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
SAMA.Web/Services/LdapAuthenticationService.cs	Core LDAP authentication logic with connection handling, user search, and group mapping
SAMA.Web/Services/GlobalSettingsService.cs	Added LDAP configuration properties with encrypted password storage
SAMA.Web/Pages/Admin/Settings/Ldap.cshtml.cs	Page model for LDAP settings management and test login functionality
SAMA.Web/Pages/Admin/Settings/GroupMappings.cshtml.cs	Page model for managing LDAP group to workspace/role mappings
SAMA.Web/Pages/Account/Login.cshtml.cs	Updated login flow to support LDAP authentication with fallback to local auth
SAMA.Web/Services/Queries/GroupMappingQueryService.cs	Query service for retrieving group mappings
SAMA.Web/Services/Commands/GroupMappingCommandService.cs	Command service for creating/deleting group mappings
SAMA.Web/Services/LdapLoginResult.cs	Result object for LDAP authentication attempts
SAMA.Web/Models/UserViewModel.cs	Added IsExternalUser property to identify LDAP users
SAMA.Web/wwwroot/css/site.css	Added CSS styles for h5/h6 headings and standardized table action links

Comments suppressed due to low confidence (4)

SAMA.Web/Pages/Admin/Settings/Ldap.cshtml:1

Corrected spelling of 'recieve' to 'receive'.
SAMA.Web/Pages/Admin/Settings/Ldap.cshtml.cs:1
Setting LdapBindPassword to null! (null-forgiving operator) is inconsistent with line 111 where it's set to an actual value. Consider using string.Empty instead of null! for consistency with the SetEncryptedStringAsync method which expects a string.
SAMA.Web/Services/GlobalSettingsService.cs:1
The null-coalescing assignment is applied to the value parameter before it's used. However, this modification affects the parameter itself, which could be unexpected. Consider assigning to a local variable instead to make the intent clearer: var safeValue = value ?? string.Empty; and use safeValue in subsequent operations.
SAMA.Web/Services/LdapAuthenticationService.cs:1
The parameter customRootCa has inconsistent nullability handling. In the method signature it's non-nullable string, but the CreateConnection method at line 363 expects string?. This parameter should be marked as nullable string? to match its usage.

Copilot

Pull request overview

Copilot reviewed 53 out of 53 changed files in this pull request and generated 8 comments.

Copilot · 2026-02-17T17:08:56Z

@arktronic-sep I've opened a new pull request, #39, to work on those changes. Once the pull request is ready, I'll request review from you.

…tions (#39) * Initial plan * Add validation for LDAP search filters to prevent string.Format exceptions Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Improve LDAP filter validation to catch malformed format strings Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Always validate GroupSearchFilter regardless of GroupSearchBase state Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Add additional validation to ensure exactly one placeholder in LDAP filters Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Allow multiple {0} placeholders and simplify exception handling Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Use specific exception type and string.Empty for consistency Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Rename tests to remove misleading 'GroupSearchEnabled' suffix Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Clarify comment and fix duplicate test name Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Extract default filter values as constants for maintainability Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Configure CI to run on all pull requests, not just ones targeting main Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Show validation errors to users instead of silently replacing invalid filters Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> * Fix error messages to say 'at least one' instead of 'exactly one' Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: arktronic-sep <120513939+arktronic-sep@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 54 out of 54 changed files in this pull request and generated 4 comments.

Copilot · 2026-02-17T18:03:08Z

    branches: [ main ]
  pull_request:
-    branches: [ main ]



The CI workflow file removes the branches: [ main ] filter from pull_request events. This means pull requests from any branch will now trigger the CI workflow, not just those targeting main.

This change should be intentional. If it was meant to allow PRs from any branch to trigger CI (e.g., for feature branches), this is fine. However, if the intent was to keep CI running only for PRs to main, this line should be restored.

Suggested change

branches: [ main ]

Copilot · 2026-02-17T18:03:08Z

+        else if (!shouldBeAdmin && isAdmin)
+        {
+            await userManager.RemoveFromRoleAsync(user, AuthConstants.AdminRole);
+            _logger.LogInformation("Revoked Admin role from user {Email} — no longer in an LDAP admin group", user.Email);
+        }


The code automatically revokes the Admin role from LDAP users when they no longer match an admin group mapping. However, this applies to ALL admin users, including those who may have been manually promoted to Admin before LDAP was configured. This could create a security risk where legitimate admin accounts lose access unexpectedly.

Consider tracking whether a user was provisioned via LDAP initially (similar to how workspace assignments use Source), and only apply automatic admin revocation to LDAP-sourced admin users. Alternatively, document this behavior clearly and ensure administrators are aware that enabling LDAP group-based admin management will take precedence over manual admin assignments for LDAP users.

Copilot · 2026-02-17T18:03:08Z

+            if (setting != null && !string.IsNullOrEmpty(setting.Value) && _encryptionService != null && _keyProvider != null)
+            {
+                var decrypted = _encryptionService.Decrypt(setting.Value, _keyProvider.Key);
+                return decrypted;
+            }
+
+            return string.Empty;


The encrypted LDAP bind password is not cached (unlike other settings). This means every time LDAP settings are accessed, the encrypted password will be read from the database and decrypted. While this is correct for security (avoiding password caching), if encryption services are null (e.g., no encryption key configured), the password will silently return as empty string without logging a warning.

Consider adding a warning log when encryption services are null but an encrypted setting is being accessed, to help administrators debug configuration issues.

arktronic-sep added 8 commits February 16, 2026 14:13

WIP: LDAP

f17957d

Add LDAP package license

7cbdf00

Surface LDAP status for users

fd563ae

Remove outdated mentions of SAML

5bd9f94

Accept email directly when using bind templates

3586c96

Add group mapping functionality

982ce68

Commonize styling

cd3f328

Update login field name

ae18e74

arktronic-sep requested a review from Copilot February 17, 2026 15:39

Copilot started reviewing on behalf of arktronic-sep February 17, 2026 15:43 View session

Copilot AI reviewed Feb 17, 2026

View reviewed changes

Fix import button usage

b421c01

arktronic-sep requested a review from Copilot February 17, 2026 16:06

Copilot started reviewing on behalf of arktronic-sep February 17, 2026 16:07 View session

Copilot AI reviewed Feb 17, 2026

View reviewed changes

Copilot AI mentioned this pull request Feb 17, 2026

Add validation for LDAP search filters to prevent string.Format exceptions #39

Merged

arktronic-sep requested a review from Copilot February 17, 2026 17:58

Copilot started reviewing on behalf of arktronic-sep February 17, 2026 17:59 View session

Copilot AI reviewed Feb 17, 2026

View reviewed changes

arktronic-sep added 4 commits February 17, 2026 13:03

Minor tweaks

f2ee916

Commonize SSL/STARTTLS terminology

9aaf7ee

Fix improper null assignment

9aeed87

Add explanation for LDAP/local account mixing

24e7693

arktronic-sep merged commit 1bd0e89 into main Feb 17, 2026
4 checks passed

arktronic-sep deleted the ldap branch February 17, 2026 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement LDAP#38

Implement LDAP#38
arktronic-sep merged 14 commits into
mainfrom
ldap

arktronic-sep commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

arktronic-sep commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants