-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(core): Add SAML login setup #5515
feat(core): Add SAML login setup #5515
Conversation
# Conflicts: # packages/cli/src/controllers/auth.controller.ts
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.
Looks good, some minor changes or coments
* POST /sso/saml/acs | ||
* Assertion Consumer Service endpoint | ||
*/ | ||
samlController.post(SamlUrls.acs, async (req: express.Request, res: express.Response) => { |
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.
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.
updated and sort of works, but ends with ERR_MISSING_SIG_ALG issue. This may be an authentik setup issue though
Co-authored-by: Omar Ajoue <krynble@gmail.com>
Co-authored-by: Omar Ajoue <krynble@gmail.com>
Co-authored-by: Omar Ajoue <krynble@gmail.com>
| undefined | ||
> { | ||
const attributes = await this.getAttributesFromLoginResponse(req, binding); | ||
if (attributes.email) { |
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.
Maybe not for this iteration, but if email cannot be find as part of the mapping, we need to warn the user about it.
@@ -190,7 +190,7 @@ export function send<T, R extends Request, S extends Response>( | |||
try { | |||
const data = await processFunction(req, res); | |||
|
|||
sendSuccessResponse(res, data, raw); | |||
if (!res.headersSent) sendSuccessResponse(res, data, raw); |
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.
Is this change necessary since saml-related controllers are not using the annotations?
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.
yes it's for the default auth /# endpoint to be able to redirect to the initsso endpoint
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.
LGTM
Got released with |
* initial commit with sample data * basic saml setup * cleanup console logs * limit saml endpoints through middleware * basic login and token issue * saml service and cleanup * refactor and create user * get/set saml prefs * fix authentication issue * redirect to user details * merge fix * add generated password to saml user * update user from attributes where possible * refactor and fix creating new user * rename saml prefs key * minor cleanup * Update packages/cli/src/config/schema.ts Co-authored-by: Omar Ajoue <krynble@gmail.com> * Update packages/cli/src/config/schema.ts Co-authored-by: Omar Ajoue <krynble@gmail.com> * Update packages/cli/src/controllers/auth.controller.ts Co-authored-by: Omar Ajoue <krynble@gmail.com> * code review changes * fix default saml enabled * remove console.log * fix isSamlLicensed --------- Co-authored-by: Omar Ajoue <krynble@gmail.com>
Github issue / Community forum post (link here to close automatically):