kanboard / plugin-oauth2

Generic OAuth2 authentication plugin
MIT License
25 stars 32 forks source link

OAuth2 does not work with OwnCloud #17

Open 12delta opened 5 years ago

12delta commented 5 years ago

Actual behaviour

It is not possible to connect the OAuth2 Client Plugin to the OwnCloud OAuth2 Provider App. You get an External Authentication Error. Problem is that the OwnCloud expect the Client ID an Client Secret as Basic Authentication, but Kanboard send it as client_id and client_secret parameter.

Expected behaviour

It should work.

Steps to reproduce

Workaround

On fetching the Auth2 Token OwnCloud response with an error.

https://github.com/owncloud/oauth2/blob/fc47f947de78e7180f3c73455159683fb667dc89/lib/Controller/OAuthApiController.php#L114

Just patch https://github.com/kanboard/kanboard/blob/8cee04101d351fb5321f225963d589883761d214/app/Core/Http/OAuth2.php#L116 with this:

    public function getAccessToken($code)
    {
        if (empty($this->accessToken) && ! empty($code)) {
            $params = array(
                'code' => $code,
                'client_id' => $this->clientId,
                'client_secret' => $this->secret,
                'redirect_uri' => $this->callbackUrl,
                'grant_type' => 'authorization_code',
                'state' => $this->getState(),
            );

            $authBasic = 'Authorization: Basic ' . base64_encode($this->clientId . ':' . $this->secret);
            $response = json_decode($this->httpClient->postForm($this->tokenUrl, $params, array('Accept: application/json', $authBasic)), true);

            $this->tokenType = isset($response['token_type']) ? $response['token_type'] : '';
            $this->accessToken = isset($response['access_token']) ? $response['access_token'] : '';
        }

        return $this->accessToken;
    }

This adds the client_id and client_secret also as basic authentication.

Configuration

suniahk commented 3 years ago

It would probably be better to implement something in the UI to make it selectable, i.e. Basic Authentication vs Request Body. Increasing the size of the request with duplicated information, especially sensitive information, is a bad idea.

That being said, according to the spec on transmitting the client_id and client_secret, it's heavily preferred that you send these using the HTTP Basic Auth method rather than in the request body. Ultimately it's probably a better idea to enforce sending using basic auth since, from what I can tell, most oauth authentication servers already support this anyway.

12delta commented 3 years ago

I never saw OAuth2 authentication with a setting for the type like BasicAuth or RequestBody. I don't know, how other software determine the right way.

But I think, this request is only done once for the session initialization. Maybe size doesn't matter so much. According to the security, I wouldn't be concerned to have it twice in the the same request. Usually request bodies are not recorded and Authorization headers are not recorded for obvious reason.

It would make it more friendly, if there wouldn't be a new setting. This is just my suggestion.