jebrosen / rocket_oauth2

OAuth2 for Rocket applications
Apache License 2.0
67 stars 26 forks source link

Reddit configuration not actually working #18

Closed DependentAppearance0 closed 4 years ago

DependentAppearance0 commented 4 years ago

By default rocket_oauth2 doesn't work with the reddit config it provides. This is because on the exchange_code function reddit expects the client id and secret to be provided using a Authorization header. Reddit details the requirements here but basically using the default code reddit will just return a 401 because the Authorization header isn't provided

I was able to get reddit logon working by replacing lines 76-85 in hyper_sync_rustls_adapter.rs with

        //ser.append_pair("client_id", config.client_id());
        //ser.append_pair("client_secret", config.client_secret());

        let req_str = ser.finish();

        let request = client
            .post(config.provider().token_uri().as_ref())
            .header(Accept::json())
            .header(ContentType::form_url_encoded())
            .header(Authorization(
                Basic {
                    username: config.client_id().to_owned(),
                    password: Some(config.client_secret().to_owned()),
                }
            ))
            .body(&req_str);

also its impossible to get an refresh token with reddit as its api requires you to tell it during authorization whenever you want a temporary or permanent token using a duration parameter in the request. I was also able to workaround this by just hardcoding the authorization request to set the duration. I think this wouldn't be a problem if issue #13 got resolved as then duration could be sent using an custom parameter.

also unrelated the version of rocket_sync_rustls should probably be updated as it uses an old version of rustls. This caused problems for me as i use another lib that uses a newer version of rustls and I could not compile as two versions of ring-asm were required and as ring-asm is a native library it can only have one compiled version. I was able to fix this by just changing the cargo.toml to use version =0.3.0-rc.17 instead of =0.3.0-rc.4

jebrosen commented 4 years ago

By default rocket_oauth2 doesn't work with the reddit config it provides.

Well shoot, I thought I had tested or checked all of these. It looks like the use of basic authentication is allowed and even recommended by https://tools.ietf.org/html/rfc6749#section-2.3.1; I will look into implementing this.

I think this wouldn't be a problem if issue #13 got resolved as then duration could be sent using an custom parameter.

:+1:

also unrelated the version of rocket_sync_rustls should probably be updated as it uses an old version of rustls

Thanks for the reminder! This was restricted to whatever version of rocket_sync_rustls used the same version of ring as cookie, but now that cookie does not depend on ring this can be updated. I still consider it a breaking change, though, so I will include the change in a 0.4 release.

jebrosen commented 4 years ago

This was restricted to whatever version of rocket_sync_rustls used the same version of ring as cookie, but now that cookie does not depend on ring this can be updated.

I'm actually still reluctant to do this. With the tls feature enabled, rocket still depends on hyper-sync-rustls 0.3.0-rc.4 so this would create a conflict in the different ring versions.

jebrosen commented 4 years ago

I've published a commit that hopefully resolves this issue, additionally tested against the providers in examples/user_info_hyper_sync_rustls. You can try it by adding a patch specification to Cargo.toml:

[patch.crates-io]
rocket_oauth2 = { git = "https://github.com/jebrosen/rocket_oauth2", rev = "c4519e40" }

In testing I have discovered that at least one provider so far (Nextcloud) doesn't actually support HTTP Basic auth even though it is required, so I will do some more investigation before merging it since it could accidentally break current users.

DependentAppearance0 commented 4 years ago

Hi I tested the patch you provided to me and it appears to work perfectly unless you need permanent access to reddits api.

I do require permanent access so I think I will keep using my own workaround with a hardcoded parameter for the duration until its possible to pass custom parameters with the request. But ignoring the problem with permanent access this patch appears to work perfectly for reddit.

jebrosen commented 4 years ago

Thanks for helping test this! I've implemented it as an option, and I plan to include this and a solution for #13 in a 0.4.x release.