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

Rework CookieAuth for compat with CookiePolicy. #843

Merged
merged 1 commit into from
May 26, 2016
Merged

Conversation

Tratcher
Copy link
Member

#814 @anurse @HaoK @muratg

Change ChunkingCookieManager from appending raw headers to appending via the response cookies API. Here are a few side effects:

  • Removed the key & value encoding. The lower level will handle this and most of the time no encoding is required. Worst case we loose a little bit of length precision and devs need to lower ChunkSize accordingly.
  • Removed the quote handling. Cookie auth never uses quotes.
  • Change the format of the first cookie from key=chunks:2 to key=chunks-2. : would be escaped by the response cookies API.

@Tratcher Tratcher added this to the 1.0.0 milestone May 24, 2016
@Tratcher Tratcher self-assigned this May 24, 2016
@dnfclas
Copy link

dnfclas commented May 24, 2016

Hi @Tratcher, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@HaoK
Copy link
Member

HaoK commented May 24, 2016

:shipit:

1 similar comment
@analogrelay
Copy link

:shipit:

@kevinchalet
Copy link
Contributor

I wonder why the cookie manager is not part of a more general repository like HttpAbstractions. It's not really specific to authentication cookies, actually.

@Tratcher
Copy link
Member Author

That's what we thought in Katana, but then nobody else ever used it.

@Tratcher
Copy link
Member Author

Hmm, this format change will break interop with Katana. However the ICookieManager could be replaced there to read the modified format. @HaoK is this a problem for the interop work you did?

@HaoK
Copy link
Member

HaoK commented May 25, 2016

Did you change the TicketSerializer?

@Tratcher
Copy link
Member Author

No, I changed the format of the first cookie from key=chunks:2 to key=chunks-2 because the : would be escaped by the response cookies API.

@HaoK
Copy link
Member

HaoK commented May 25, 2016 via email

@Tratcher
Copy link
Member Author

Should I add a compatible cookie manager to the interop package?

@kevinchalet
Copy link
Contributor

Should I add a compatible cookie manager to the interop package?

If you want to bring back a dependency on the OWIN cookies package, that would be great to do that in a separate package (something like Microsoft.Owin.Security.Cookies.Interop).

@HaoK
Copy link
Member

HaoK commented May 25, 2016

Is cookie chunking on by default?

On Wed, May 25, 2016 at 11:03 AM, Chris R [email protected] wrote:

Should I add a compatible cookie manager to the interop package?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#843 (comment)

@Tratcher
Copy link
Member Author

Yes, chunking is on by default, but it only takes effect if the cookie is over 4kb. This happens pretty often for OIDC.

@HaoK
Copy link
Member

HaoK commented May 25, 2016

A cookie interop package is good since it would be a better home for the extensions methods needed for cookie data protection sharing than the Identity compat package.

@Tratcher
Copy link
Member Author

Updated. I was able to add a compatible ChunkingCookieManager to the interop package without adding any dependencies because the old interface was in the Microsoft.Owin package.

@kevinchalet
Copy link
Contributor

I was able to add a compatible ChunkingCookieManager to the interop package without adding any dependencies because the old interface was in the Microsoft.Owin package.

Nice 👍

@HaoK
Copy link
Member

HaoK commented May 26, 2016

Nice :shipit:

@Tratcher Tratcher force-pushed the tratcher/cookies branch from 4287e4a to 2634fe3 Compare May 26, 2016 21:21
@Tratcher Tratcher merged commit 2634fe3 into dev May 26, 2016
@Tratcher Tratcher deleted the tratcher/cookies branch May 26, 2016 21:21
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.

5 participants