Feat/saml auth#8201
Conversation
|
This pull request has a conflict. Could you fix it @seanlongcc? |
There was a problem hiding this comment.
should probably dig a little bit more to find out where the logo was originally created just to confirm it is actually the SAML format logo as opposed to anything else
There was a problem hiding this comment.
based off of discussion it seems like this is an unofficial logo so we probably shouldn't use it. instead we should come up with some generic logo or not provide a logo at all.
There was a problem hiding this comment.
saml is not oauth so remove it from the list
| return enabledOauth; | ||
| } | ||
|
|
||
| frontendStartupSettings(): StartupSettingsDto { |
There was a problem hiding this comment.
actually maybe the thing that makes the most sense is to adjust this dto to reflect auth strategies more broadly
Dto({
...all the other stuff,
authStrategies: {
localLoginEnabled: this.isLocalLoginAllowed(),
ldap: this.get('LDAP_ENABLED')..., // maybe we should change the name to be consistent with the 'Enabled' suffix
...(make an object from the supportedOauth array where the keys are the array items and the values are if it is enabled or not),
samlEnabled: this.isSamlAllowed() // implement this func
},
})
so that it looks like this when processed:
Dto({
...all the other stuff,
authStrategies: {
localLoginEnabled: true,
ldapEnabled: false,
githubEnabled: true,
gitlabEnabled: false,
...
samlEnabled: true
},
})
| @@ -37,11 +37,14 @@ | |||
| enabledOauthStrategies() { | |||
There was a problem hiding this comment.
instead of returning an array of the valid functions, make it return a map where the key is the oath strategy name and the value is whether or not it's enabled
There was a problem hiding this comment.
you're going to have to look for all the places that oauthstrategies was used and make sure that it can handle the new data format
| } | ||
|
|
||
| @Injectable() | ||
| export class SAMLStrategy extends PassportStrategy(Strategy as any, 'saml') { |
There was a problem hiding this comment.
try to remove 'as any' cause sometimes the strategy 'shape'/type matches the new passportstrategy type. other times, especially older / less maintained strategies need to use the 'as any' escape mechanism.
| private readonly authnService: AuthnService, | ||
| private readonly configService: ConfigService, | ||
| ) { | ||
| super({ |
There was a problem hiding this comment.
add support for all the other config options for SAML
especially make sure that any of the certs / proxies / etc options are defined
| const email = samlClaimAttribute(profile, emailAttribute); | ||
| const firstName = samlClaimAttribute(profile, firstNameAttribute); | ||
| const lastName = samlClaimAttribute(profile, lastNameAttribute); | ||
| if (email && email.length > 0) { |
There was a problem hiding this comment.
we also need to validate that first name and last name were provided properly, not just email
| const email = samlClaimAttribute(profile, emailAttribute); | ||
| const firstName = samlClaimAttribute(profile, firstNameAttribute); | ||
| const lastName = samlClaimAttribute(profile, lastNameAttribute); |
There was a problem hiding this comment.
if any of these are undefined we should throw an error at this point stating that the user did not configure the envvars properly
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
seanlongcc
force-pushed
the
feat/saml-auth
branch
from
f1077b6 to
9c740c2
Compare
|
4 participants
No description provided.