Skip to content

Conversation

ivir
Copy link
Contributor

@ivir ivir commented Apr 3, 2020

With default SAML configuration wasn't possible use Windows Authentication via AD FS and everytime is offered only login via AD FS form. That is due missing 'requestedAuthnContext' set to false when
IDP choose available method. Instead of that is send request with "'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport" which on AD FS force to use form.
I just use setting from onelogin/php-saml/advanced_settings and provide configuration to .ENV file

Copy link

@tmaguire tmaguire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes issues when using Azure AD as a SAML idp with BookStack (I've had issues where if a user is already logged into Azure AD, they then can't log into BookStack, they've had to log out and then log back in).

@ssddanbrown ssddanbrown added this to the v21.04.4 milestone May 8, 2021
ssddanbrown added a commit that referenced this pull request May 8, 2021
Added tests to cover.
Changed default to align with existing default.
Added env option parsing.
For #1998
@ssddanbrown ssddanbrown merged commit b8e2d75 into BookStackApp:master May 8, 2021
@ssddanbrown
Copy link
Member

Thanks for this @ivir and sorry for my very very late response, Detailed SAML/Auth issues can be a pain to understand and test.

This is now merged, to be in the next patch release, after some tweaks in 9cf4191.
I changed the default from false to true since that is what I found the existing library default to be after digging into things. Also added some testing to cover this and some .env option parsing to allow the muli-value array input that the SAML library accepts.

I could not get any error or unexpected scenario to occur on my instance of Azure AD but I think it may depend on the exact AD setup and user authentication mechanisms use.


For my own reference:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants