Add cache for retrieved RBAC claims - #25698
Conversation
ghost
added
the
| /// </summary> | ||
| public int ClaimsCacheSize { get; set; } = 1024; | ||
|
|
||
| internal MemoryCache ClaimsCache { get; set; } |
There was a problem hiding this comment.
This is different from the certificate validation cache in that it's not stored in DI. It might be a bit odd to have the cache in LdapSettings but I'm not sure if there's any value in putting this cache in DI.
There was a problem hiding this comment.
Main value of putting it in DI is if you want to make it easy for them to plug in their own cache implementation
There was a problem hiding this comment.
While that's true, I'm not sure we'd want that kind of flexibility here at the moment. I don't see it as particularly useful in this scenario.
There was a problem hiding this comment.
That's fine, I was just pointing out what advantages putting it in DI has
|
You can write a test that seeds the cache and verifies those are the values returned. |
| { | ||
| ldapSettings.Domain = "domain.NET"; | ||
| ldapSettings.ClaimsCache = claimsCache; | ||
| ldapSettings.EnableLdapClaimResolution = false; // This disables binding to the LDAP connection on startup |
There was a problem hiding this comment.
As I expected, this is really hacky but it works.
Co-authored-by: Chris Ross <Tratcher@Outlook.com>
JunTaoLuo
added
the
Servicing-consider
|
Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge. |
|
cc @Pilchie for rc2 approval. |
Pilchie
added
Servicing-approved
|
Approved for .NET 5 RC2 pending CI completion. |
4 participants
fixes #25329.
Tested manually, I have a feeling unit tests are going to be tricky.