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

scope parameter is not returned by token exchange #2

Closed
hcrohland opened this issue Sep 30, 2019 · 10 comments
Closed

scope parameter is not returned by token exchange #2

hcrohland opened this issue Sep 30, 2019 · 10 comments
Labels
enhancement New feature or request noncompliant Issue is caused by a noncompliant provider

Comments

@hcrohland
Copy link
Contributor

Hi Jeb,

just started using your crate.

Your code assumes that the scope parameter is returned by the token exchange.
But the RFC states that it will be transmitted with the authorization callback.
So TokenResponse.scope is always none.

I am willing to work on a patch but I am unsure how to do it:

  • Fill TokenResponse.scope in fn handle or...
  • Change the callback interface to have an explicit parameter and remove the scope from struct TokenResponse.

The first one should be easy. But this field does not really belong in TokenResponse. And if we wanna add Refresh token support it would not be able to fill the scope field.

What do you think?

Cheers,
Christoph

@jebrosen
Copy link
Owner

But the RFC states that it will be transmitted with the authorization callback.

Where in the RFC does it state that? This crate only implements the Authorization Code Grant, which does require scope to be specified in the token response if it differs from the requested scope.

@hcrohland
Copy link
Contributor Author

In https://tools.ietf.org/html/rfc6749#section-3.3 it states:
"The authorization and token endpoints allow the client to specify the
scope of the access request using the "scope" request parameter. In
turn, the authorization server uses the "scope" response parameter to
inform the client of the scope of the access token issued.
"
So only if you send a scope parameter the endpoints will answer with scope. The code only sends scope to the authorization endpoint, not to the token endpoint. So it only gets it back for authorization.

And this all makes sense to me. The user selects scope during authorization. He is not involved in token exchange. So we need to check scope after authorization.

@jebrosen
Copy link
Owner

I am not seeing where my implementation differs from the Authorization Code grant:

4.1.1. Authorization Request (rocket_oauth2 produces a redirect to authorization_uri(), including client_id, redirect_uri, and scope)
4.1.2. Authorization Response (the authentication server produces a redirect to the redirect_uri with code and state)
4.1.3. Access Token Request (rocket_oauth2 calls exchange_code(), passing code, state, and redirect_uri to the server)
4.1.4. Access Token Response (the server returns the fields in TokenResponse).

If you have example code that should be receiving a scope but isn't, that would be helpful.

@hcrohland
Copy link
Contributor Author

Look at the pull request I created. It implements the first option.

Your code does not parse the scope response from the auth callback and by that looses the information. When I request authorization from strava scope was always None with your code. With the proposed change it is filled with the user selected scope.

@hcrohland
Copy link
Contributor Author

hcrohland commented Sep 30, 2019

Here is my oauth code:

`use rocket_oauth2::{Callback, OAuth2, OAuthConfig, TokenResponse};
use rocket_oauth2::hyper_sync_rustls_adapter::HyperSyncRustlsAdapter;

use rocket::response::Redirect;
use rocket::;
use rocket::http::
;
use super::Config;
use reqwest::Url;
use crate::*;

// use super::error::ResultExt;
// use super::error::Result as MyResult;

fn callback(request: &Request, token: TokenResponse)
-> Result<Redirect, Box>
{

info!("got token {:?}", token);
if let Some(athlete) = token.extras.get("athlete") {
    info!("got athlete {} {}, with id {}", athlete["firstname"], athlete["lastname"], athlete["id"])
};
let mut cookies = request.guard::<Cookies>().expect("request cookies");

// Set a private cookie with the access token
cookies.add_private(
    Cookie::build("token", token.access_token)
        .same_site(SameSite::Lax)
        .finish()
);
Ok(Redirect::to("/"))

}

pub fn init_auth () -> impl rocket::fairing::Fairing {
let provider = rocket_oauth2::Provider {
auth_uri: "https://www.strava.com/oauth/authorize".into(),
token_uri: "https://www.strava.com/oauth/token".into()
};
let config = OAuthConfig::new(provider,
std::env::var("CLIENT_ID").expect("Couldn't read var CLIENT_ID"),
std::env::var("CLIENT_SECRET").expect("Couldn't read var CLIENT_SECRET"),
"http://localhost:8001/token".into());

OAuth2::custom(HyperSyncRustlsAdapter, callback, config, "/token", Some(("/login", vec!["activity:read_all".to_string()])))

}
`

@jebrosen
Copy link
Owner

After reading https://developers.strava.com/docs/authentication/ I can only conclude that Strava is either not compliant with the RFC or is not actually using the Authorization Code Grant. They do specify that they send scope in the authorization callback and not in the token response, which is not to spec.

https://tools.ietf.org/html/rfc6749#section-4.1.2 clearly lists only code and state as valid authorization callback parameters, and also says:

The client MUST ignore unrecognized response parameters.


I don't like the idea of adding lots of workarounds for noncompliant authentication servers, but unfortunately it seems necessary. I will think about the best way to include this change.

@hcrohland
Copy link
Contributor Author

But it is in sync with section 3.3.
The RFC seems to be written ambiguously

@hcrohland
Copy link
Contributor Author

I added a commit to only use the auth callback's scope if the token exchange did not return one.

@jebrosen
Copy link
Owner

But it is in sync with section 3.3.
The RFC seems to be written ambiguously

I don't see any ambiguity. Section 3.3 only says "the authorization server uses the "scope" response parameter" but does not specify where the response parameter appears. 4.1.4. specifies that it is part of the token response.

Nonetheless, I am going to accept an "early" scope in the auth callback if it is not provided in the token response.

@jebrosen jebrosen added the enhancement New feature or request label Oct 1, 2019
@jebrosen
Copy link
Owner

jebrosen commented Oct 1, 2019

0.1.0 is now available, including a fix for this issue.

@jebrosen jebrosen closed this as completed Oct 1, 2019
@jebrosen jebrosen added the noncompliant Issue is caused by a noncompliant provider label Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request noncompliant Issue is caused by a noncompliant provider
Projects
None yet
Development

No branches or pull requests

2 participants