feat(authorization): OAuth2 Group Sync#1266
feat(authorization): OAuth2 Group Sync#1266MisterErwin wants to merge 1 commit intoLibreBooking:developfrom
Conversation
|
Sorry in case I didn't follow the versioning changes from the contributing document, but it looks like that part of the contributing guidelines might be outdated, as I couldn't find an example in the recent merged PRs. |
There was a problem hiding this comment.
Pull request overview
Adds optional OAuth2-based group synchronization during SSO login so LibreBooking users can be assigned to existing LibreBooking groups based on IdP-provided group membership claims (Issue #837).
Changes:
- Introduces a new authentication config key to enable/disable OAuth2 group sync and specify the scope/claim used.
- Appends the configured groups scope to the OAuth2 authorization request scope.
- Passes group membership data from the OAuth2 userinfo response into the user synchronization/registration flow and updates docs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Config/ConfigKeys.php | Adds a new config key definition for OAuth2 groups syncing. |
| docs/source/Oauth2-Configuration.rst | Updates OAuth2 configuration example to include the new setting. |
| docs/source/ADVANCED-CONFIGURATION.rst | Documents the new advanced config key for OAuth2 group sync. |
| Presenters/LoginPresenter.php | Updates OAuth2 authorization URL generation to request the groups scope when configured. |
| Presenters/Authentication/ExternalAuthLoginPresenter.php | Extracts group memberships from userinfo and passes them through to user synchronization. |
|
Since copilot co-authored commits violate the commitizen rules: Should I squash all commits of this branch & force-push? |
Yes it would be nice if you could squash down to one commit please. Thanks. As the repository rules prevent being able to merge if the CI fails to pass. |
42a7f86 to
79cf8c3
Compare
No worries & thanks for the reply. |
|
@MisterErwin Thanks for the PR. I'll try to find some time over the next few days to review this. |
79cf8c3 to
20e8e27
Compare
|
What if used your new config option and it has a default value of And then it what will be used as the value for |
We also require the So we would need the I'll try to get around to this change (and fix the conflicts) |
9ac3ad0 to
fb515d1
Compare
|
I've (checks the avatar to make sure its really me) found some time to split the scope (aka From my PoV the only remaining/unresolved discussion is whether to add (mocked) tests for the OAuth2 flows |
Add the option to sync groups via OAuth2 SSO. If enabled, the configured scope is requested and consequently used to retrieve the group-memberships. Closes: LibreBooking#837
fb515d1 to
9b1aa26
Compare
Thanks from a quick glance this looks better. I will do a more thorough review this week. |
| $groups = null; | ||
| $groupsScope = Configuration::Instance()->GetKey(ConfigKeys::AUTHENTICATION_OAUTH2_GROUPS_CLAIM); | ||
| if (!empty($groupsScope)) { | ||
| $groupsClaim = $user[$groupsScope] ?? null; | ||
| if (is_array($groupsClaim)) { |
There was a problem hiding this comment.
$groupsScope is populated from AUTHENTICATION_OAUTH2_GROUPS_CLAIM and then used as the claim key in the userinfo array. The variable name (and later $groupsClaim) makes this easy to misread as an OAuth2 scope. Rename to something like $groupsClaimKey/$groupsClaimName to match what the value represents (aligns with repo naming guidance).
|
Here is Claude's review Code Review:
|
|
Codex review: Review: 9b1aa26 feat(authorization): OAuth2 Group SyncMerge RecommendationDo not merge as-is. The feature is useful and the changed surface area is fairly small, but the commit changes existing OAuth login behavior so that existing users now go through FindingsP1: Existing OAuth users can fail group sync when matched by email but not username
WHERE `username` = @username OR `email` = @emailHowever, the group update path then reloads the user only by the incoming username: $updatedUser = $this->userRepository->LoadByUsername($user->Username());If an existing LibreBooking account is matched by email while the OAuth Recommended fix: load the actual matched user by id, or otherwise carry the matched account identity through the sync path instead of reloading only by incoming username. P1: A valid empty OAuth group claim does not clear existing memberships
{ "groups": [] }should mean the user currently has no groups. But if (empty($userGroups)) {
return null;
}
Recommended fix: preserve the distinction between "group sync disabled / claim missing / claim malformed" and "claim present but empty." Empty-but-present should update the user to no matching groups. Did This Behavior Change in the Commit?Yes. Before this commit, existing external-auth users were logged in without calling if ($this->registration->UserExists($username, $email)) {
$this->authentication->Login(...);
}This commit changed the flow so existing users call if ($allowRegistration || $this->registration->UserExists($username, $email)) {
$this->registration->Synchronize(...);
$this->authentication->Login(...);
}The email-match/username-mismatch issue is newly exposed by this commit. The underlying The empty-group behavior also existed in shared registration code, but OAuth2 group sync is introduced by this commit, so this is a new OAuth2 feature bug. |
JohnVillalovos
left a comment
There was a problem hiding this comment.
I think the AI reviews are raising valid points about how the behavior changes. And I'm concerned it could break authentication for some providers.
Please let me know what you think? Thanks.
Add the option to sync groups via OAuth2 SSO.
If enabled, the configured scope is requested and
consequently used to retrieve the group-memberships.
Closes: #837