rohit/probo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Reject empty SAML NameIDs on login

Browse Source 
Empty NameID values were stored as '' and occupied the
unique saml_subject index, causing duplicate-key failures
on later logins. Reject blank NameIDs during assertion
validation, return a clear error when a NameID is already
linked to another account, and stop returning internal
errors from the SAML consume endpoint.

Signed-off-by: Sacha Al Himdani <sacha@probo.com>
This commit is contained in:
Sacha Al Himdani
2026-07-24 09:47:09 +02:00
parent e12351e0f4
commit bcd05a2e55
5 changed files with 234 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
pkg/coredata/identity.go
View File

@@ -52,6 +52,10 @@ type (
Identities []*Identity
)
var (
ErrSAMLSubjectAlreadyExists = errors.New("saml subject already exists")
)
func (i Identity) CursorKey(orderBy IdentityOrderField) page.CursorKey {
switch orderBy {
case IdentityOrderFieldCreatedAt:
@@ -247,8 +251,11 @@ VALUES (
_, err := conn.Exec(ctx, q, args)
if err != nil {
if pgErr, ok := errors.AsType[*pgconn.PgError](err); ok {
if pgErr.Code == "23505" && strings.Contains(pgErr.ConstraintName, "email_address") {
if pgErr, ok := errors.AsType[*pgconn.PgError](err); ok && pgErr.Code == "23505" {
switch pgErr.ConstraintName {
case "idx_users_saml_subject":
return ErrSAMLSubjectAlreadyExists
case "usrmgr_users_email_address_key":
return ErrResourceAlreadyExists
}
}
@@ -288,6 +295,15 @@ WHERE
result, err := conn.Exec(ctx, q, args)
if err != nil {
if pgErr, ok := errors.AsType[*pgconn.PgError](err); ok && pgErr.Code == "23505" {
switch pgErr.ConstraintName {
case "idx_users_saml_subject":
return ErrSAMLSubjectAlreadyExists
case "usrmgr_users_email_address_key":
return ErrResourceAlreadyExists
}
}
return fmt.Errorf("cannot update identity: %w", err)
}
@@ -304,6 +320,10 @@ func (i *Identity) LoadBySAMLSubject(
conn pg.Querier,
samlSubject string,
) error {
if strings.TrimSpace(samlSubject) == "" {
return ErrResourceNotFound
}
q := `
SELECT
id,

22
pkg/iam/saml/errors.go
View File

@@ -104,3 +104,25 @@ func NewUserInactiveError(profileID gid.GID) error {
func (e ErrUserInactive) Error() string {
return fmt.Sprintf("user %q is inactive", e.ProfileID)
}
type ErrSAMLSubjectAlreadyInUse struct {
AssertionID string
}
func NewSAMLSubjectAlreadyInUseError(assertionID string) error {
return &ErrSAMLSubjectAlreadyInUse{AssertionID: assertionID}
}
func (e ErrSAMLSubjectAlreadyInUse) Error() string {
return fmt.Sprintf("SAML NameID is already linked to another account (assertion %q)", e.AssertionID)
}
type ErrSAMLSubjectRequired struct{}
func NewSAMLSubjectRequiredError() error {
return &ErrSAMLSubjectRequired{}
}
func (e ErrSAMLSubjectRequired) Error() string {
return "NameID value is required"
}

35
pkg/iam/saml/service.go
View File

@@ -257,14 +257,16 @@ func (s *Service) HandleAssertion(
return NewEmailDomainMismatchError(email, config.EmailDomain)
}
samlSubject := strings.TrimSpace(assertion.Subject.NameID.Value)
err = identity.LoadByEmail(ctx, tx, email)
if err == coredata.ErrResourceNotFound && !config.AutoSignupEnabled {
if errors.Is(err, coredata.ErrResourceNotFound) && !config.AutoSignupEnabled {
return NewSAMLAutoSignupDisabledError(config.ID)
} else if err == coredata.ErrResourceNotFound && config.AutoSignupEnabled {
} else if errors.Is(err, coredata.ErrResourceNotFound) && config.AutoSignupEnabled {
*identity = coredata.Identity{
ID: gid.New(gid.NilTenant, coredata.IdentityEntityType),
EmailAddress: email,
SAMLSubject: &assertion.Subject.NameID.Value,
SAMLSubject: &samlSubject,
FullName: fullname,
HashedPassword: nil,
EmailAddressVerified: true,
@@ -272,8 +274,11 @@ func (s *Service) HandleAssertion(
UpdatedAt: now,
}
err := identity.Insert(ctx, tx)
if err != nil {
if err := identity.Insert(ctx, tx); err != nil {
if errors.Is(err, coredata.ErrSAMLSubjectAlreadyExists) {
return NewSAMLSubjectAlreadyInUseError(assertion.ID)
}
return fmt.Errorf("cannot insert identity: %w", err)
}
} else if err != nil {
@@ -282,16 +287,18 @@ func (s *Service) HandleAssertion(
identity.EmailAddress = email
identity.FullName = fullname
// Identity can exist (e.g. provisioned via SCIM) but not have a SAML subject
if identity.SAMLSubject == nil {
identity.SAMLSubject = &assertion.Subject.NameID.Value
if !hasSAMLSubject(identity) {
identity.SAMLSubject = &samlSubject
}
identity.EmailAddressVerified = true
identity.UpdatedAt = now
err = identity.Update(ctx, tx)
if err != nil {
if err = identity.Update(ctx, tx); err != nil {
if errors.Is(err, coredata.ErrSAMLSubjectAlreadyExists) {
return NewSAMLSubjectAlreadyInUseError(assertion.ID)
}
return fmt.Errorf("cannot update identity: %w", err)
}
}
@@ -415,6 +422,10 @@ func (s *Service) validateAssertion(assertion *saml.Assertion, config *coredata.
return fmt.Errorf("subject or NameID missing")
}
if strings.TrimSpace(assertion.Subject.NameID.Value) == "" {
return NewSAMLSubjectRequiredError()
}
if assertion.Issuer.Value != config.IdPEntityID {
return fmt.Errorf("assertion issuer %q does not match expected issuer %q",
assertion.Issuer.Value, config.IdPEntityID)
@@ -479,3 +490,7 @@ func (s *Service) baseServiceProvider() *saml.ServiceProvider {
AllowIDPInitiated: true,
}
}
func hasSAMLSubject(identity *coredata.Identity) bool {
return identity.SAMLSubject != nil && strings.TrimSpace(*identity.SAMLSubject) != ""
}

117
pkg/iam/saml/service_test.go Normal file
View File

@@ -0,0 +1,117 @@
// Copyright (c) 2025-2026 Probo Inc <hello@probo.com>.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.
package saml
import (
"errors"
"testing"
"time"
"github.com/crewjam/saml"
"go.probo.inc/probo/pkg/coredata"
)
func TestValidateAssertionRejectsEmptySAMLSubject(t *testing.T) {
t.Parallel()
s := &Service{}
config := &coredata.SAMLConfiguration{IdPEntityID: "https://idp.example"}
now := time.Now()
tests := []struct {
name string
value string
}{
{name: "empty", value: ""},
{name: "whitespace", value: " "},
}
for _, tt := range tests {
t.Run(
tt.name,
func(t *testing.T) {
t.Parallel()
err := s.validateAssertion(
&saml.Assertion{
ID: "assertion-1",
Issuer: saml.Issuer{
Value: config.IdPEntityID,
},
Subject: &saml.Subject{
NameID: &saml.NameID{Value: tt.value},
},
Conditions: &saml.Conditions{
NotOnOrAfter: now.Add(time.Hour),
},
},
config,
now,
)
if err == nil {
t.Fatal("expected error for empty SAML subject")
}
if _, ok := errors.AsType[*ErrSAMLSubjectRequired](err); !ok {
t.Fatalf("expected *ErrSAMLSubjectRequired, got %T: %v", err, err)
}
if got := err.Error(); got != "NameID value is required" {
t.Fatalf("unexpected error: %v", err)
}
},
)
}
}
func TestHasSAMLSubject(t *testing.T) {
t.Parallel()
empty := ""
whitespace := " "
value := "user@example.com"
tests := []struct {
name string
subject *string
expected bool
}{
{name: "nil", subject: nil, expected: false},
{name: "empty", subject: &empty, expected: false},
{name: "whitespace", subject: &whitespace, expected: false},
{name: "present", subject: &value, expected: true},
}
for _, tt := range tests {
t.Run(
tt.name,
func(t *testing.T) {
t.Parallel()
identity := &coredata.Identity{SAMLSubject: tt.subject}
if got := hasSAMLSubject(identity); got != tt.expected {
t.Fatalf("hasSAMLSubject() = %v, want %v", got, tt.expected)
}
},
)
}
}

49
pkg/server/api/connect/v1/saml_handler.go
View File

@@ -32,6 +32,7 @@ import (
"go.probo.inc/probo/pkg/baseurl"
"go.probo.inc/probo/pkg/gid"
"go.probo.inc/probo/pkg/iam"
"go.probo.inc/probo/pkg/iam/saml"
"go.probo.inc/probo/pkg/saferedirect"
"go.probo.inc/probo/pkg/securecookie"
"go.probo.inc/probo/pkg/server/api/authn"
@@ -59,6 +60,52 @@ func (h *SAMLHandler) renderInternalServerError(w http.ResponseWriter) {
httpserver.RenderError(w, http.StatusInternalServerError, errors.New("internal server error"))
}
func (h *SAMLHandler) renderAssertionError(w http.ResponseWriter, r *http.Request, err error) {
if isClientSAMLError(err) {
httpserver.RenderError(w, http.StatusUnauthorized, err)
return
}
h.logger.ErrorCtx(r.Context(), "cannot handle SAML assertion", log.Error(err))
httpserver.RenderError(w, http.StatusUnauthorized, errors.New("authentication failed"))
}
func isClientSAMLError(err error) bool {
if _, ok := errors.AsType[*saml.ErrSAMLConfigurationNotFound](err); ok {
return true
}
if _, ok := errors.AsType[*saml.ErrSAMLDisabled](err); ok {
return true
}
if _, ok := errors.AsType[*saml.ErrInvalidAssertion](err); ok {
return true
}
if _, ok := errors.AsType[*saml.ErrReplayAttackDetected](err); ok {
return true
}
if _, ok := errors.AsType[*saml.ErrEmailDomainMismatch](err); ok {
return true
}
if _, ok := errors.AsType[*saml.ErrSAMLAutoSignupDisabled](err); ok {
return true
}
if _, ok := errors.AsType[*saml.ErrUserInactive](err); ok {
return true
}
if _, ok := errors.AsType[*saml.ErrSAMLSubjectAlreadyInUse](err); ok {
return true
}
return false
}
func (h *SAMLHandler) MetadataHandler(w http.ResponseWriter, r *http.Request) {
metadataXML, err := h.iam.SAMLService.GenerateSpMetadata()
if err != nil {
@@ -101,7 +148,7 @@ func (h *SAMLHandler) ConsumeHandler(w http.ResponseWriter, r *http.Request) {
user, membership, err := h.iam.SAMLService.HandleAssertion(ctx, samlResponse, configID)
if err != nil {
httpserver.RenderError(w, http.StatusUnauthorized, err)
h.renderAssertionError(w, r, err)
return
}