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.

Sign up for GitHub

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

Jump to bottom

Access Token Proof of Possession Capability #2512

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Access Token Proof of Possession Capability #2512

wants to merge 7 commits into from

Conversation

FehintolaObafemi
Copy link
Contributor

@FehintolaObafemi FehintolaObafemi commented Jan 3, 2024 •
edited
Loading

Fixes #

Changes proposed in this pull request

  • Introducing an Access Token Proof of Possession capability for this SDK
  • Updating the dependencies to the latest version to support all recent changes
  • Adding the option to specify http method for AT PoP bound token acquisition
  • Adding the option to specify the URI for AT PoP bound token acquisition

Other links

@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/AT-PoP branch 3 times, most recently from f46153c to d79e836 Compare February 29, 2024 18:22
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/AT-PoP branch 2 times, most recently from 929b2b4 to 4d5e435 Compare June 5, 2024 17:15
@FehintolaObafemi FehintolaObafemi added enhancement New feature or request and removed Do not merge labels Jun 5, 2024
adamedx
adamedx reviewed Jun 10, 2024
View reviewed changes
docs/AT-Pop.md
To enable AT PoP, configure the Microsoft Graph SDK options as follows:

```powershell
Set-MgGraphOption -EnableATPoP $true
Copy link

Choose a reason for hiding this comment

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

Should we release this specific feature as preview / experimental? Does MS Graph PS have this capability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good idea. @timayabi2020 can this be released as a preview version similar to the version 2.0 roll out?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it can, however I think we should first publish the feature to an internal feed and get a few guys to test internally

@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/AT-PoP branch 2 times, most recently from e90f311 to 13e2057 Compare June 13, 2024 18:25
@FehintolaObafemi FehintolaObafemi marked this pull request as ready for review June 13, 2024 18:25
@FehintolaObafemi FehintolaObafemi requested a review from a team as a code owner June 13, 2024 18:25
darrelmiller
darrelmiller reviewed Jun 26, 2024
View reviewed changes
src/Authentication/Authentication.Core/Microsoft.Graph.Authentication.Core.csproj
<PackageReference Include="Azure.Identity.Broker" Version="1.1.0" />
<PackageReference Include="Microsoft.Graph.Core" Version="3.0.9" />
<PackageReference Include="Azure.Identity" Version="1.12.0-beta.3" />
<PackageReference Include="Azure.Identity.Broker" Version="1.2.0-beta.1" />
Copy link
Contributor

Choose a reason for hiding this comment

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

In previous conversations, we had wondered if Azure.Identity is actually bringing any value here over using Microsoft.Identity.Client directly. Azure.Identity is intended to limit the surface area exposed to 3P developers. This is not relevant for PowerShell scenarios. I am especially concerned when I see the introduction of Azure.Core.Pipeline. We explicitly don't use the Azure Pipelining because it is only there to abstract away language differences, and again that isn't relevant because our PowerShell modules are only implemented in C#.

@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/AT-PoP branch 5 times, most recently from d2d03fa to ff194cb Compare July 15, 2024 20:31
@FehintolaObafemi FehintolaObafemi force-pushed the fehintolaobafemi/AT-PoP branch 2 times, most recently from 8b69aea to a1f778a Compare July 25, 2024 21:38
timayabi2020 and others added 7 commits August 28, 2024 12:47
@timayabi2020 @FehintolaObafemi
Rolled back devicemanagement to the one that 2.19.0 had.
8a295f0
@FehintolaObafemi
adding "-AT PoP" option to "Set-MgGraphOptions"
09d1981 
Adding AT PoP skeleton (#2511)

* adding "-AT PoP" option to "Set-MgGraphOptions"

---------

AT PoP Version 1

Fehintolaobafemi/methodanduri (#2751)

* Making changes to how httpmethod and uri is processed

---------

Fixing and updating the docs

resolving build break

fixing build issues related to docs

resolving PR comments

updating nuget packages

removing unnecessary string assignment

Resolving PR comments

reverting test csproj changes
@FehintolaObafemi
Resolving comments regarding static instance
b63a670
@FehintolaObafemi
resolving issues with PS 7.4.4
0d7f175
@FehintolaObafemi
Fixed pester test expected messages
252fcd0
@FehintolaObafemi
Updated expected message
4ca2480
@FehintolaObafemi
Updated error message
2136757
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darrelmiller darrelmiller darrelmiller left review comments

@nkasco nkasco nkasco left review comments

@adamedx adamedx adamedx left review comments

@andrueastman andrueastman andrueastman left review comments

@timayabi2020 timayabi2020 timayabi2020 left review comments

@bgavrilMS bgavrilMS bgavrilMS left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@FehintolaObafemi @darrelmiller @nkasco @adamedx @andrueastman @timayabi2020 @bgavrilMS