oauth2-rs icon indicating copy to clipboard operation
oauth2-rs copied to clipboard

Mandatory auth_url and 4.0

Open jeroenvervaeke opened this issue 4 years ago • 3 comments

Currently the auth_url is mandatory when creating a new Client: doc However, auth url isn't mandatory when using the Client Credentials Grant and the Resource Owner Password Credentials Grant.

Since you're introducing 4.0 semantic versioning would allow us to introduce a breaking change. I propose to make the auth_url optional in the new method of a client, this way you don't need to enter a valid garbage url when you're using client credentials. Before:

let client =
    BasicClient::new(
        ClientId::new("client_id".to_string()),
        Some(ClientSecret::new("client_secret".to_string())),
        AuthUrl::new("http://garbage_url".to_string())?,
        Some(TokenUrl::new("http://token".to_string())?),
    );

After:

let client =
    BasicClient::new(
        ClientId::new("client_id".to_string()),
        Some(ClientSecret::new("client_secret".to_string())),
        None,
        Some(TokenUrl::new("http://token".to_string())?),
    );

In a later version we could add a BasicClientBuilder struct to avoid having to provide a lot of None values to the Client

jeroenvervaeke avatar Mar 31 '21 15:03 jeroenvervaeke

hey @jeroenvervaeke,

this brings up an interesting API design issue that I've struggled with a few times in the past. you're correct that the authorization endpoint isn't required for the grants you mentioned. this came up with some of the newer endpoints (see #127), and we introduced a ConfigurationError type for those.

my worry with making the auth endpoint optional is that it'll create a situation in which code that omits it but then tries to make a request that requires it won't fail until runtime. ideally, the type system would be able to enforce this. this is already an issue for the newer endpoints like token introspection, but now it would be affecting the most common OAuth2 flows as well.

I think we'd be better off exploring an alternative solution, such as allowing a ClientCredentialsTokenRequest to be constructed without first constructing a Client.

separately, this issue just missed the cutoff for breaking changes in 4.x since I promoted the release to beta a couple of weeks ago (see #99).

thoughts?

ramosbugs avatar Mar 31 '21 23:03 ramosbugs

I have a specific solution in mind that I would like to approach. I might make a POC and come back to it later. The solution that I have in mind uses the type system to avoid incorrect configuration & avoids using the Client

jeroenvervaeke avatar Apr 01 '21 21:04 jeroenvervaeke

@ramosbugs @ramosbugs, any updates on this? Even I am in the same situation where the auth_url is not required.

vidhatha avatar Feb 13 '23 08:02 vidhatha