-
Notifications
You must be signed in to change notification settings - Fork 88
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
Add per-request tenant ID option #343
Conversation
There are two things here:
1.17 is out of support. Go team supports two versions, which means 1.18 and 1.19
1.18 introduces generics. I haven't looked but if he ported my libs into here it uses generics in the apply loop.
Charles and I have been talking, and we think we should probably switch these shortly after Go team drops support for the previous version.
What do you think Joel?
Charles, I haven't looked at this yet, but it will probably be Monday because I took a couple days off this week. You may need to ping me, as I tend to forget stuff when I go off for 4 days.
-- Doak
________________________________
From: Joel Hendrix ***@***.***>
Sent: Wednesday, August 17, 2022 11:09 AM
To: AzureAD/microsoft-authentication-library-for-go ***@***.***>
Cc: John Doak ***@***.***>; Mention ***@***.***>
Subject: Re: [AzureAD/microsoft-authentication-library-for-go] Add per-request tenant ID option (PR #343)
@jhendrixMSFT commented on this pull request.
@@ -1,6 +1,6 @@
module github.com/AzureAD/microsoft-authentication-library-for-go
…-go 1.17
+go 1.18
Are we using something new in Go 1.18?
—
Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.luolix.top%2FAzureAD%2Fmicrosoft-authentication-library-for-go%2Fpull%2F343%23pullrequestreview-1076194688&data=05%7C01%7CJohn.Doak%40microsoft.com%7C315b69af032d497e38c508da807b99c8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637963565594323655%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=sSVwg9BwxKzVgw1zFV%2BA9Z9nrWM1f%2FDwOmiNc%2B5IGK0%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.luolix.top%2Fnotifications%2Funsubscribe-auth%2FAKCSL7BNLTNCPFCX6TLQNTLVZUTE3ANCNFSM56XUJKOQ&data=05%7C01%7CJohn.Doak%40microsoft.com%7C315b69af032d497e38c508da807b99c8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637963565594323655%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=amC11H3dj%2B1ui7b8hf9CWVVZcTl2voQZKq1V9RYmtVc%3D&reserved=0>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
1e38b23
to
eb253f6
Compare
apps/confidential/confidential.go
Outdated
AcquireSilentOption | ||
shared.CallOption | ||
}{ | ||
CallOption: shared.NewCallOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add another restriction here, i.e. we should not allow the tenantID
to be common
or organizations
because the intent of this option is to help multi-tenant apps get tokens for specific tenants only. It's ok to go from general to specific, i.e. authority = "l.m.o/common" and tenant = "12345-..", but there aren't any scenarios where you'd want to go from "common" to "common" or from "12345" to "common".
Never mind, this is implemented in authority.go and there are tests for it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just FYI / for context, both in client_credentials flow and in OBO flow, calls should not be made over "common" / "organizations".
- in client_credentials we should outright ban it, because "common" is only supposed to work with user flows
- in OBO, if you've got a multi-tenant app, the tenant should be
incomming_assertion.tid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"common" is the default confidential client tenant here, so I opened #348 to track addressing your first point. For OBO, you mean that the application should request a token from the same tenant the user authenticated in i.e., the value of the assertion's tid
claim?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed from the options part of the code, which lgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM although somebody from the MSAL should review the MSAL-specific parts.
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the discussion on the feature. This implementation looks good to me.
This closes #296 by adding a new option,
WithTenantID
, for public and confidential client token acquisition methods. Tenant resolution follows the logic laid out in #296; see the new tests. Feel free to just look at the tests if you aren't interested in the Go plumbing that makes this new option work.WithTenantID
is straightforward to use in an application but its implementation is complex because it must function alongside options which apply only to particular methods, those methods must accept it without a breaking change to their signatures, and we still want the compiler to prevent users from passing invalid options to a method. Credit goes to @element-of-surprise for the pattern I implemented here.I exported the new
*Option
interfaces supportingWithTenantID
to enable users to fakeClient
with their own interfaces (but users can't implement the*Option
interfaces). Sadly, this clutters the public API with overlapping names, for example: