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

OAuth1: Allow empty customer secrets, according to the specifications. #1596

Merged
merged 2 commits into from Jul 15, 2021

Conversation

JamesMenetrey
Copy link
Contributor

@JamesMenetrey JamesMenetrey commented Jun 13, 2021

Description

OAuth 1.0a specifications allow to use of an empty custom secret (see [here](https://oauth.net/core/1.0a/, section 4: The Consumer Secret MAY be an empty string [..]). Similarly to #776, I needed to call MAAS API, where the customer secret must not be provided (0-legged OAuth). This pull request removes the constraint of providing customer secrets for OAuth1.

This has been tested over the API of MAAS, which does not require any customer secret.

Purpose

This pull request is a:

  • Bugfix (non-breaking change which fixes an issue)

Checklist

  • I have added tests that prove my fix is effective or that my feature works (added one to verify that the customer secret can be empty).
  • I have added necessary documentation (explained the purpose of this PR on a comment over the provided unit test).

Don't hesitate to ask for changes in that pull request if needed.

Cheers

Signed-off-by: Jämes Ménétrey <james@menetrey.me>
@patrickhampson
Copy link
Contributor

patrickhampson commented Jun 17, 2021

Hey James,

Funny, I stumbled upon this as I was opening my own PR for this exact MAAS api issue!

I opened #1597 explaining the behavior a bit but wanted to comment on this PR rather than opening a duplicate:

  • I recommend leaving the OAuth consumerSecret parameter as a regular string vs the nullable type you changed it to. This should reduce the impact of this change as it would be a signature change vs an internal validation change.

  • It might also be beneficial to modify the unit test to account for the different OAuthTypes:

[Test]
[TestCase(OAuthType.AccessToken)]
[TestCase(OAuthType.ProtectedResource)]
[TestCase(OAuthType.AccessToken)]
[TestCase(OAuthType.ProtectedResource)]
public void Authenticate_ShouldAllowEmptyConsumerSecret_OnHttpAuthorizationHeaderHandling(OAuthType type)
{
    // Arrange
    const string url = "https://no-query.string";

    var client = new RestClient(url);
    var request = new RestRequest();
    _authenticator.Type           = type;
    _authenticator.ConsumerSecret = "";

    // Act
    _authenticator.Authenticate(client, request);

    // Assert
    var authParameter = request.Parameters.Single(x => x.Name == "Authorization");
    var value = (string)authParameter.Value;

    Assert.IsNotEmpty(value);
    Assert.IsTrue(value!.Contains("OAuth"));
    Assert.IsTrue(value.Contains("oauth_signature=\"" + OAuthTools.UrlEncodeStrict("&")));
}

@JamesMenetrey
Copy link
Contributor Author

JamesMenetrey commented Jun 18, 2021

Hello Patrick,

Many thanks for your feedback and I am happy to see some tractions around this use case! I have applied your suggestion regarding the enhancement of the unit test.

I recommend leaving the OAuth consumerSecret parameter as a regular string vs the nullable type you changed it to. This should reduce the impact of this change as it would be a signature change vs an internal validation change.

I hesitated before changing these signatures. My reasoning was that introducing a nullable type for the parameter consumerSecret would be ok, since C# allowed an implicit cast from string? to string and this conveys the intent that the consumer secret may be empty. In the case I restore the initial type of the parameter (string), would you still allow passing null as an argument to consumerSecret, or only allow an empty string to be passed? Should we wait the feedback of a RestSharp's dev before changing it?

Cheers!

@alexeyzimarev alexeyzimarev merged commit ddcb137 into restsharp:dev Jul 15, 2021
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants