Add support for AttributeConsumingService in SAML metadata#396
Add support for AttributeConsumingService in SAML metadata#396
Conversation
- Add comprehensive JSDoc comments to AttributeConsumingService interface - Include detailed examples and common attribute OIDs - Add @see reference to SAML 2.0 Metadata Specification - Enhance metadataAttributeConsumingServices property documentation - Provide IDE-friendly hints with @example and @default annotations - Include language code examples and service name/description guidance
- Add COMMON_SAML_ATTRIBUTES constants with common OIDs and descriptions - Add SAML_ATTRIBUTE_NAME_FORMATS constants for attribute name formats - Add CommonSamlAttributeName, and SamlAttributeNameFormat types - Export new constants and types from index.ts for public API - Provide comprehensive IDE support with intellisense for common SAML attributes - Enable type-safe usage of standard SAML attribute names and formats
|
Update: it seems that description has been edited to point to correct issue. Following comment is now outdated: Sidenote: referenced issue (406) at first chapter of description does not exist at It (must) refer to I.e. consider editing description so that issue number is replaced with URL to passport-saml repository's node-saml/passport-saml#406. |
src/types.ts
Outdated
| /** | ||
| * Common language codes for ServiceName and ServiceDescription | ||
| */ | ||
| export type CommonLanguageCode = "en" | "es" | "fr" | "de" | "it" | "pt" | "ja" | "ko" | "zh" | "ar" | "ru"; |
There was a problem hiding this comment.
Out of curiosity:
From what perspective these are most common language codes? :)
I see that list includes e.g. it but not e.g. hi, vi or id
References:
- https://en.wikipedia.org/wiki/List_of_languages_by_total_number_of_speakers
- https://en.wikipedia.org/wiki/List_of_ISO_639_language_codes
Real question: what purpose does aforementioned 11 out of 31 languages (by total number of speakers and it being 31st) serve?
There was a problem hiding this comment.
I'm in agreement here, I've deleted all references to CommonLanguageCode (an import, and export, and this definition) and the code still compiles just fine. I can't find any reference to this type anywhere useful. What purpose does having this definition serve @markstos ?
There was a problem hiding this comment.
Can't we leave this out for now? I don't see it used as both of you mentioned and it seems to be the only blocking factor in getting this merged?
|
I tested it out and it works as expected. 👌 |
WalkthroughAdds AttributeConsumingService support for SP metadata (types, generation, docs, tests), re-exports related constants/types, coerces undefined/null SAML form values to empty strings, and adds tests verifying single/multiple/absent AttributeConsumingService behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Application
participant API as Public API (generateServiceProviderMetadata)
participant Builder as Metadata Builder
participant XML as XML Generator
App->>API: Call generateServiceProviderMetadata(params)
API->>Builder: Build SPSSODescriptor (include basic elements)
alt params.metadataAttributeConsumingServices present
note right of Builder #D6EAF8: Add AttributeConsumingService entries\n(index, isDefault?, ServiceName, ServiceDescription?, RequestedAttribute[])
Builder-->>XML: Insert AttributeConsumingService blocks
else not present
note right of Builder #F8ECE0: Omit AttributeConsumingService
end
Builder->>XML: Produce final Metadata XML
XML-->>App: Return metadata XML
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/types.ts (1)
164-168: Remove unused CommonLanguageCode (prior concern stands)This type isn’t used and prior reviewers asked about its purpose. Recommend removal to avoid API bloat.
🧹 Nitpick comments (10)
src/saml.ts (1)
583-583: Prevent "undefined" and handle arrays in POSTed form inputsUse nullish coalescing to avoid clobbering falsy-but-valid values, and emit multiple inputs when a param is an array.
- return '<input type="hidden" name="' + k + '" value="' + quoteattr(String(samlMessage[k] || '')) + '" />'; + return '<input type="hidden" name="' + k + '" value="' + quoteattr(String(samlMessage[k] ?? '')) + '" />';If you want full array support:
- const formInputs = Object.keys(samlMessage) - .map((k) => { - return '<input type="hidden" name="' + k + '" value="' + quoteattr(String(samlMessage[k] || '')) + '" />'; - }) - .join("\r\n"); + const formInputs = Object.keys(samlMessage) + .flatMap((k) => { + const raw = (samlMessage as Record<string, unknown>)[k]; + const values = Array.isArray(raw) ? raw : [raw ?? ""]; + return values.map( + (val) => + '<input type="hidden" name="' + + k + + '" value="' + + quoteattr(String(val)) + + '" />', + ); + }) + .join("\r\n");README.md (2)
173-173: Clarify index uniqueness and default constraintsDocument that each AttributeConsumingService must have a unique @index and at most one can set @isdefault: true.
- - `metadataAttributeConsumingServices`: Optional. This parameter can be used to specify which attributes the Service Provider wants to consume. This adds AttributeConsumingService elements to the generated metadata, helping Identity Providers understand what user attributes should be included in SAML assertions. Each service can specify required and optional attributes, along with friendly names. + - `metadataAttributeConsumingServices`: Optional. This parameter can be used to specify which attributes the Service Provider wants to consume. This adds AttributeConsumingService elements to the generated metadata, helping Identity Providers understand what user attributes should be included in SAML assertions. Each service can specify required and optional attributes, along with friendly names. Note: each service must have a unique `@index` (unsigned integer), and at most one service should set `@isDefault: true`.
184-212: Tighten the example with spec nudges
- Consider a comment noting ServiceName is required, ServiceDescription optional.
- Optionally note that NameFormat defaults to urn:oasis:names:tc:SAML:2.0:attrname-format:uri if omitted.
src/types.ts (4)
128-151: Good set of common OIDs; consider adding a few widely used onesOptional: add UID (urn:oid:0.9.2342.19200300.100.1.1) and eduPersonPrincipalName (urn:oid:1.3.6.1.4.1.5923.1.1.1.6) for broader coverage.
152-163: Name format constants look good; wire the type throughYou already export SamlAttributeNameFormat—use it in RequestedAttribute["@NameFormat"] below.
170-178: Expose and use these aliasesConsider:
- RequestedAttribute["@name"]: CommonSamlAttributeName | string
- RequestedAttribute["@NameFormat"]: SamlAttributeNameFormat
-export type CommonSamlAttributeName = typeof COMMON_SAML_ATTRIBUTES[keyof typeof COMMON_SAML_ATTRIBUTES]; +export type CommonSamlAttributeName = + typeof COMMON_SAML_ATTRIBUTES[keyof typeof COMMON_SAML_ATTRIBUTES];
230-321: Tighten AttributeConsumingService typing
- Allow numeric @index for convenience (XML will still serialize as string).
- Use SamlAttributeNameFormat for @NameFormat.
- Optionally narrow @name to CommonSamlAttributeName | string.
export interface AttributeConsumingService { @@ - "@index": string; + "@index": string | number; @@ - "@Name": string; + "@Name": CommonSamlAttributeName | string; @@ - "@NameFormat"?: string; + "@NameFormat"?: SamlAttributeNameFormat;src/metadata.ts (1)
142-151: Guard against empty arrays and validate minimal schemaIf metadataAttributeConsumingServices is an empty array, we’ll emit an empty node array. Prefer skipping emission when length === 0, and optionally assert that each service has at least one ServiceName and one RequestedAttribute.
Apply:
- if (params.metadataAttributeConsumingServices) { + if (params.metadataAttributeConsumingServices?.length) { metadata.EntityDescriptor.SPSSODescriptor.AttributeConsumingService = params.metadataAttributeConsumingServices.map((service) => ({ "@index": service["@index"], ...(service["@isDefault"] ? { "@isDefault": service["@isDefault"] } : {}), ServiceName: service.ServiceName, ...(service.ServiceDescription ? { ServiceDescription: service.ServiceDescription } : {}), RequestedAttribute: service.RequestedAttribute, })); }test/attribute-consuming-service.spec.ts (2)
6-7: Reuse shared FAKE_CERTAvoid hardcoding a different fake cert; import the shared one from test/types for consistency.
Apply:
-import { SAML, SamlConfig, AttributeConsumingService } from "../src"; +import { SAML, SamlConfig, AttributeConsumingService } from "../src"; +import { FAKE_CERT } from "./types"; - - const FAKE_CERT = "fake cert";
145-227: Add edge-case test: empty array should omit the nodeCurrent code would emit an empty array; clarify behavior with a test and adjust implementation if needed.
Apply (add this new test near the end):
+ it("should not emit AttributeConsumingService when provided as an empty array", function (done) { + const samlConfig: SamlConfig = { + issuer: "http://example.serviceprovider.com", + callbackUrl: "http://example.serviceprovider.com/saml/callback", + idpCert: FAKE_CERT, + metadataAttributeConsumingServices: [], + generateUniqueId: () => "test-unique-id", + }; + const samlObj = new SAML(samlConfig); + const metadata = samlObj.generateServiceProviderMetadata(null); + parseString(metadata, function (err, result) { + if (err) return done(err); + const spssoDescriptor = result.EntityDescriptor.SPSSODescriptor[0]; + expect(spssoDescriptor.AttributeConsumingService).to.be.undefined; + done(); + }); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
README.md(2 hunks)src/index.ts(2 hunks)src/metadata.ts(1 hunks)src/saml.ts(1 hunks)src/types.ts(4 hunks)test/attribute-consuming-service.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/attribute-consuming-service.spec.ts (2)
test/types.ts (1)
FAKE_CERT(44-44)src/types.ts (2)
AttributeConsumingService(230-320)SamlConfig(434-434)
src/types.ts (1)
src/index.ts (7)
COMMON_SAML_ATTRIBUTES(48-48)SAML_ATTRIBUTE_NAME_FORMATS(49-49)CommonLanguageCode(50-50)CommonSamlAttributeName(51-51)SamlAttributeNameFormat(52-52)AttributeConsumingService(46-46)SamlOptions(33-33)
🪛 GitHub Actions: Build Status
test/attribute-consuming-service.spec.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix. Command: prettier --config .prettierrc.json --check .
src/types.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix. Command: prettier --config .prettierrc.json --check .
src/saml.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix. Command: prettier --config .prettierrc.json --check .
🔇 Additional comments (11)
src/saml.ts (1)
1-1: Fix Prettier failuresCI flagged formatting issues. Please run: prettier --write
src/types.ts (3)
362-389: Option addition LGTM; ensure builder validates spec constraintsLooks good. Please ensure metadata generation enforces: at least one ServiceName per service; unique @index across services; at most one @isdefault=true. If already covered by tests in test/attribute-consuming-service.spec.ts, we're good.
428-432: Param passthrough to generator LGTMCleanly exposes metadataAttributeConsumingServices via the params.
1-1: Fix Prettier failuresCI flagged formatting issues. Please run: prettier --write
src/metadata.ts (1)
142-151: LGTM: correct ACS metadata mappingAccurately mirrors AttributeConsumingService into the SPSSODescriptor, including optional isDefault and ServiceDescription. Tests cover the main paths.
src/index.ts (2)
19-26: Public API surface extension looks goodRe-exporting types/constants is fine and non-breaking at runtime.
46-53: Semver note: document as a minorAdding new exports should be released as a minor and called out in the changelog.
test/attribute-consuming-service.spec.ts (4)
1-4: Imports are fineUsing chai + xml2js here is consistent with the rest of the suite.
8-114: Strong end-to-end assertion coverageValidates presence, attributes, and multilingual nodes; good.
115-144: Absence case coveredCorrectly ensures no AttributeConsumingService is emitted when not provided.
1-228: Fix Prettier checkCI reports Prettier violations. Please run: prettier --write
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/types.ts (1)
164-179: Remove CommonLanguageCode (unused and arbitrary subset).This type isn’t used in the new interfaces and invites questions about omissions. Prefer plain string (BCP47) and let consumers pass any valid lang tag.
Apply within this file:
-/** - * Common language codes for ServiceName and ServiceDescription - */ -export type CommonLanguageCode = - | "en" - | "es" - | "fr" - | "de" - | "it" - | "pt" - | "ja" - | "ko" - | "zh" - | "ar" - | "ru";Also remove its re-export in src/index.ts:
- CommonLanguageCode,
🧹 Nitpick comments (5)
src/types.ts (3)
183-191: Consider using these unions in the interface for stronger typing.Use SamlAttributeNameFormat for RequestedAttribute["@NameFormat"] (see targeted diff below). Keep @name as string to allow non-common attributes.
243-333: Broaden @index to number | string and narrow @NameFormat to the provided union.This improves DX: callers can pass 0 or "0"; and IDEs will offer the format constants.
Apply:
- "@index": string; + "@index": number | string; @@ - "@NameFormat"?: string; + "@NameFormat"?: SamlAttributeNameFormat;
380-401: Add runtime validation for spec rules (unique index; ≤1 default).Type-level guarantees aren’t feasible here. In metadata generation, validate:
- all "@index" values are unique (per SPSSODescriptor)
- at most one "@isdefault" === true
Fail fast with a clear error.
If you want, I can draft the guard for src/metadata.ts.
test/attribute-consuming-service.spec.ts (2)
1-4: Use exported constants to avoid URN typos.Import SAML_ATTRIBUTE_NAME_FORMATS and use .URI instead of repeating the string.
-import { SAML, SamlConfig, AttributeConsumingService } from "../src"; +import { SAML, SamlConfig, AttributeConsumingService, SAML_ATTRIBUTE_NAME_FORMATS } from "../src"; @@ - "@NameFormat": "urn:oasis:names:tc:SAML:2.0:attrname-format:uri", + "@NameFormat": SAML_ATTRIBUTE_NAME_FORMATS.URI, @@ - "@NameFormat": "urn:oasis:names:tc:SAML:2.0:attrname-format:uri", + "@NameFormat": SAML_ATTRIBUTE_NAME_FORMATS.URI, @@ - "@NameFormat": "urn:oasis:names:tc:SAML:2.0:attrname-format:uri", + "@NameFormat": SAML_ATTRIBUTE_NAME_FORMATS.URI,Also applies to: 27-41
145-185: Optional: add a check for defaulting behavior (if implemented).If metadata generation defaults NameFormat to URI when omitted, add an assertion for one of these RequestedAttribute nodes. If not, ignore this.
Also applies to: 207-220
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/saml.ts(1 hunks)src/types.ts(4 hunks)test/attribute-consuming-service.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/saml.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/types.ts (1)
src/index.ts (7)
COMMON_SAML_ATTRIBUTES(48-48)SAML_ATTRIBUTE_NAME_FORMATS(49-49)CommonLanguageCode(50-50)CommonSamlAttributeName(51-51)SamlAttributeNameFormat(52-52)AttributeConsumingService(46-46)SamlOptions(33-33)
test/attribute-consuming-service.spec.ts (1)
src/types.ts (2)
AttributeConsumingService(243-333)SamlConfig(447-447)
🔇 Additional comments (4)
src/types.ts (4)
152-163: LGTM on NameFormat constants.Good centralization; see follow-up to narrow the interface field type to this union.
205-242: Docs are clear and aligned with SAML metadata.Nice example and spec link.
441-441: LGTM: params surface mirrors SamlOptions.Consistent pass-through for metadata generation.
128-151: No change needed for USER_PRINCIPAL_NAME OID
Verified thaturn:oid:1.2.840.113556.1.4.656matches the official Microsoft attributeId for userPrincipalName.
4 participants
This adds support for rich AttributeConsumingService metadata, fixing #406.
While it's "just XML" and folks could create their own, this approaches improves the developer experience with TypeScript and rich JSdoc hints that IDEs can service to developers to make the experience more efficient and enjoyable than crafting raw
XML by hand.
To support that, detailed JSDoc and TypeScript docs are included.
Recommend Testing
Try specifying the new attribute in an IDE like VS Code and check the experience. Intellisense should provide lots of hints
and docs for this now.
Context
SAML 2.0 Metadata Specification
Key Sections:
• Section 2.4.4: "Element md:AttributeConsumingService" (pages 23-25)
• Section *******: "Element md:ServiceName"
• Section *******: "Element md:ServiceDescription"
• Section *******: "Element md:RequestedAttribute"
XML Schema:
The XML Schema definition for AttributeConsumingService can be found in the SAML 2.0 Metadata Schema:
saml-schema-metadata-2.0.xsd
Key Points from the Spec:
• AttributeConsumingService is used by service providers to describe the attributes they would like to receive
• It's optional but highly recommended for proper attribute handling
• Each service can have multiple AttributeConsumingService elements with different purposes
• The index attribute is required and must be unique within the SPSSODescriptor
• The isDefault attribute indicates the default service for attribute consumption
The specification defines how identity providers should interpret these elements when generating attribute statements in SAML assertions.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests