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

OpenID Connect: Use group details from user_info endpoint #3873

Closed
107142 opened this issue Nov 25, 2022 · 7 comments
Closed

OpenID Connect: Use group details from user_info endpoint #3873

107142 opened this issue Nov 25, 2022 · 7 comments

Comments

@107142
Copy link

107142 commented Nov 25, 2022

Describe the Bug

It seems the application only parses the id_token when enumerating group claims but not the userinfo endpoint resulting in missing groups when user_info is in use.
We have a large amount of custom claims containing lots of groups making usage of id_token impossible (as its size would be simply too much).

Steps to Reproduce

  1. Make sure you IdP uses user_info to send claims with groups
  2. Configure OIDC to sync groups
  3. Dump user detail upon login

Expected Behaviour

A list of groups should be returned.

Browser Details

Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Firefox/102.0

Exact BookStack Version

v22.10.2

PHP Version

8.1.12

Hosting Environment

Rancher Kubernetes

Docker image: solidnerd/bookstack:latest

Clean install

@107142 107142 changed the title OpenID Connect: Empty group list (when it shouldn't be) OpenID Connect: Not parsing user_info resuls in missing groups Nov 25, 2022
@107142 107142 changed the title OpenID Connect: Not parsing user_info resuls in missing groups OpenID Connect: Not parsing user_info results in empty groups Nov 25, 2022
@107142
Copy link
Author

107142 commented Nov 25, 2022

Updated the description as at first I thought there is an issue with the groups claim parsing logic. But after going through the source it seems like the issue is we do not include groups in id_token, but in user_info and that does not seem to be used.

@ssddanbrown
Copy link
Member

I'll re-label this as an auth feature request, since we currently don't currently support gaining extra claims from the user_info endpoint as you have found, and it's not a bug in existing logic.

@ssddanbrown ssddanbrown changed the title OpenID Connect: Not parsing user_info results in empty groups OpenID Connect: Use group details from user_info endpoint Feb 6, 2023
@felixschloesser
Copy link

felixschloesser commented Feb 22, 2023

As this issue was a showstopper for my organisation I hacked something together which works for my needs. I know next to nothing about lavarel or php; so I dont consider this PR worthy, but maybe it can be a starting point for someone else.

I overhauled the get user details function to use the access token to query both name and groups form the userinfo endpoint as defined by the provider. As I didnt get it to work with the Psr\Http\Client\ClientInterface I just resorted to curl.

OidcService.php:
    /**
     * Query the user info endpoint for the user's details.
     *
     * @return array{name: string, email: string, external_id: string, groups: string[]}
     */
    protected function getUserInfo(OidcAccessToken $accessToken): ?array 
    {
        $settings = $this->getProviderSettings();
        $user_info_endpoint = $settings->userInfoEndpoint;

        $headers = [
            'Authorization: Bearer ' . $accessToken,
            'Content-Type: application/json',
        ];

        $curl = curl_init();
        curl_setopt_array($curl, [
            CURLOPT_URL => $user_info_endpoint,
            CURLOPT_RETURNTRANSFER => true,
            CURLOPT_HTTPHEADER => $headers,
        ]);

        $responseBody = curl_exec($curl);

        curl_close($curl);

        $userinfo = json_decode($responseBody, true);

        return $userinfo ?? null;
    }
    


    /**
     * Get the user's name using the access token.
     *
     * @return string
     */
    protected function getUserName(OidcAccessToken $token): string
    {
        $nameAttr = $this->config()['display_name_claims'] ?? 'name';

        // Get the user info from the endpoint
        $userInfo = $this->getUserInfo($token);

        if (is_array($nameAttr)) {
            $name = '';
            foreach ($nameAttr as $attr) {
                if (isset($userInfo[$attr])) {
                    $name = $userInfo[$attr];
                    break;
                }
            }
        } else {
            $name = $userInfo[$nameAttr] ?? 'Anonymous';
        }

        return $name;
    }


    /**
     * Get the user's groups using the access token.
     *
     * @return string[]
     */
    protected function getUserGroups(OidcAccessToken $token): array
    {
        $groupsAttr = $this->config()['groups_claim'];
        if (empty($groupsAttr)) {
            return [];
        }

        // $groupsList = Arr::get($token->getAllClaims(), $groupsAttr);
        // Instead we use the user info endpoint
        $userInfo = $this->getUserInfo($token);
        $groupsList = $userInfo[$groupsAttr];
        
        if (!is_array($groupsList)) {
            return [];
        }

        return array_values(array_filter($groupsList, function ($val) {
            return is_string($val);
        }));
    }

    /**
     * Extract the details of a user from an ID token.
     *
     * @return array{name: string, email: string, external_id: string, groups: string[]}
     */
    protected function getUserDetails(OidcIdToken $token, OidcAccessToken $accessToken): array
    {
        $idClaim = 'sub';
        $id = $token->getClaim($idClaim);

        return [
            'external_id' => $id,
            'email'       => $token->getClaim('email'),
            'name'        => $this->getUserName($accessToken),
            'groups'      => $this->getUserGroups($accessToken),
        ];
    }

I also adjusted AppServiceProvider.php to include the userinfo endpoint: See gist.

@107142
Copy link
Author

107142 commented Feb 24, 2023

@ssddanbrown
Would you accept a PR with added functionality provided you are satisfied with the code quality?
I'm not referring to the code above, but ATM we are possibly considering allocating a dev for this particular functionality (no promise though as the decision does not lie with me).

@ssddanbrown
Copy link
Member

@107142 I would be willing to review a PR for this, ideally with the following in mind:

  • The implementation should not affect existing OIDC usages/flows.
  • The scope and surface area of the addition should be minimal, and accept group info in the same format as we accept in the ID_TOKEN currently (Which I think is a simple array of strings, which could be nested in a JSON structure).
  • Feature additions should be covered by tests.

I'm happy to provide pointers/direction/general-help where needed.
Just shout if anything is started and I can assign this issue as required.

@ssddanbrown
Copy link
Member

Just a note on this, v23.05 added a new logical theme event which I believe could be used to call the user_info endpoint (Or any other endpoint/data-source) to supplement ID token data. I specifically ensured that the added event was passed access token data so this kind of thing was made possible.

Details in #4200.
Just shout if you'd like an example.

@ssddanbrown
Copy link
Member

With work done in #4726 and #4955 BookStack will now use the userinfo endpoint, where expected details are missing from the IDToken. This will be part of the next feature release and I'll therefore close off this request.

Note: If the auth system still provides data for expected claims in the ID token then BookStack will just use ID Token data unless any expected claims are missing. If you need userinfo endpoint claims, but can't alter the ID token to stop that behaviour, it is possible to nullify ID token claims before the userinfo look using the logical theme event I shared above. Happy to provide an example of how that would look upon request.

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

No branches or pull requests

3 participants