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

Add Secure cookie flag option to session cookie #28

Closed
wants to merge 3 commits into from
Closed

Add Secure cookie flag option to session cookie #28

wants to merge 3 commits into from

Conversation

SteveArr
Copy link

@SteveArr SteveArr commented May 1, 2015

The session cookie needs the ability to have the 'Secure' flag set. This PR adds support for always use secure flag, never use secure flag and use secure flag only if initial request is SSL.

Included unit tests for Secure flag as well as HTTPOnly flag.

SteveArr added 3 commits May 1, 2015 20:56
Add the ability for the developer to specify if the session cookie
should be protected with the Secure flag.
Added unit tests for cookie flags.  Moved some session defaults into
SessionDefault.cs and out of SessionOptions.cs for consistency.
Add Apache License to new .cs file.
@ghost
Copy link

ghost commented May 1, 2015

Hi @SteveArr, I'm your friendly neighborhood Microsoft Open Technologies, Inc. Pull Request Bot (You can call me MSOTBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.msopentech.com.

TTYL, MSOTBOT;

@ghost ghost added the cla-required label May 1, 2015
@ghost
Copy link

ghost commented May 1, 2015

@SteveArr, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSOTBOT;

@ghost ghost added cla-signed and removed cla-required labels May 1, 2015
@Eilon
Copy link
Member

Eilon commented May 4, 2015

@blowdart can you have a look?

@blowdart
Copy link
Member

blowdart commented May 4, 2015

My usual naming issues appear

Https != Secure (We should look at CookieOptions as well to fix this)

There's no other problem aside from that.

@@ -9,5 +9,7 @@ public static class SessionDefaults
{
public static string CookieName = ".AspNet.Session";
Copy link
Member

Choose a reason for hiding this comment

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

@Tratcher are all these suppose to be readonly?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

@blowdart
Copy link
Member

blowdart commented May 4, 2015

There is a side effect here, in that if you switch protocols you will loose session if you're switching from HTTPS to HTTP. I don't consider this a bad thing, other opinions may vary.

@@ -0,0 +1,39 @@
// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

BTW we just changed the copyright notice to:

Copyright (c) .NET Foundation. All rights reserved.

@blowdart
Copy link
Member

blowdart commented May 4, 2015

This would probably be better addressed as part of abstractions, where there is already an open issue on the topic. aspnet/HttpAbstractions#42

@SteveArr
Copy link
Author

SteveArr commented May 4, 2015

After reading your comments, here is the change I propose:

Take CookieSecureOption.cs and move it up into Aspnet/HttpAbstractions like @blowdart is recommending. Update code comments appropriately.

Update this pull request to use that relocated CookieSecureOption.cs

Update Aspnet/Security/Microsoft.AspNet.Authentication.Cookies to also use the newly located CookieSecureOption.cs to reduce code duplication.

Question: I am unfamiliar with the process for updating code in three different projects. Do I have to submit a PR to HTTPAbstractions first and let that get in before making the changes to Session and Security?

@Tratcher
Copy link
Member

Tratcher commented May 4, 2015

You can submit concurrent PRs in multiple repos, just and links to each related PR in the comments.

Do you know how to use the global.json file to compile multiple repos locally?

@SteveArr
Copy link
Author

SteveArr commented May 4, 2015

@Tratcher , I am unaware of how to use the global.json file to compile multiple repos locally. Can you forward me some information or pointers?

@Tratcher
Copy link
Member

Tratcher commented May 4, 2015

In Session modify the global.json to add a relative reference to HttpAbstractions. This way your local changes will be used rather than the nuget packages.

{
    "projects": [ "src", "..\\HttpAbstractions\\src" ]
}

Do not check in this change, it is only for local development.

@SteveArr
Copy link
Author

SteveArr commented May 4, 2015

Got it. I will work on this tonight when I get home.

@dnfclas
Copy link

dnfclas commented May 5, 2015

Hi @SteveArr, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@muratg
Copy link

muratg commented Sep 4, 2015

@Tratcher Could you look at this one (and the corresponding HttpAbstractions one)?

@Tratcher
Copy link
Member

Tratcher commented Sep 4, 2015

@SteveArr Can you update your CLA as described above?

@Tratcher Tratcher self-assigned this Sep 4, 2015
@muratg
Copy link

muratg commented Sep 9, 2015

@SteveArr Are you still looking into this one?

@muratg
Copy link

muratg commented Sep 9, 2015

Closing this one, we'll do aspnet/HttpAbstractions#42 instead.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants