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

GetLoginRequestUri Method with optional NameValueCollection parameter and default access_type=offline in GoogleClient to have refresh_token #121

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Sbaia
Copy link

@Sbaia Sbaia commented Jun 20, 2018

GetLoginRequestUri Method with optional NameValueCollection parameter
for add query string parameters in uri.

Added default queryString parameter access_type=offline in GoogleClient,
to take refresh_token from googleauth api, that otherwise is not
returned.

Added OAuth.pfx signature with OAuth2 password

for add query string parameters in uri.
Added default queryString parameter access_type=offline in GoogleClient,
to take refresh_token from googleauth api, that otherwise is not
returned.
Added OAu
Copy link
Collaborator

@niemyjski niemyjski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a pfx? Can you please remove it

OAuth2/OAuth2.csproj Show resolved Hide resolved
@@ -74,6 +79,14 @@ public override string Name
}


public override Task<string> GetLoginLinkUriAsync(string state = null, CancellationToken cancellationToken = default, NameValueCollection parametersCollection = null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this couldn't be a IDictionary<string,string>?

if (parametersCollection == null)
parametersCollection = new NameValueCollection();
if (!parametersCollection.AllKeys.Contains("access_type"))
parametersCollection.Add("access_type", "offline");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any documentation this and why it should be the default value?

@niemyjski
Copy link
Collaborator

Hello,

Can you please make the requested changes.

}

private string Nonce { get; }
// private string CodeVerifier { get; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the dead code and todo's from this pr

Comment on lines +7 to +9
<SignAssembly>true</SignAssembly>
<AssemblyOriginatorKeyFile>OAuth2.pfx</AssemblyOriginatorKeyFile>
<DelaySign>false</DelaySign>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<SignAssembly>true</SignAssembly>
<AssemblyOriginatorKeyFile>OAuth2.pfx</AssemblyOriginatorKeyFile>
<DelaySign>false</DelaySign>

</PropertyGroup>
<ItemGroup>
<PackageReference Include="Newtonsoft.Json" Version="10.0.3" />
<PackageReference Include="RestSharp" Version="106.3.0" />
<PackageReference Include="RestSharp" Version="106.2.1" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for downgrading? If anything we should be moving to the latest, can we revert this change

@@ -16,3 +16,4 @@ log.txt
*.userprefs

artifacts/
OAuth2.Console.Tests/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OAuth2.Console.Tests/

@@ -1,7 +1,7 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio 15
VisualStudioVersion = 15.0.26730.16
VisualStudioVersion = 15.0.27130.2036
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we revert all changes to this file as there wasn't an added test project

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.

2 participants