Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit 7f5a1cc

Browse files
committed
Check scopes on login.
When performing a login, check that the scopes returned are what we expect, otherwise throw an exception causing a new login to be required.
1 parent 7f295a4 commit 7f5a1cc

File tree

7 files changed

+155
-45
lines changed

7 files changed

+155
-45
lines changed

src/GitHub.Api/ApiClientConfiguration.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Collections.Generic;
23
using System.Globalization;
34
using System.Linq;
45
using System.Net;
@@ -33,6 +34,11 @@ static ApiClientConfiguration()
3334
/// </summary>
3435
public static string ClientSecret { get; private set; }
3536

37+
/// <summary>
38+
/// Gets the scopes required by the application.
39+
/// </summary>
40+
public static IReadOnlyList<string> RequiredScopes { get; } = new[] { "user", "repo", "gist", "write:public_key" };
41+
3642
/// <summary>
3743
/// Gets a note that will be stored with the OAUTH token.
3844
/// </summary>

src/GitHub.Api/GitHub.Api.csproj

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,10 @@
4646
</PropertyGroup>
4747
<Import Project="$(SolutionDir)\src\common\signing.props" />
4848
<ItemGroup>
49+
<Reference Include="Serilog, Version=2.0.0.0, Culture=neutral, PublicKeyToken=24c2f752a8e58a10, processorArchitecture=MSIL">
50+
<HintPath>..\..\packages\Serilog.2.5.0\lib\net46\Serilog.dll</HintPath>
51+
<Private>True</Private>
52+
</Reference>
4953
<Reference Include="System" />
5054
<Reference Include="System.ComponentModel.Composition" />
5155
<Reference Include="System.Core" />
@@ -64,6 +68,7 @@
6468
<Compile Include="ApiClientConfiguration_User.cs" Condition="$(Buildtype) != 'Internal'" />
6569
<Compile Include="IKeychain.cs" />
6670
<Compile Include="ILoginManager.cs" />
71+
<Compile Include="IncorrectScopesException.cs" />
6772
<Compile Include="ITwoFactorChallengeHandler.cs" />
6873
<Compile Include="LoginManager.cs" />
6974
<Compile Include="KeychainCredentialStore.cs" />
@@ -99,6 +104,9 @@
99104
<Name>GitHub.Logging</Name>
100105
</ProjectReference>
101106
</ItemGroup>
107+
<ItemGroup>
108+
<None Include="packages.config" />
109+
</ItemGroup>
102110
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
103111
<!-- To modify your build process, add your task inside one of the targets below and uncomment it.
104112
Other similar extension points exist, see Microsoft.Common.targets.
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
using System;
2+
3+
namespace GitHub.Api
4+
{
5+
/// <summary>
6+
/// Thrown when the login for a user does not have the required scopes.
7+
/// </summary>
8+
public class IncorrectScopesException : Exception
9+
{
10+
public IncorrectScopesException()
11+
: this("You need to sign out and back in.")
12+
{
13+
}
14+
15+
public IncorrectScopesException(string message)
16+
: base(message)
17+
{
18+
}
19+
}
20+
}

src/GitHub.Api/LoginManager.cs

Lines changed: 62 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
11
using System;
2+
using System.Collections.Generic;
3+
using System.Linq;
24
using System.Net;
35
using System.Threading.Tasks;
46
using GitHub.Extensions;
7+
using GitHub.Logging;
58
using GitHub.Primitives;
69
using Octokit;
10+
using Serilog;
711

