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

Replace INonceCache by IDistributedCache #212

Closed
kevinchalet opened this issue Apr 8, 2015 · 9 comments
Closed

Replace INonceCache by IDistributedCache #212

kevinchalet opened this issue Apr 8, 2015 · 9 comments

Comments

@kevinchalet
Copy link
Contributor

#202 (comment)

@Eilon
Copy link
Contributor

Eilon commented Jun 11, 2015

@PinpointTownes do you want to take a stab at this?

@Eilon Eilon added this to the 1.0.0-beta6 milestone Jun 11, 2015
@kevinchalet
Copy link
Contributor Author

@Eilon yup, feel free to assign me this issue 😄

@Eilon
Copy link
Contributor

Eilon commented Jun 11, 2015

Can't actually assign to you because you have to be in our GitHub org, but we know it's you 😄

@kevinchalet
Copy link
Contributor Author

Can't actually assign to you because you have to be in our GitHub org, but we know it's you 😄

Actually, that's a strong sign I should be in this organization 😄

@kevinchalet
Copy link
Contributor Author

I got two questions concerning the general design we should adopt for this PR:

  • Should the nonce cache be used by default? Currently, there's absolutely no INonceCache implementation offered by the OIDC package, which means that it's turned off by default. Should we change that?
  • Where should the distributed cache be retrieved from?
    • Directly from the DI container: in this case, it will necessarily use the distributed cache defined at the app-level.
    • From the OIDC options: with this approach, it won't be retrieved from the DI container and will be isolated from the rest of the application.
    • From the OIDC options or from the DI container: that's the hybrid scenario, where the distributed cache is first retrieved from the options when available, and from the DI container when the user didn't provide its own cache.

FWIW, I opted for the second approach in AspNet.Security.OpenIdConnect.Server, but I'll probably replace it by the hybrid solution: https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Server/blob/vNext/src/AspNet.Security.OpenIdConnect.Server/OpenIdConnectServerOptions.cs#L195

/cc @Eilon @HaoK @Tratcher

@Tratcher
Copy link
Member

This needs to be under developer control. I don't think the presence or absence of an IDistributedCache in DI is sufficient to turn this on or off since the cache may be used for other components as well. Maybe just a bool that toggles nonce cache vs nonce cookies? I would still default to cookies, as cache requires dependencies and setup.

Check how we use IDistributedCache in Session. It may not be quite the same since technically it's optional here. The hybrid approach sounds the most accessible.

@kevinchalet
Copy link
Contributor Author

Maybe just a bool that toggles nonce cache vs nonce cookies?

Yup, that's what I had in mind.

I would still default to cookies, as cache requires dependencies and setup.

True. But on the other hand, storing nonces in a server-side cache is definitely safer, as "nonce cookies" cannot really protect your app against replay attacks.

Am I allowed to submit a prototype using sessions instead of directly using a distributed cache?

@Tratcher
Copy link
Member

Stay away from session. Using the memory cache implementation of IDistributedCache (LocalCache) should be adequate for development.

@muratg
Copy link
Contributor

muratg commented Jun 29, 2015

Thanks @PinpointTownes for the PR! @HaoK, @Tratcher can one of you guys merge in Beta 6?

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

No branches or pull requests

4 participants