diff --git a/docs/ACTIVE-DIRECTORY.md b/docs/ACTIVE-DIRECTORY.md index 0b43ca3..fe35ba3 100644 --- a/docs/ACTIVE-DIRECTORY.md +++ b/docs/ACTIVE-DIRECTORY.md @@ -151,8 +151,14 @@ These map AD attributes to SimpleAuth user fields: | `department_attr` | `department` | Department name | | `company_attr` | `company` | Company name | | `job_title_attr` | `title` | Job title | +| `given_name_attr` | `givenName` | First name, emitted as the OIDC `given_name` claim | +| `family_name_attr` | `sn` | Last name, emitted as the OIDC `family_name` claim | | `groups_attr` | `memberOf` | Group membership (multi-valued DN list) | +Left empty, `given_name_attr` and `family_name_attr` fall back to `givenName` and `sn`, so existing +configurations start emitting `given_name` / `family_name` without changes. OIDC clients such as +OpenProject use these claims for the user's first and last name. + ### Priority | Field | Default | Description | diff --git a/docs/API.md b/docs/API.md index ebcb440..a208950 100644 --- a/docs/API.md +++ b/docs/API.md @@ -580,7 +580,7 @@ curl -k https://auth.example.com/sauth/.well-known/openid-configuration "scopes_supported": ["openid", "profile", "email", "roles"], "token_endpoint_auth_methods_supported": ["client_secret_basic", "client_secret_post"], "claims_supported": [ - "sub", "iss", "aud", "exp", "iat", "name", "email", + "sub", "iss", "aud", "exp", "iat", "name", "given_name", "family_name", "email", "preferred_username", "realm_access", "department", "company", "job_title", "groups" ] @@ -817,7 +817,8 @@ Error codes: `invalid_request`, `invalid_client`, `invalid_grant`, `unsupported_ **Auth:** Bearer Token -OIDC UserInfo endpoint. +OIDC UserInfo endpoint. `given_name` and `family_name` come from the directory (`given_name_attr` / +`family_name_attr`) and are omitted when unknown, for example for local users. ```bash curl -k -H "Authorization: Bearer ACCESS_TOKEN" \ @@ -830,6 +831,8 @@ curl -k -H "Authorization: Bearer ACCESS_TOKEN" \ { "sub": "550e8400-e29b-41d4-a716-446655440000", "name": "John Smith", + "given_name": "John", + "family_name": "Smith", "email": "jsmith@corp.local", "preferred_username": "jsmith@corp.local", "department": "Engineering", diff --git a/internal/auth/jwt.go b/internal/auth/jwt.go index d62f497..f9cff75 100644 --- a/internal/auth/jwt.go +++ b/internal/auth/jwt.go @@ -29,6 +29,8 @@ type Claims struct { jwt.RegisteredClaims GUID string `json:"guid,omitempty"` Name string `json:"name,omitempty"` + GivenName string `json:"given_name,omitempty"` + FamilyName string `json:"family_name,omitempty"` Email string `json:"email,omitempty"` Department string `json:"department,omitempty"` Company string `json:"company,omitempty"` diff --git a/internal/auth/ldap.go b/internal/auth/ldap.go index 6903eff..2a9d5db 100644 --- a/internal/auth/ldap.go +++ b/internal/auth/ldap.go @@ -35,6 +35,8 @@ type LDAPConfig struct { CompanyAttr string JobTitleAttr string GroupsAttr string + GivenNameAttr string + FamilyNameAttr string } type LDAPResult struct { @@ -45,6 +47,8 @@ type LDAPResult struct { Department string Company string JobTitle string + GivenName string + FamilyName string Groups []string // Disabled is true when AD reports the account as disabled // (userAccountControl ACCOUNTDISABLE) or past its accountExpires date. @@ -250,7 +254,7 @@ func ldapAttrs(cfg *LDAPConfig) []string { usernameAttr = "sAMAccountName" } attrs := []string{"dn", usernameAttr, "userAccountControl", "accountExpires"} - for _, a := range []string{cfg.DisplayNameAttr, cfg.EmailAttr, cfg.DepartmentAttr, cfg.CompanyAttr, cfg.JobTitleAttr, cfg.GroupsAttr} { + for _, a := range []string{cfg.DisplayNameAttr, cfg.EmailAttr, cfg.DepartmentAttr, cfg.CompanyAttr, cfg.JobTitleAttr, cfg.GroupsAttr, cfg.GivenNameAttr, cfg.FamilyNameAttr} { if a != "" { attrs = append(attrs, a) } @@ -283,6 +287,12 @@ func entryToResult(entry *ldap.Entry, cfg *LDAPConfig) *LDAPResult { if cfg.JobTitleAttr != "" { result.JobTitle = entry.GetAttributeValue(cfg.JobTitleAttr) } + if cfg.GivenNameAttr != "" { + result.GivenName = entry.GetAttributeValue(cfg.GivenNameAttr) + } + if cfg.FamilyNameAttr != "" { + result.FamilyName = entry.GetAttributeValue(cfg.FamilyNameAttr) + } if cfg.GroupsAttr != "" { result.Groups = entry.GetAttributeValues(cfg.GroupsAttr) // Extract the CN from full DN group names, case-insensitively. AD returns diff --git a/internal/auth/ldap_names_test.go b/internal/auth/ldap_names_test.go new file mode 100644 index 0000000..e668b5c --- /dev/null +++ b/internal/auth/ldap_names_test.go @@ -0,0 +1,38 @@ +package auth + +import ( + "slices" + "testing" + + "github.com/go-ldap/ldap/v3" +) + +func TestLDAPReadsGivenAndFamilyName(t *testing.T) { + cfg := &LDAPConfig{UsernameAttr: "sAMAccountName", GivenNameAttr: "givenName", FamilyNameAttr: "sn"} + + attrs := ldapAttrs(cfg) + if !slices.Contains(attrs, "givenName") || !slices.Contains(attrs, "sn") { + t.Fatalf("search must request the name attributes, got %v", attrs) + } + + entry := ldap.NewEntry("CN=Salem Almarri,CN=Users,DC=green,DC=org", map[string][]string{ + "sAMAccountName": {"salem"}, + "givenName": {"سالم"}, + "sn": {"المري"}, + }) + got := entryToResult(entry, cfg) + if got.GivenName != "سالم" || got.FamilyName != "المري" { + t.Fatalf("given/family name not read from the entry: %q/%q", got.GivenName, got.FamilyName) + } +} + +func TestLDAPNameAttributesOptional(t *testing.T) { + cfg := &LDAPConfig{UsernameAttr: "sAMAccountName"} + if attrs := ldapAttrs(cfg); slices.Contains(attrs, "") { + t.Fatalf("unset name attributes must not add empty attribute names: %v", attrs) + } + got := entryToResult(ldap.NewEntry("CN=x", map[string][]string{"sAMAccountName": {"x"}, "givenName": {"X"}}), cfg) + if got.GivenName != "" || got.FamilyName != "" { + t.Fatalf("unmapped attributes must not populate names: %q/%q", got.GivenName, got.FamilyName) + } +} diff --git a/internal/handler/admin_ldap.go b/internal/handler/admin_ldap.go index 80de2ad..d2ce601 100644 --- a/internal/handler/admin_ldap.go +++ b/internal/handler/admin_ldap.go @@ -127,6 +127,8 @@ func (h *Handler) handleTestLDAPUser(w http.ResponseWriter, r *http.Request) { jsonResp(w, map[string]interface{}{ "status": "ok", "display_name": result.DisplayName, + "given_name": result.GivenName, + "family_name": result.FamilyName, "email": result.Email, "department": result.Department, "company": result.Company, @@ -204,6 +206,8 @@ func (h *Handler) handleImportLDAP(w http.ResponseWriter, r *http.Request) { CompanyAttr: "company", JobTitleAttr: "title", GroupsAttr: "memberOf", + GivenNameAttr: "givenName", + FamilyNameAttr: "sn", Domain: req.Domain, } @@ -352,6 +356,8 @@ func (h *Handler) handleAutoDiscoverLDAP(w http.ResponseWriter, r *http.Request) CompanyAttr: "company", JobTitleAttr: "title", GroupsAttr: "memberOf", + GivenNameAttr: "givenName", + FamilyNameAttr: "sn", Domain: domain, } @@ -484,6 +490,8 @@ func (h *Handler) handleImportLDAPUsers(w http.ResponseWriter, r *http.Request) // Create SimpleAuth user user := &store.User{ DisplayName: ldapUser.DisplayName, + GivenName: ldapUser.GivenName, + FamilyName: ldapUser.FamilyName, Email: ldapUser.Email, Department: ldapUser.Department, Company: ldapUser.Company, diff --git a/internal/handler/auth.go b/internal/handler/auth.go index a0e5048..8dc115d 100644 --- a/internal/handler/auth.go +++ b/internal/handler/auth.go @@ -191,6 +191,8 @@ func (h *Handler) authenticateUser(username, password string, app *store.App) (s // JIT provisioning newUser := &store.User{ DisplayName: result.DisplayName, + GivenName: result.GivenName, + FamilyName: result.FamilyName, Email: result.Email, Department: result.Department, Company: result.Company, @@ -331,6 +333,14 @@ func (h *Handler) syncUserFromLDAP(user *store.User, result *auth.LDAPResult) { user.JobTitle = result.JobTitle changed = true } + if result.GivenName != "" && result.GivenName != user.GivenName { + user.GivenName = result.GivenName + changed = true + } + if result.FamilyName != "" && result.FamilyName != user.FamilyName { + user.FamilyName = result.FamilyName + changed = true + } if result.Username != "" && result.Username != user.SAMAccountName { user.SAMAccountName = result.Username changed = true @@ -390,6 +400,8 @@ func (h *Handler) issueTokenPair(user *store.User, roles []string, perms []strin claims := auth.Claims{ GUID: user.GUID, Name: user.DisplayName, + GivenName: user.GivenName, + FamilyName: user.FamilyName, Email: user.Email, Department: user.Department, Company: user.Company, @@ -717,6 +729,8 @@ func (h *Handler) handleRefresh(w http.ResponseWriter, r *http.Request) { newClaims := auth.Claims{ GUID: user.GUID, Name: user.DisplayName, + GivenName: user.GivenName, + FamilyName: user.FamilyName, Email: user.Email, Department: user.Department, Company: user.Company, @@ -1614,6 +1628,8 @@ func (h *Handler) resolveKerberosUser(username string) (string, []string, error) newUser := &store.User{ DisplayName: result.DisplayName, + GivenName: result.GivenName, + FamilyName: result.FamilyName, Email: result.Email, Department: result.Department, Company: result.Company, diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 0b9e4e3..e061733 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -490,6 +490,13 @@ func (h *Handler) auditLogin(user *store.User, ip string, extra map[string]inter } func ldapConfigFromStore(p *store.LDAPConfig) *auth.LDAPConfig { + givenNameAttr, familyNameAttr := p.GivenNameAttr, p.FamilyNameAttr + if givenNameAttr == "" { + givenNameAttr = "givenName" + } + if familyNameAttr == "" { + familyNameAttr = "sn" + } return &auth.LDAPConfig{ URL: p.URL, BaseDN: p.BaseDN, @@ -506,6 +513,8 @@ func ldapConfigFromStore(p *store.LDAPConfig) *auth.LDAPConfig { CompanyAttr: p.CompanyAttr, JobTitleAttr: p.JobTitleAttr, GroupsAttr: p.GroupsAttr, + GivenNameAttr: givenNameAttr, + FamilyNameAttr: familyNameAttr, } } diff --git a/internal/handler/oidc.go b/internal/handler/oidc.go index 75d58c8..13776cd 100644 --- a/internal/handler/oidc.go +++ b/internal/handler/oidc.go @@ -119,7 +119,7 @@ func (h *Handler) handleOIDCDiscovery(w http.ResponseWriter, r *http.Request) { "token_endpoint_auth_methods_supported": authMethods, "code_challenge_methods_supported": []string{"S256"}, "claims_supported": []string{ - "sub", "iss", "aud", "exp", "iat", "name", "email", + "sub", "iss", "aud", "exp", "iat", "name", "given_name", "family_name", "email", "preferred_username", "realm_access", "resource_access", "department", "company", "job_title", "groups", }, @@ -888,6 +888,8 @@ func (h *Handler) issueOIDCTokens(w http.ResponseWriter, r *http.Request, user * idAud := appAudience(app) idClaims := auth.Claims{ Name: user.DisplayName, + GivenName: user.GivenName, + FamilyName: user.FamilyName, Email: user.Email, PreferredUsername: user.Email, Nonce: nonce, @@ -941,6 +943,8 @@ func (h *Handler) buildOIDCAccessClaims(user *store.User, roles, perms, groups [ claims := auth.Claims{ Name: user.DisplayName, + GivenName: user.GivenName, + FamilyName: user.FamilyName, Email: user.Email, Department: user.Department, Company: user.Company, @@ -1000,6 +1004,12 @@ func (h *Handler) handleOIDCUserInfo(w http.ResponseWriter, r *http.Request) { if user.Email == "" { resp["preferred_username"] = user.DisplayName } + if user.GivenName != "" { + resp["given_name"] = user.GivenName + } + if user.FamilyName != "" { + resp["family_name"] = user.FamilyName + } if user.Department != "" { resp["department"] = user.Department } diff --git a/internal/handler/oidc_profile_names_test.go b/internal/handler/oidc_profile_names_test.go new file mode 100644 index 0000000..1f1420d --- /dev/null +++ b/internal/handler/oidc_profile_names_test.go @@ -0,0 +1,177 @@ +package handler + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "simpleauth/internal/auth" + "simpleauth/internal/store" +) + +// oidcCodeFlowTokens drives authorize → code → token for the app-local "buyer" +// user that mkPKCEApp creates, and returns the token endpoint's JSON response. +func oidcCodeFlowTokens(t *testing.T, h *Handler, appID, cb string) map[string]interface{} { + t.Helper() + const realm = "test-issuer" + const verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + + form := url.Values{} + form.Set("client_id", appID) + form.Set("redirect_uri", cb) + form.Set("scope", "openid profile email") + form.Set("state", "st") + form.Set("nonce", "nc") + form.Set("code_challenge", s256(verifier)) + form.Set("code_challenge_method", "S256") + form.Set("username", "buyer") + form.Set("password", "buypass1") + rec := postAuthz(t, h, "/realms/"+realm+"/protocol/openid-connect/auth", form) + cbURL, _ := url.Parse(rec.Header().Get("Location")) + code := cbURL.Query().Get("code") + if code == "" { + t.Fatalf("no auth code: %d %q", rec.Code, rec.Header().Get("Location")) + } + + exchange := url.Values{ + "grant_type": {"authorization_code"}, "code": {code}, + "redirect_uri": {cb}, "code_verifier": {verifier}, + } + req := httptest.NewRequest("POST", "/realms/"+realm+"/protocol/openid-connect/token", strings.NewReader(exchange.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + trec := httptest.NewRecorder() + h.ServeHTTP(trec, req) + if trec.Code != http.StatusOK { + t.Fatalf("token exchange: %d %s", trec.Code, trec.Body.String()) + } + var tokens map[string]interface{} + if err := json.Unmarshal(trec.Body.Bytes(), &tokens); err != nil { + t.Fatalf("token response: %v", err) + } + return tokens +} + +func oidcUserinfo(t *testing.T, h *Handler, accessToken string) map[string]interface{} { + t.Helper() + req := httptest.NewRequest("GET", "/realms/test-issuer/protocol/openid-connect/userinfo", nil) + req.Header.Set("Authorization", "Bearer "+accessToken) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("userinfo: %d %s", rec.Code, rec.Body.String()) + } + var info map[string]interface{} + if err := json.Unmarshal(rec.Body.Bytes(), &info); err != nil { + t.Fatalf("userinfo response: %v", err) + } + return info +} + +func setBuyerNames(t *testing.T, s store.Store, appID, given, family string) { + t.Helper() + guid, err := s.ResolveMapping("applocal:"+appID, "buyer") + if err != nil { + t.Fatalf("resolve buyer: %v", err) + } + u, err := s.GetUser(guid) + if err != nil { + t.Fatalf("get buyer: %v", err) + } + u.DisplayName, u.GivenName, u.FamilyName = given+" "+family, given, family + if err := s.UpdateUser(u); err != nil { + t.Fatalf("update buyer: %v", err) + } +} + +// OIDC relying parties (OpenProject among them) build a user's first and last name +// from the standard profile claims; with only `name` they put the whole display +// name into the first name and ask the user for a last name on first login. +func TestOIDCTokensCarryGivenAndFamilyName(t *testing.T) { + h, s := testSetup(t) + cb := mkPKCEApp(t, h, "names1") + setBuyerNames(t, s, "names1", "سالم", "المري") + + tokens := oidcCodeFlowTokens(t, h, "names1", cb) + sources := map[string]map[string]interface{}{ + "id_token": decodeJWTPayload(t, tokens["id_token"].(string)), + "access_token": decodeJWTPayload(t, tokens["access_token"].(string)), + "userinfo": oidcUserinfo(t, h, tokens["access_token"].(string)), + } + for source, claims := range sources { + if claims["given_name"] != "سالم" || claims["family_name"] != "المري" { + t.Errorf("%s: given_name=%v family_name=%v", source, claims["given_name"], claims["family_name"]) + } + } +} + +func TestOIDCTokensOmitUnknownNames(t *testing.T) { + h, _ := testSetup(t) + cb := mkPKCEApp(t, h, "names2") + + tokens := oidcCodeFlowTokens(t, h, "names2", cb) + idToken := decodeJWTPayload(t, tokens["id_token"].(string)) + info := oidcUserinfo(t, h, tokens["access_token"].(string)) + for _, claims := range []map[string]interface{}{idToken, info} { + if _, ok := claims["given_name"]; ok { + t.Errorf("given_name must be omitted when unknown, got %v", claims["given_name"]) + } + if _, ok := claims["family_name"]; ok { + t.Errorf("family_name must be omitted when unknown, got %v", claims["family_name"]) + } + } +} + +func TestOIDCDiscoveryAdvertisesNameClaims(t *testing.T) { + h, _ := testSetup(t) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest("GET", "/realms/test-issuer/.well-known/openid-configuration", nil)) + var doc struct { + ClaimsSupported []string `json:"claims_supported"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &doc); err != nil { + t.Fatalf("discovery: %d %v", rec.Code, err) + } + for _, want := range []string{"given_name", "family_name"} { + found := false + for _, c := range doc.ClaimsSupported { + found = found || c == want + } + if !found { + t.Errorf("claims_supported is missing %q: %v", want, doc.ClaimsSupported) + } + } +} + +func TestLDAPConfigDefaultsNameAttributes(t *testing.T) { + cfg := ldapConfigFromStore(&store.LDAPConfig{}) + if cfg.GivenNameAttr != "givenName" || cfg.FamilyNameAttr != "sn" { + t.Errorf("unset name attributes must default to AD's givenName/sn, got %q/%q", cfg.GivenNameAttr, cfg.FamilyNameAttr) + } + custom := ldapConfigFromStore(&store.LDAPConfig{GivenNameAttr: "firstName", FamilyNameAttr: "lastName"}) + if custom.GivenNameAttr != "firstName" || custom.FamilyNameAttr != "lastName" { + t.Errorf("configured name attributes must be kept, got %q/%q", custom.GivenNameAttr, custom.FamilyNameAttr) + } +} + +func TestSyncUserFromLDAPUpdatesNames(t *testing.T) { + h, s := testSetup(t) + u := &store.User{DisplayName: "John Doe", SAMAccountName: "jdoe"} + if err := s.CreateUser(u); err != nil { + t.Fatalf("create: %v", err) + } + + h.syncUserFromLDAP(u, &auth.LDAPResult{Username: "jdoe", DisplayName: "John Doe", GivenName: "John", FamilyName: "Doe"}) + got, _ := s.GetUser(u.GUID) + if got.GivenName != "John" || got.FamilyName != "Doe" { + t.Fatalf("sync must store the directory names, got %q/%q", got.GivenName, got.FamilyName) + } + + h.syncUserFromLDAP(got, &auth.LDAPResult{Username: "jdoe", DisplayName: "John Doe-Smith", GivenName: "John", FamilyName: "Doe-Smith"}) + got, _ = s.GetUser(u.GUID) + if got.FamilyName != "Doe-Smith" { + t.Fatalf("a renamed directory account must update family_name, got %q", got.FamilyName) + } +} diff --git a/internal/store/types.go b/internal/store/types.go index b037526..1ecfd30 100644 --- a/internal/store/types.go +++ b/internal/store/types.go @@ -8,6 +8,8 @@ type User struct { GUID string `json:"guid"` PasswordHash string `json:"password_hash,omitempty"` DisplayName string `json:"display_name"` + GivenName string `json:"given_name,omitempty"` + FamilyName string `json:"family_name,omitempty"` Email string `json:"email"` Department string `json:"department,omitempty"` Company string `json:"company,omitempty"` @@ -85,6 +87,12 @@ type LDAPConfig struct { GroupsAttr string `json:"groups_attr"` Domain string `json:"domain,omitempty"` ConfiguredAt time.Time `json:"configured_at"` + + // GivenNameAttr / FamilyNameAttr feed the OIDC given_name / family_name + // claims. Empty means the AD defaults (givenName / sn), so deployments + // configured before these existed pick them up without reconfiguration. + GivenNameAttr string `json:"given_name_attr,omitempty"` + FamilyNameAttr string `json:"family_name_attr,omitempty"` } // App is a registered application (OAuth client) with its own per-app