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

Temp keys user policies #3820

Merged
merged 5 commits into from
Apr 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/NuGetGallery.Core/Entities/EntitiesContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,15 @@ protected override void OnModelCreating(DbModelBuilder modelBuilder)
modelBuilder.Entity<Role>()
.HasKey(u => u.Key);

modelBuilder.Entity<UserSecurityPolicy>()
.HasRequired<User>(p => p.User)
.WithMany(cr => cr.SecurityPolicies)
.HasForeignKey(p => p.UserKey)
.WillCascadeOnDelete(true);

modelBuilder.Entity<UserSecurityPolicy>()
.HasKey(p => p.Key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is not this duplication of the line 123.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed


modelBuilder.Entity<EmailMessage>()
.HasKey(em => em.Key);

Expand Down
5 changes: 5 additions & 0 deletions src/NuGetGallery.Core/Entities/User.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public User() : this(null)
public User(string username)
{
Credentials = new List<Credential>();
SecurityPolicies = new List<UserSecurityPolicy>();
Roles = new List<Role>();
Username = username;
}
Expand Down Expand Up @@ -52,6 +53,7 @@ public bool Confirmed
public string PasswordResetToken { get; set; }

public DateTime? PasswordResetTokenExpirationDate { get; set; }

public int Key { get; set; }

public DateTime? CreatedUtc { get; set; }
Expand All @@ -67,8 +69,11 @@ public string LastSavedEmailAddress
return UnconfirmedEmailAddress ?? EmailAddress;
}
}

public virtual ICollection<Credential> Credentials { get; set; }

public virtual ICollection<UserSecurityPolicy> SecurityPolicies { get; set; }

