Export generateServiceProviderMetadata #337
Conversation
src/metadata.ts
Outdated
| decryptionPvk, | ||
| privateKey, | ||
| metadataContactPerson, | ||
| metadataOrganization, | ||
| generateUniqueId, | ||
| // The below defaults are selected to match the defaults in the `SAML` class. | ||
| identifierFormat = "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress", |
There was a problem hiding this comment.
I'd prefer to keep all the defaults in a single location. If they need to be duplicated here, then perhaps we should put a run-time check to make sure they are populated and leave the onus on the one calling this method directly to know what their doing.
There was a problem hiding this comment.
Is there any particular way you'd like me to share these defaults? Maybe declare them in a constant somewhere? Is there precedent for this?
I'd rather not leave this to the caller; if I'm not explicitly configuring identifierFormat for instances of SAML, I wouldn't expect to have to configure them here either.
There was a problem hiding this comment.
I think I might be OK with a set of constants in a new file: constants.ts. I see we've already polluted the types.ts file a little with a class that should probably be in utilities.ts, but I don't want that to be a pattern. It also doesn't seem to be a great fit for utilities.ts. I'm interested in your thoughts.
I really want to make sure that the next guy (or future me) doesn't think there is just one place for defaults and forgets to change something.
There was a problem hiding this comment.
I think your motivation is spot on. I think a constants.ts file with exports like DEFAULT_IDENTIFIER_FORMAT and DEFAULT_WANT_ASSERTIONS_SIGNED would be a good idea; I'll make that change if you're good with it!
|
Looks like some tests were failing in CI. However, they also fail on |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #337 +/- ##
==========================================
+ Coverage 82.82% 83.03% +0.20%
==========================================
Files 11 12 +1
Lines 821 831 +10
Branches 250 253 +3
==========================================
+ Hits 680 690 +10
Misses 58 58
Partials 83 83 ☔ View full report in Codecov by Sentry. |
|
@nwalters512 , I think everything checks out now. Can you give this a final once-over to make sure that the merge I did still looks good? I want to do a release and this code is close to landing, so I want to include it. |
|
@cjbarth thanks for getting this up to date with master. This looks ready to go to me! |
Description
This PR makes the change proposed in #336 and directly exports
generateServiceProviderMetadata. This is useful for generating metadata without constructing a complete and validSAMLinstance.For this function to be maximally useful on its own, I made some changes so that it would behave the same as if it were used on an instance of
SAML. That is, for some set of options,new SAML(options).generateServiceProviderMetadata() === generateServiceProviderMetadata(options)should be true. This mostly entailed making additional options optional and providing defaults that matched those set inSAML#initialize().Checklist: