Skip to content

Commit d54d492

Browse files
Narrow PackageIdScopeMismatch message to only subject-specific failures
Agent-Logs-Url: https://github.com/NuGet/NuGetGallery/sessions/ea627cbc-c05e-4934-a5db-f026d6e804ee Co-authored-by: joelverhagen <[email protected]>
1 parent ed3e9e9 commit d54d492

5 files changed

Lines changed: 33 additions & 5 deletions

File tree

src/NuGetGallery.Services/Authentication/ApiScopeEvaluationResult.cs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,18 @@ public class ApiScopeEvaluationResult
3030
public bool IsOwnerConfirmed => Owner != null && Owner.Confirmed;
3131
public bool IsOwnerLocked => Owner != null && Owner.IsLocked;
3232

33-
public ApiScopeEvaluationResult(User owner, PermissionsCheckResult permissionsCheckResult, bool scopesAreValid)
33+
/// <summary>
34+
/// True when <see cref="ScopesAreValid"/> is false and at least one scope allows the requested action,
35+
/// meaning the mismatch is specifically because no scope's subject pattern matches the package ID.
36+
/// </summary>
37+
public bool IsPackageIdScopeMismatch { get; }
38+
39+
public ApiScopeEvaluationResult(User owner, PermissionsCheckResult permissionsCheckResult, bool scopesAreValid, bool isPackageIdScopeMismatch = false)
3440
{
3541
ScopesAreValid = scopesAreValid;
3642
PermissionsCheckResult = permissionsCheckResult;
3743
Owner = owner;
44+
IsPackageIdScopeMismatch = isPackageIdScopeMismatch;
3845
}
3946

4047
/// <summary>

src/NuGetGallery.Services/Authentication/ApiScopeEvaluator.cs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,11 @@ internal ApiScopeEvaluationResult Evaluate<TEntity>(
7272

7373
if (matchingScope == null)
7474
{
75-
return new ApiScopeEvaluationResult(ownerInScope, PermissionsCheckResult.Unknown, scopesAreValid: false);
75+
// Determine whether the subject (package ID) is specifically what caused the mismatch,
76+
// by checking whether any scope allows the requested action regardless of subject.
77+
var isPackageIdScopeMismatch = requestedActions != null
78+
&& scopes.Any(scope => scope.AllowsActions(requestedActions));
79+
return new ApiScopeEvaluationResult(ownerInScope, PermissionsCheckResult.Unknown, scopesAreValid: false, isPackageIdScopeMismatch: isPackageIdScopeMismatch);
7680
}
7781

7882
var isActionAllowed = action.CheckPermissions(currentUser, ownerInScope, entity);

src/NuGetGallery/Controllers/ApiController.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1227,7 +1227,7 @@ private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluationHe
12271227
}
12281228

12291229
string message;
1230-
if (!result.ScopesAreValid)
1230+
if (result.IsPackageIdScopeMismatch)
12311231
{
12321232
message = Strings.ApiKeyNotAuthorized_PackageIdScopeMismatch;
12331233
}

tests/NuGetGallery.Facts/Authentication/ApiScopeEvaluatorFacts.cs

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,10 @@ private void AssertResult(ApiScopeEvaluationResult result, User owner, Permissio
4141
Assert.True(owner.MatchesUser(result.Owner));
4242
}
4343

44-
private void AssertScopesNotValidResult(ApiScopeEvaluationResult result)
44+
private void AssertScopesNotValidResult(ApiScopeEvaluationResult result, bool isPackageIdScopeMismatch = false)
4545
{
4646
AssertResult(result, owner: null, permissionsCheckResult: PermissionsCheckResult.Unknown, scopesAreValid: false);
47+
Assert.Equal(isPackageIdScopeMismatch, result.IsPackageIdScopeMismatch);
4748
}
4849

4950
[Fact]
@@ -77,6 +78,21 @@ public void ReturnsForbiddenWhenActionIsNotAllowedByScope()
7778
AssertScopesNotValidResult(result);
7879
}
7980

81+
[Fact]
82+
public void SetsIsPackageIdScopeMismatchWhenSubjectDoesNotMatchButActionDoes()
83+
{
84+
// Arrange
85+
var scope = new Scope("notallowed", NuGetScopes.PackagePush);
86+
87+
var apiScopeEvaluator = Setup();
88+
89+
// Act
90+
var result = apiScopeEvaluator.Evaluate(null, new[] { scope }, null, null, DefaultGetSubjectFromEntity, NuGetScopes.PackagePush);
91+
92+
// Assert
93+
AssertScopesNotValidResult(result, isPackageIdScopeMismatch: true);
94+
}
95+
8096
public static IEnumerable<object[]> EvaluatesNoScopesAsAllInclusive_Data
8197
{
8298
get

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,8 @@ public static IEnumerable<object[]> InvalidScopes_Data
205205
{
206206
get
207207
{
208-
yield return MemberDataHelper.AsData(new ApiScopeEvaluationResult(null, PermissionsCheckResult.Unknown, scopesAreValid: false), HttpStatusCode.Forbidden, Strings.ApiKeyNotAuthorized_PackageIdScopeMismatch);
208+
yield return MemberDataHelper.AsData(new ApiScopeEvaluationResult(null, PermissionsCheckResult.Unknown, scopesAreValid: false, isPackageIdScopeMismatch: true), HttpStatusCode.Forbidden, Strings.ApiKeyNotAuthorized_PackageIdScopeMismatch);
209+
yield return MemberDataHelper.AsData(new ApiScopeEvaluationResult(null, PermissionsCheckResult.Unknown, scopesAreValid: false, isPackageIdScopeMismatch: false), HttpStatusCode.Forbidden, Strings.ApiKeyNotAuthorized);
209210

210211
foreach (var result in Enum.GetValues(typeof(PermissionsCheckResult)).Cast<PermissionsCheckResult>())
211212
{

0 commit comments

Comments
 (0)