812
namespace GitHub.Api
913
{
@@ -12,11 +16,14 @@ namespace GitHub.Api
1216
/// </summary>
1317
public class LoginManager : ILoginManager
1418
{
15-
readonly string[] scopes = { "user", "repo", "gist", "write:public_key" };
19+
const string ScopesHeader = "X-OAuth-Scopes";
20+
static readonly ILogger log = LogManager.ForContext<LoginManager>();
21+
static readonly Uri UserEndpoint = new Uri("user", UriKind.Relative);
1622
readonly IKeychain keychain;
1723
readonly ITwoFactorChallengeHandler twoFactorChallengeHandler;
1824
readonly string clientId;
1925
readonly string clientSecret;
26+
readonly IReadOnlyList<string> scopes;
2027
readonly string authorizationNote;
2128
readonly string fingerprint;
2229

@@ -34,6 +41,7 @@ public LoginManager(
3441
ITwoFactorChallengeHandler twoFactorChallengeHandler,
3542
string clientId,
3643
string clientSecret,
44+
IReadOnlyList<string> scopes,
3745
string authorizationNote = null,
3846
string fingerprint = null)
3947
{
@@ -46,6 +54,7 @@ public LoginManager(
4654
this.twoFactorChallengeHandler = twoFactorChallengeHandler;
4755
this.clientId = clientId;
4856
this.clientSecret = clientSecret;
57+
this.scopes = scopes;
4958
this.authorizationNote = authorizationNote;
5059
this.fingerprint = fingerprint;
5160
}
@@ -106,24 +115,7 @@ public async Task<User> Login(
106115
} while (auth == null);
107116

108117
await keychain.Save(userName, auth.Token, hostAddress).ConfigureAwait(false);
109-
110-
var retry = 0;
111-
112-
while (true)
113-
{
114-
try
115-
{
116-
return await client.User.Current().ConfigureAwait(false);
117-
}
118-
catch (AuthorizationException)
119-
{
120-
if (retry++ == 3) throw;
121-
}
122-
123-
// It seems that attempting to use a token immediately sometimes fails, retry a few
124-
// times with a delay of of 1s to allow the token to propagate.
125-
await Task.Delay(1000);
126-
}
118+
return await ReadUserWithRetry(client);
127119
}
128120

129121
/// <inheritdoc/>
@@ -132,7 +124,7 @@ public Task<User> LoginFromCache(HostAddress hostAddress, IGitHubClient client)
132124
Guard.ArgumentNotNull(hostAddress, nameof(hostAddress));
133125
Guard.ArgumentNotNull(client, nameof(client));
134126

135-
return client.User.Current();
127+
return ReadUserWithRetry(client);
136128
}
137129

138130
/// <inheritdoc/>
@@ -258,5 +250,55 @@ bool EnterpriseWorkaround(HostAddress hostAddress, Exception e)
258250
e is ForbiddenException ||
259251
apiException?.StatusCode == (HttpStatusCode)422);
260252
}
253+
254+
async Task<User> ReadUserWithRetry(IGitHubClient client)
255+
{
256+
var retry = 0;
257+
258+
while (true)
259+
{
260+
try
261+
{
262+
return await GetUserAndCheckScopes(client).ConfigureAwait(false);
263+
}
264+
catch (AuthorizationException)
265+
{
266+
if (retry++ == 3) throw;
267+
}
268+
269+
// It seems that attempting to use a token immediately sometimes fails, retry a few
270+
// times with a delay of of 1s to allow the token to propagate.
271+
await Task.Delay(1000);
272+
}
273+
}
274+
275+
async Task<User> GetUserAndCheckScopes(IGitHubClient client)
276+
{
277+
var response = await client.Connection.Get<User>(
278+
UserEndpoint, null, null).ConfigureAwait(false);
279+
280+
if (response.HttpResponse.Headers.ContainsKey(ScopesHeader))
281+
{
282+
var returnedScopes = response.HttpResponse.Headers[ScopesHeader]
283+
.Split(',')
284+
.Select(x => x.Trim())
285+
.ToArray();
286+
287+
if (scopes.Except(returnedScopes).Count() == 0)
288+
{
289+
return response.Body;
290+
}
291+
else
292+
{
293+
log.Error("Incorrect API scopes: require {RequiredScopes} but got {Scopes}", scopes, returnedScopes);
294+
}
295+
}
296+
else
297+
{
298+
log.Error("Error reading scopes: /user succeeded but scopes header was not present");
299+
}
300+
301+
throw new IncorrectScopesException();
302+
}
261303
}
262304
}

src/GitHub.Api/packages.config

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<packages>
3+
<package id="Serilog" version="2.5.0" targetFramework="net461" />
4+
</packages>

src/GitHub.VisualStudio/GitHubPackage.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,7 @@ async Task<object> CreateService(IAsyncServiceContainer container, CancellationT
220220
twoFaHandler,
221221
ApiClientConfiguration.ClientId,
222222
ApiClientConfiguration.ClientSecret,
223+
ApiClientConfiguration.RequiredScopes,
223224
ApiClientConfiguration.AuthorizationNote,
224225
ApiClientConfiguration.MachineFingerprint);
225226
}

0 commit comments

Comments
 (0)