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

Allow specifying custom TokenCache #886

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow specifying custom TokenCache #886

wants to merge 1 commit into from

Conversation

aloneguid
Copy link

This library doesn't allow specifying custom TokenCache, always defaulting to TokenCache.Default. This is OK when working with non-expiring credentials (i.e. service principal) but doesn't work when tokens need to be persisted.

@msftclas
Copy link

msftclas commented Nov 12, 2019

CLA assistant check
All CLA requirements met.

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

Copy link

@qub1n qub1n left a comment

Choose a reason for hiding this comment

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

This pull request is a very good idea.

@yaohaizh
Copy link

@aloneguid Thanks for your contribution!

@weidongxu-microsoft
Copy link
Member

I think it looks good.

The DefaultShared would cache the token in memory.

@aloneguid Would you share a bit of your use case? Do you persist the token for later use, or for parellel use across process?

@eosfor
Copy link

eosfor commented May 16, 2020

I wonder if this could help to reuse the token cache from an external context. for example, I'm making a PowerShell cmdlet using Fluent SDK. I want to use this cmdlet along with other cmdlets provided by AZ module. In theory, I could try to reuse the AZ module token cache after it is authenticated, so I do not need to authenticate twice, once for the AZ module, and the second time for my Fluent cmdlet.

Or there is another way to reuse the cache?

@weidongxu-microsoft, what do you think?

@weidongxu-microsoft
Copy link
Member

@eosfor

I think the rationale underlying not sharing the token credential is mostly for security reason. The authenticate itself is not costly (maybe 1 additional request per hour?).

The AzureCredentials class itself is public. So it might be easier to extend the class and override ProcessHttpRequestAsync?

@qub1n
Copy link

qub1n commented May 19, 2020

My rationale behind it is that we have multitenant application. The app is used by amny users which access different tenants and different subscriptions based on roles and accounts.

Thats why the app use multiple credentials to access multiple tenants/subscription.

Currently if there is only one shared token cache, we have to solve security boundary by application logic which is quite britle.

I would prefer to have security boundary by architecture, this mean to have token cache by user role or something similar.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented May 19, 2020

@qub1n

Will this PR solve your use case? If so, I will merge it.

@eosfor
Copy link

eosfor commented May 22, 2020

Hi @weidongxu-microsoft , basically my problem is here or here. I tried to overcome this by sharing the AZ cache, but there are other changes necessary to make this all work. It seems like there are conflicting Auth libraries used in the AZ find Fluent, so they cannot be easily loaded into the pwsh process and some other things. But this PR will be just a part of the solution I think. But maybe, together with the AZ module team you could find some other way to make it.

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.

7 participants