public void ConfirmEmailAddress()
{
if (string.IsNullOrEmpty(UnconfirmedEmailAddress))
Expand Down
48 changes: 48 additions & 0 deletions src/NuGetGallery.Core/Entities/UserSecurityPolicy.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.ComponentModel.DataAnnotations;

namespace NuGetGallery
{
/// <summary>
/// User-subscribed security policy.
/// </summary>
public class UserSecurityPolicy : IEntity
{
public UserSecurityPolicy()
{
}

public UserSecurityPolicy(string name)
{
Name = name;
}

/// <summary>
/// Policy key.
/// </summary>
public int Key { get; set; }

/// <summary>
/// User key.
/// </summary>
public int UserKey { get; set; }

/// <summary>
/// User subscribed to this security policy.
/// </summary>
public User User { get; set; }

/// <summary>
/// Type name for the policy handler that provides policy behavior.
/// </summary>
[Required]
Copy link
Member

Choose a reason for hiding this comment

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

Required [](start = 9, length = 8)

Can' this [Required] attribute be defined in EntitiesContext instead? Seems like you are requiring User.

Copy link
Member Author

Choose a reason for hiding this comment

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

HasRequired in EntitiesContext is for a required relationship. I added Required here since it's the only non-nullable column that isn't a PK/FK. Not sure how this actually gets used, but we don't seem to be consistent with [Required] usage.

public string Name { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Name [](start = 22, length = 4)

I think type name is an implementation detail. We should have a hard coded constant for this (either an int which is consistent with the rest of the DB or const string).

Copy link
Member Author

Choose a reason for hiding this comment

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

Policy handler names are now constants. I left as strings which I think will be easier to read when querying the DB. This PR only reads policies, but the next PR (onboarding) will start writing them.


/// <summary>
/// Support for JSON-serialized properties for specific policies.
/// </summary>
public string Value { get; set; }
}
}
1 change: 1 addition & 0 deletions src/NuGetGallery.Core/NuGetGallery.Core.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@
<Compile Include="Entities\ReadOnlyModeException.cs" />
<Compile Include="Entities\Role.cs" />
<Compile Include="Entities\Scope.cs" />
<Compile Include="Entities\UserSecurityPolicy.cs" />
<Compile Include="Entities\User.cs" />
<Compile Include="Infrastructure\AzureEntityList.cs" />
<Compile Include="Infrastructure\TableErrorLog.cs" />
Expand Down
6 changes: 6 additions & 0 deletions src/NuGetGallery/App_Start/DefaultDependenciesModule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
using NuGetGallery.Infrastructure;
using NuGetGallery.Infrastructure.Authentication;
using NuGetGallery.Infrastructure.Lucene;
using NuGetGallery.Security;

namespace NuGetGallery
{
Expand Down Expand Up @@ -201,6 +202,11 @@ protected override void Load(ContainerBuilder builder)
.As<IStatusService>()
.InstancePerLifetimeScope();

builder.RegisterType<SecurityPolicyService>()
.AsSelf()
.As<ISecurityPolicyService>()
.InstancePerLifetimeScope();
Copy link
Contributor

@skofman1 skofman1 Apr 25, 2017

Choose a reason for hiding this comment

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

Any reason for ISecurityPolicyService not to be a single instance (SingleInstance())?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should prefer InstancePerLifetimeScope over SuntingleInstance until we have a caching/perf/behavior requirement for it. It's easier for unintended interaction between requests to occur with SingleInstance


In reply to: 113238685 [](ancestors = 113238685)


var mailSenderThunk = new Lazy<IMailSender>(
() =>
{
Expand Down
6 changes: 3 additions & 3 deletions src/NuGetGallery/Controllers/ApiController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ public async virtual Task<ActionResult> CreatePackageVerificationKeyAsync(string

[HttpGet]
[RequireSsl]
[ApiAuthorize]
[ApiAuthorize(SecurityPolicyAction.PackageVerify)]
[ApiScopeRequired(NuGetScopes.PackageVerify, NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion)]
[ActionName("VerifyPackageKey")]
public async virtual Task<ActionResult> VerifyPackageKeyAsync(string id, string version)
Expand Down Expand Up @@ -291,7 +291,7 @@ private HttpStatusCodeWithBodyResult VerifyPackageKeyInternal(User user, Credent

[HttpPut]
[RequireSsl]
[ApiAuthorize]
[ApiAuthorize(SecurityPolicyAction.PackagePush)]
[ApiScopeRequired(NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion)]
[ActionName("PushPackageApi")]
public virtual Task<ActionResult> CreatePackagePut()
Expand All @@ -301,7 +301,7 @@ public virtual Task<ActionResult> CreatePackagePut()

[HttpPost]
[RequireSsl]
[ApiAuthorize]
[ApiAuthorize(SecurityPolicyAction.PackagePush)]
[ApiScopeRequired(NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion)]
[ActionName("PushPackageApi")]
public virtual Task<ActionResult> CreatePackagePost()
Expand Down
62 changes: 59 additions & 3 deletions src/NuGetGallery/Filters/ApiAuthorizeAttribute.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,38 @@
using System.Web;
using System.Web.Mvc;
using NuGetGallery.Authentication;
using NuGetGallery.Security;
using AuthenticationTypes = NuGetGallery.Authentication.AuthenticationTypes;
using AuthorizationContext = System.Web.Mvc.AuthorizationContext;
using System.Net;

namespace NuGetGallery.Filters
{
public sealed class ApiAuthorizeAttribute : AuthorizeAttribute
{
/// <summary>
/// Security policy action, or null for no evaluation.
/// </summary>
public SecurityPolicyAction? SecurityPolicyAction { get; }

/// <summary>
/// Security policy evaluation result.
/// </summary>
private SecurityPolicyResult SecurityPolicyResult { get; set; }

/// <summary>
/// Security policy service.
/// </summary>
private ISecurityPolicyService SecurityPolicyService { get; set; }

public ApiAuthorizeAttribute()
{}

public ApiAuthorizeAttribute(SecurityPolicyAction action)
{
SecurityPolicyAction = action;
}

public override void OnAuthorization(AuthorizationContext filterContext)
{
// Add a warning header if the API key is about to expire (or has expired)
Expand Down Expand Up @@ -49,16 +74,47 @@ public override void OnAuthorization(AuthorizationContext filterContext)
}
}
}

// Resolve the policy service if security policy checks are required.
if (SecurityPolicyAction.HasValue)
{
SecurityPolicyService = ((AppController)filterContext.Controller)?.GetService<ISecurityPolicyService>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This will throw if the ApiAuthorizeAttribute is used on a controller that doesn't extend AppController, right? I would recommend that you add documentation that this attribute must be used on controllers that extend AppController. Also, maybe throw an exception if the controller isn't an AppController to make this easier to debug if this issue ever comes up?
*

}

base.OnAuthorization(filterContext);
}

protected override bool AuthorizeCore(HttpContextBase httpContext)
{
var authorizeResult = base.AuthorizeCore(httpContext);

// If ApiKey authorization succeeds, evaluate any security policies.
if (authorizeResult && SecurityPolicyAction.HasValue)
{
SecurityPolicyResult = SecurityPolicyService.Evaluate(SecurityPolicyAction.Value, httpContext);
return SecurityPolicyResult.Success;
}

return authorizeResult;
}

protected override void HandleUnauthorizedRequest(AuthorizationContext filterContext)
{
var owinContext = filterContext.HttpContext.GetOwinContext();
owinContext.Authentication.Challenge(AuthenticationTypes.ApiKey);
owinContext.Response.StatusCode = 401;
filterContext.Result = new HttpUnauthorizedResult();

// ApiKey authorization succeeded, but a security policy failed.
if (SecurityPolicyResult != null)
{
owinContext.Response.StatusCode = 400;
filterContext.Result = new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, SecurityPolicyResult.ErrorMessage);
}
// ApiKey authorization failed.
else
{
owinContext.Authentication.Challenge(AuthenticationTypes.ApiKey);
owinContext.Response.StatusCode = 401;
filterContext.Result = new HttpUnauthorizedResult();
}
}
}
}
11 changes: 11 additions & 0 deletions src/NuGetGallery/Filters/SecurityPolicyAction.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace NuGetGallery.Filters
{
public enum SecurityPolicyAction
{
PackagePush,
PackageVerify
}
}
5 changes: 5 additions & 0 deletions src/NuGetGallery/Helpers/HttpContextBaseExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ namespace NuGetGallery
{
public static class HttpContextBaseExtensions
{
public static User GetCurrentUser(this HttpContextBase httpContext)
{
return httpContext.GetOwinContext().GetCurrentUser();
}

public static void SetConfirmationReturnUrl(this HttpContextBase httpContext, string returnUrl)
{
var confirmationContext = new ConfirmationContext
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions src/NuGetGallery/Migrations/201704211454424_SecurityPolicies.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace NuGetGallery.Migrations
{
using System.Data.Entity.Migrations;

public partial class SecurityPolicies : DbMigration
{
public override void Up()
{
CreateTable("UserSecurityPolicies", c => new
{
Key = c.Int(nullable: false, identity: true),
Name = c.String(nullable: false, maxLength: 256),
UserKey = c.Int(nullable: false),
Value = c.String(nullable: true, maxLength: 256)
})
.PrimaryKey(t => t.Key)
.ForeignKey("Users", t => t.UserKey);
}

public override void Down()
{
DropTable("UserSecurityPolicies");
}
}
}
Loading