-
Notifications
You must be signed in to change notification settings - Fork 475
Require cert for every strategy #548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
ff2f062
4080a51
4e7a874
1c169db
c97a599
dd72c77
b7a8c5b
020f97d
5a136f8
3e1386e
ef38d58
3ccd30f
6dea742
9fcc972
161c444
6642d6a
5ca517e
060e2d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,3 +2,5 @@ | |
node_modules | ||
lib | ||
package-lock.json | ||
.eslintcache | ||
.prettierignore |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,35 +8,35 @@ import { | |
AuthorizeOptions, | ||
MultiSamlConfig, | ||
RequestWithUser, | ||
SamlConfig, | ||
VerifyWithoutRequest, | ||
VerifyWithRequest, | ||
} from "./types"; | ||
|
||
class MultiSamlStrategy extends SamlStrategy { | ||
_options: MultiSamlConfig; | ||
static newSamlProviderOnConstruct = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would find it more elegant to have it as a protected instance field. The current way forces an explicit cast, which should always be avoided IMO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And normally I would agree with you, but we can't have an instance field before the call to See "src/passport-saml/multiSamlStrategy.ts:23:3 - error TS2376: A 'super' call must be the first statement in the constructor when a class contains initialized properties, parameter properties, or private identifiers." |
||
|
||
_options: SamlConfig & MultiSamlConfig; | ||
|
||
constructor(options: MultiSamlConfig, verify: VerifyWithRequest); | ||
constructor(options: MultiSamlConfig, verify: VerifyWithoutRequest); | ||
constructor(options: MultiSamlConfig, verify: never) { | ||
if (!options || typeof options.getSamlOptions != "function") { | ||
if (!options || typeof options.getSamlOptions !== "function") { | ||
throw new Error("Please provide a getSamlOptions function"); | ||
} | ||
|
||
if (!options.requestIdExpirationPeriodMs) { | ||
options.requestIdExpirationPeriodMs = 28800000; // 8 hours | ||
} | ||
|
||
if (!options.cacheProvider) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
options.cacheProvider = new InMemoryCacheProvider({ | ||
keyExpirationPeriodMs: options.requestIdExpirationPeriodMs, | ||
}); | ||
} | ||
// Force the type on this since we've disabled `newOnConstruct` | ||
// so the `SAML` constructor will not be called at this time | ||
// and there are defaults for all `strategy`-required options. | ||
const samlConfig = { | ||
...options, | ||
} as SamlConfig & MultiSamlConfig; | ||
|
||
super(options, verify); | ||
this._options = options; | ||
super(samlConfig, verify); | ||
this._options = samlConfig; | ||
} | ||
|
||
authenticate(req: RequestWithUser, options: AuthenticateOptions) { | ||
authenticate(req: RequestWithUser, options: AuthenticateOptions): void { | ||
this._options.getSamlOptions(req, (err, samlOptions) => { | ||
if (err) { | ||
return this.error(err); | ||
|
@@ -90,6 +90,11 @@ class MultiSamlStrategy extends SamlStrategy { | |
); | ||
}); | ||
} | ||
|
||
// This is reduntant, but helps with testing | ||
error(err: Error): void { | ||
super.error(err); | ||
} | ||
} | ||
|
||
export = MultiSamlStrategy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me what this line does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It allows us to preserve all incoming options, while simultaneously converting the option from
partial<CacheProviderOptions>
toCacheProviderOptions
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I would find it clearer with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made an adjustment to clarify this code block. Please have a look at this change.