Skip to content
This repository was archived by the owner on Nov 22, 2018. It is now read-only.

Add CookieBuilder property to SessionOptions and obsolete duplicated properties #173

Merged
merged 1 commit into from
Jun 30, 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
15 changes: 14 additions & 1 deletion Session.sln
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio 15
VisualStudioVersion = 15.0.26228.9
VisualStudioVersion = 15.0.26621.2
MinimumVisualStudioVersion = 10.0.40219.1
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "test", "test", "{E9D63F97-6078-42AD-BFD3-F956BF921BB5}"
EndProject
Expand All @@ -15,6 +15,16 @@ EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SessionSample", "samples\SessionSample\SessionSample.csproj", "{FE0B9969-3BDE-4A7D-BE1B-47EAE8DBF365}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "SolutionItems", "SolutionItems", "{3B45F658-5BF1-4E07-BE9C-6F5110AC2277}"
ProjectSection(SolutionItems) = preProject
.gitattributes = .gitattributes
.gitignore = .gitignore
.travis.yml = .travis.yml
appveyor.yml = appveyor.yml
NuGet.config = NuGet.config
NuGetPackageVerifier.json = NuGetPackageVerifier.json
README.md = README.md
version.props = version.props
EndProjectSection
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "build", "build", "{4F21221F-2813-41B7-AAFC-E03FD52971CC}"
ProjectSection(SolutionItems) = preProject
Expand Down Expand Up @@ -50,4 +60,7 @@ Global
{FE0B9969-3BDE-4A7D-BE1B-47EAE8DBF365} = {94E80ED2-9F27-40AC-A9EF-C707BDFAA3BE}
{4F21221F-2813-41B7-AAFC-E03FD52971CC} = {3B45F658-5BF1-4E07-BE9C-6F5110AC2277}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {6AE224B9-B604-4E47-9617-9D114DAE9BE5}
EndGlobalSection
EndGlobal
20 changes: 3 additions & 17 deletions src/Microsoft.AspNetCore.Session/SessionMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public async Task Invoke(HttpContext context)
{
var isNewSessionKey = false;
Func<bool> tryEstablishSession = ReturnTrue;
var cookieValue = context.Request.Cookies[_options.CookieName];
var cookieValue = context.Request.Cookies[_options.Cookie.Name];
var sessionKey = CookieProtection.Unprotect(_dataProtector, cookieValue, _logger);
if (string.IsNullOrWhiteSpace(sessionKey) || sessionKey.Length != SessionKeyLength)
{
Expand Down Expand Up @@ -150,23 +150,9 @@ private static Task OnStartingCallback(object state)

private void SetCookie()
{
var cookieOptions = new CookieOptions
{
Domain = _options.CookieDomain,
SameSite = _options.SameSiteMode,
HttpOnly = _options.CookieHttpOnly,
Path = _options.CookiePath ?? SessionDefaults.CookiePath,
};
if (_options.CookieSecure == CookieSecurePolicy.SameAsRequest)
{
cookieOptions.Secure = _context.Request.IsHttps;
}
else
{
cookieOptions.Secure = _options.CookieSecure == CookieSecurePolicy.Always;
}
var cookieOptions = _options.Cookie.Build(_context);

_context.Response.Cookies.Append(_options.CookieName, _cookieValue, cookieOptions);
_context.Response.Cookies.Append(_options.Cookie.Name, _cookieValue, cookieOptions);

_context.Response.Headers["Cache-Control"] = "no-cache";
_context.Response.Headers["Pragma"] = "no-cache";
Expand Down
100 changes: 86 additions & 14 deletions src/Microsoft.AspNetCore.Session/SessionOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,45 +12,117 @@ namespace Microsoft.AspNetCore.Builder
/// </summary>
public class SessionOptions
{
private CookieBuilder _cookieBuilder = new SessionCookieBuilder();

/// <summary>
/// Determines the settings used to create the cookie.
/// <para>
/// <see cref="CookieBuilder.Name"/> defaults to <see cref="SessionDefaults.CookieName"/>.
/// <see cref="CookieBuilder.Path"/> defaults to <see cref="SessionDefaults.CookiePath"/>.
/// <see cref="CookieBuilder.SameSite"/> defaults to <see cref="SameSiteMode.Lax"/>.
/// <see cref="CookieBuilder.HttpOnly"/> defaults to <c>true</c>
/// </para>
/// </summary>
public CookieBuilder Cookie
{
get => _cookieBuilder;
set => _cookieBuilder = value ?? throw new ArgumentNullException(nameof(value));
}

/// <summary>
/// The IdleTimeout indicates how long the session can be idle before its contents are abandoned. Each session access
/// resets the timeout. Note this only applies to the content of the session, not the cookie.
/// </summary>
public TimeSpan IdleTimeout { get; set; } = TimeSpan.FromMinutes(20);

#region Obsolete API
/// <summary>
/// <para>
/// This property is obsolete and will be removed in a future version. The recommended alternative is <seealso cref="CookieBuilder.Name"/> on <see cref="Cookie"/>.
/// </para>
/// <para>
/// Determines the cookie name used to persist the session ID.
/// Defaults to <see cref="SessionDefaults.CookieName"/>.
/// </para>
/// </summary>
public string CookieName { get; set; } = SessionDefaults.CookieName;
[Obsolete("This property is obsolete and will be removed in a future version. The recommended alternative is " + nameof(Cookie) + "." + nameof(CookieBuilder.Name) + ".")]
public string CookieName { get => Cookie.Name; set => Cookie.Name = value; }

/// <summary>
/// <para>
/// This property is obsolete and will be removed in a future version. The recommended alternative is <seealso cref="CookieBuilder.Domain"/> on <see cref="Cookie"/>.
/// </para>
/// <para>
/// Determines the domain used to create the cookie. Is not provided by default.
/// </para>
/// </summary>
public string CookieDomain { get; set; }
[Obsolete("This property is obsolete and will be removed in a future version. The recommended alternative is " + nameof(Cookie) + "." + nameof(CookieBuilder.Domain) + ".")]
public string CookieDomain { get => Cookie.Domain; set => Cookie.Domain = value; }

/// <summary>
/// <para>
/// This property is obsolete and will be removed in a future version. The recommended alternative is <seealso cref="CookieBuilder.Path"/> on <see cref="Cookie"/>.
/// </para>
/// <para>
/// Determines the path used to create the cookie.
/// Defaults to <see cref="SessionDefaults.CookiePath"/>.
/// </para>
/// </summary>
public string CookiePath { get; set; } = SessionDefaults.CookiePath;
[Obsolete("This property is obsolete and will be removed in a future version. The recommended alternative is " + nameof(Cookie) + "." + nameof(CookieBuilder.Path) + ".")]
public string CookiePath { get => Cookie.Path; set => Cookie.Path = value; }

/// <summary>
/// <para>
/// This property is obsolete and will be removed in a future version. The recommended alternative is <seealso cref="CookieBuilder.HttpOnly"/> on <see cref="Cookie"/>.
/// </para>
/// <para>
/// Determines if the browser should allow the cookie to be accessed by client-side JavaScript. The
/// default is true, which means the cookie will only be passed to HTTP requests and is not made available
/// to script on the page.
/// </para>
/// </summary>
public bool CookieHttpOnly { get; set; } = true;
[Obsolete("This property is obsolete and will be removed in a future version. The recommended alternative is " + nameof(Cookie) + "." + nameof(CookieBuilder.HttpOnly) + ".")]
public bool CookieHttpOnly { get => Cookie.HttpOnly; set => Cookie.HttpOnly = value; }

/// <summary>
/// <para>
/// This property is obsolete and will be removed in a future version. The recommended alternative is <seealso cref="CookieBuilder.SameSite"/> on <see cref="Cookie"/>.
/// </para>
/// <para>
/// Determines if the browser should allow the cookie to be attached to same-site or cross-site requests. The
/// default is Lax, which means the cookie is allowed to be attached to same-site and safe cross-site requests.
/// </para>
/// </summary>
public SameSiteMode SameSiteMode { get; set; } = SameSiteMode.Lax;
[Obsolete("This property is obsolete and will be removed in a future version. The recommended alternative is " + nameof(Cookie) + "." + nameof(CookieBuilder.SameSite) + ".")]
public SameSiteMode SameSiteMode { get => Cookie.SameSite; set => Cookie.SameSite = value; }

/// <summary>
/// Determines if the cookie should only be transmitted on HTTPS requests.
/// <para>
/// This property is obsolete and will be removed in a future version. The recommended alternative is <seealso cref="CookieBuilder.SecurePolicy"/> on <see cref="Cookie"/>.
/// </para>
/// <para>
/// Determines if the cookie should only be transmitted on HTTPS requests.
/// </para>
/// </summary>
public CookieSecurePolicy CookieSecure { get; set; } = CookieSecurePolicy.None;
[Obsolete("This property is obsolete and will be removed in a future version. The recommended alternative is " + nameof(Cookie) + "." + nameof(CookieBuilder.SecurePolicy) + ".")]
public CookieSecurePolicy CookieSecure { get => Cookie.SecurePolicy; set => Cookie.SecurePolicy = value; }
#endregion

/// <summary>
/// The IdleTimeout indicates how long the session can be idle before its contents are abandoned. Each session access
/// resets the timeout. Note this only applies to the content of the session, not the cookie.
/// </summary>
public TimeSpan IdleTimeout { get; set; } = TimeSpan.FromMinutes(20);
private class SessionCookieBuilder : CookieBuilder
{
public SessionCookieBuilder()
{
Name = SessionDefaults.CookieName;
Path = SessionDefaults.CookiePath;
SecurePolicy = CookieSecurePolicy.None;
SameSite = SameSiteMode.Lax;
HttpOnly = true;
}

public override TimeSpan? Expiration
{
get => null;
set => throw new InvalidOperationException(nameof(Expiration) + " cannot be set for the cookie defined by " + nameof(SessionOptions));
}
}
}
}
}
7 changes: 5 additions & 2 deletions test/Microsoft.AspNetCore.Session.Tests/SessionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,11 @@ public async Task SecureSessionBasedOnHttpsAndSecurePolicy(
{
app.UseSession(new SessionOptions
Copy link
Member

@HaoK HaoK Jun 30, 2017

Choose a reason for hiding this comment

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

Unrelated to this PR, but this is pretty gross, given that Session is using IOptions<SessionOptions> we should really kill this pattern.

If we want to avoid making them go Configure<SessionOptions>(o => ), we can change UseSession(o => o.Cookie.Name = "TestCookie"), this new SessionOptions/Wrapper pattern stuff needs to burn....

Copy link
Member

Choose a reason for hiding this comment

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

@Tratcher @davidfowl @ajcvickers @divega thoughts? Can we get rid of all instances where we UseXyz(new XyzOptions()) and consistently always use UseXyz(o => )

Copy link
Member

Choose a reason for hiding this comment

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

Assuming UseXyz is using IOptions...

Copy link
Member

Choose a reason for hiding this comment

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

Worth considering, but not as part of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I just meant a cleanup pass for all middleware doing this, not something for this PR, I'll file an issue

Copy link
Member

Choose a reason for hiding this comment

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

{
CookieName = "TestCookie",
CookieSecure = cookieSecurePolicy
Cookie =
{
Name = "TestCookie",
SecurePolicy = cookieSecurePolicy
}
});
app.Run(context =>
{
Expand Down