Skip to content
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

dump_user_details for SAML does not include groups in the attrs_after_parsing #4706

Closed
akkornel opened this issue Dec 1, 2023 · 3 comments
Closed

Comments

@akkornel
Copy link

akkornel commented Dec 1, 2023

Describe the Bug

When debugging SAML login with SAML2_DUMP_USER_DETAILS (dump_user_details), in the attrs_after_parsing section, no group information is listed. This happens even if group sync (SAML2_USER_TO_GROUPS) is enabled.

Steps to Reproduce

  1. Configure Bookstack for SAML2: As part of the setup, make sure SAML2_GROUP_ATTRIBUTE is defined and SAML2_USER_TO_GROUPS is set to true, so that group sync is enabled.
  2. Log in via SAML2, through an IdP that provides groups.
  3. Check out the JSON that is returned at the end of the login process.

Expected Behaviour

In the attrs_from_idp part of the JSON, I see my group membership in the appropriate SAML attribute coming from the IdP.

I expect to see the parsed group membership in the attrs_after_parsing part of the JSON, but I do not.

Screenshots or Additional Context

It looks like the problem is within Saml2Service:processLoginCallback. In the JSON dump I see that attrs_after_parsing is coming from a call to getUserDetails, but getUserDetails is not providing a parsed list of groups.

Looking later in Saml2Service:processLoginCallback, I see the groups are being parsed later, after the JSON dump, by a call to getUserGroups.

I'm a new user of Bookstack, so even though I can see the cause of the problem, I don't know the best way of fixing it. Hence the report!

Browser Details

n/a

Exact BookStack Version

23.10.4

@ssddanbrown
Copy link
Member

Thanks for reporting @akkornel,
I can see how someone may expect to see the groups within the parsed data when active, so I'll look to parse the groups out earlier if enabled, to allow them to be part of the dumped data.
Have assigned to address for the next feature release.

ssddanbrown added a commit that referenced this issue Dec 3, 2023
Updated code style of class while there.
Removed redundant check and string translation used.

For #4706
@ssddanbrown
Copy link
Member

This has now been addressed within 1185336, which will be part of the next feature release.
Thanks again @akkornel for raising.

@akkornel
Copy link
Author

akkornel commented Dec 3, 2023

That's awesome, thanks very much!

akkornel added a commit to stanford-rc/docker-bookstack-certbot that referenced this issue Mar 2, 2024
See https://github.com/BookStackApp/BookStack/releases/tag/v23.12 for
the list of improvements for 23.12.  The .1 and .2 releases are smaller
bugfixes, and .3 brings a security fix related to PDF generation.

This also brings in two SAML-related improvements
(BookStackApp/BookStack#4713 and BookStackApp/BookStack#4706).  Thanks
again ssddanbrown for them!  And thanks also to the LinuxServer folks
for the packaging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants