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

Additional content on Cookie Policy Middleware #8382

Merged
merged 5 commits into from
Sep 18, 2018

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Sep 1, 2018

Fixes #8041

Internal Review Topic (Middleware)
Internal Review Topic (GDPR)

  • Surfaces Cookie Policy Middleware in Middleware topic.
  • Remarks on Cookie Policy Middleware ordering in Middleware and GDPR topics.
  • Cleans up a few small content issues.

@kichalla Follows your remarks on aspnet/Mvc#8233.

@guardrex guardrex mentioned this pull request Sep 4, 2018
32 tasks
@Rick-Anderson
Copy link
Contributor

@kichalla please review.

@Rick-Anderson
Copy link
Contributor

@Tratcher are you the right person to review Cookie Policy Middleware?

@guardrex
Copy link
Collaborator Author

I'm going for changes here that reflect Kiran's remarks here 👉 aspnet/Mvc#8233 (comment)

@@ -118,6 +132,10 @@ public void Configure(IApplicationBuilder app)
// Authenticate before you access secure resources.
app.UseIdentity();

// If the app uses session state, call Session Middleware after Cookie
// Policy Middleware and before MVC Middleware.
Copy link
Member

Choose a reason for hiding this comment

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

There's no cookie policy in this example.

@@ -209,6 +227,7 @@ ASP.NET Core ships with the following middleware components. The *Order* column
| Middleware | Description | Order |
| ---------- | ----------- | ----- |
| [Authentication](xref:security/authentication/identity) | Provides authentication support. | Before `HttpContext.User` is needed. Terminal for OAuth callbacks. |
| [Cookie Policy](xref:security/gdpr) | Ask for (and track) consent from users for storing personal information. | Before Session Middleware, if present. |
Copy link
Member

Choose a reason for hiding this comment

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

It does not request consent, it only tracks it. It also enforces minimum standards for cookie fields like secure and SameSite.

Copy link
Collaborator Author

@guardrex guardrex Sep 18, 2018

Choose a reason for hiding this comment

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

@Tratcher There's a consent button, and this remark on consent in the topic ...

Obtains the state of tracking for the user. If the app is configured to require consent, the user must consent before cookies can be tracked.

I shamelessly stole the line in the table almost word-for-word from the topic ... see the 2nd bullet of the introduction at 👉 https://docs.microsoft.com/aspnet/core/security/gdpr

Do I need to make changes to both topics?

Copy link
Member

Choose a reason for hiding this comment

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

Provide a link to a doc on Session Middleware.

Copy link
Member

Choose a reason for hiding this comment

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

The template UI that shows a consent button is independent of the middleware. Consent is requested and granted through the UI, and then the middleware tracks that result for future requests. The GDPR doc is fine because it's talking about the end-to-end scenario.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gotcha ... thanks.

2. Static file server
3. Authentication
4. MVC
The following `Configure` method adds middleware components for common app scenarios:
Copy link
Member

Choose a reason for hiding this comment

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

Configure --> Startup.Configure

@@ -209,6 +227,7 @@ ASP.NET Core ships with the following middleware components. The *Order* column
| Middleware | Description | Order |
| ---------- | ----------- | ----- |
| [Authentication](xref:security/authentication/identity) | Provides authentication support. | Before `HttpContext.User` is needed. Terminal for OAuth callbacks. |
| [Cookie Policy](xref:security/gdpr) | Ask for (and track) consent from users for storing personal information. | Before Session Middleware, if present. |
Copy link
Member

Choose a reason for hiding this comment

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

Provide a link to a doc on Session Middleware.

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 18, 2018

@scottaddie

Provide a link to a doc on Session Middleware.

Several of the tables last-column cells refer to other middlewares in the table. I didn't provide links in that column for any of them because all of them are already in the table. Session Middleware is the same ... it's the third from the bottom. I figured it's best not to have double links in the table.

[EDIT] ... or we have to link all of them. I'm in favor of just one link. The dev can see the list, so they should be able to find them (Famous last words ... put that on my tombstone plz! lol).

@guardrex guardrex removed the request for review from kichalla September 18, 2018 21:08
@guardrex guardrex merged commit 32cae13 into master Sep 18, 2018
@guardrex guardrex deleted the guardrex/middleware-ordering branch September 18, 2018 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants