Skip to content

Improvements #3478

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
Merged
merged 1 commit into from
Sep 16, 2025
Merged

Improvements #3478

merged 1 commit into from
Sep 16, 2025

Conversation

@jmprieur
Copy link
Collaborator

Sidecar improvements

@jmprieur jmprieur requested a review from a team as a code owner September 16, 2025 18:18
jmprieur
jmprieur commented Sep 16, 2025
View reviewed changes
src/Microsoft.Identity.Web.OidcFIC/OidcFicSignedAssertionProviderExtensions.cs
/// <returns>the service collection for chaining.</returns>
public static IServiceCollection AddOidcFic(this IServiceCollection services)
{
services.AddTokenAcquisition(true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OidcFic can't work with scoped token acquisition :-(

jmprieur
jmprieur commented Sep 16, 2025
View reviewed changes
src/Microsoft.Identity.Web.Sidecar/Endpoints/AuthorizationHeaderEndpoint.cs
[FromBody] DownstreamApiOptions? optionsOverride,
[FromServices] IAuthorizationHeaderProvider headerProvider,
[FromServices] IConfiguration configuration)
[FromServices] IOptionsMonitor<DownstreamApiOptions> optionsMonitor)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's better to get the options directly. and we'll be ready if we want to have a CalldownstreamApis endpoint

jmprieur
jmprieur commented Sep 16, 2025
View reviewed changes
src/Microsoft.Identity.Web.Sidecar/Endpoints/ValidateRequestEndpoints.cs
var claimsPrincipal = httpContext.User;
var token = claimsPrincipal.GetBootstrapToken() as JsonWebToken;

var token = httpContext.GetTokenUsedToCallWebAPI() as JsonWebToken;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See also the simplication in program.cs

jmprieur
jmprieur commented Sep 16, 2025
View reviewed changes
src/Microsoft.Identity.Web.Sidecar/appsettings.json
},

"DownstreamApi": {
"DownstreamApis": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the usual convention

jmprieur
jmprieur commented Sep 16, 2025
View reviewed changes
src/Microsoft.Identity.Web.Sidecar/Program.cs
builder.Services.Configure<JwtBearerOptions>(JwtBearerDefaults.AuthenticationScheme,
options =>
{
options.Events ??= new();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simplication here as this is already done.

jmprieur
jmprieur commented Sep 16, 2025
View reviewed changes
src/Microsoft.Identity.Web.Sidecar/Program.cs
builder.Services.AddHealthChecks();

// Disable claims mapping.
JwtSecurityTokenHandler.DefaultMapInboundClaims = false;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't want Wilson to transform the claims.

jmprieur
jmprieur commented Sep 16, 2025
View reviewed changes
src/Microsoft.Identity.Web.Sidecar/Program.cs
return Task.FromResult(context);
};
// Enable the right role claim type.
options.TokenValidationParameters.RoleClaimType = "roles";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

important for AuthZ

jmprieur
jmprieur commented Sep 16, 2025
View reviewed changes
Copy link
Collaborator Author

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

annotated the changes

@keegan-caruso keegan-caruso merged commit 584b0e4 into sidecar Sep 16, 2025
1 check passed
@keegan-caruso keegan-caruso deleted the jmprieur/sideCarImprovements branch September 16, 2025 18:28
keegan-caruso pushed a commit that referenced this pull request Sep 26, 2025
@jmprieur
Improvements (#3478)
d931fae
keegan-caruso pushed a commit that referenced this pull request Sep 26, 2025
@jmprieur
Improvements (#3478)
daf00e8
keegan-caruso pushed a commit that referenced this pull request Oct 1, 2025
@jmprieur
Improvements (#3478)
ee75dad
keegan-caruso pushed a commit that referenced this pull request Oct 4, 2025
@jmprieur
Improvements (#3478)
187a563
keegan-caruso added a commit that referenced this pull request Oct 7, 2025
@keegan-caruso @jmprieur
Microsoft.Identity.Web.Sidecar (#3524)
8bce157 
* initial commit

* Refactor, add basic error handling

* Adding E2E test (#3476)

* Add authorization header endpoint, minor cleanup

* more end to end tests for sidecar (#3477)

* Adding E2E test

* Improving the tests

* Fix name

* Apply suggestion from @keegan-caruso

Co-authored-by: Keegan <[email protected]>

---------

Co-authored-by: Keegan <[email protected]>

* Add more tests

* Improvements (#3478)

* Fix model bindng (#3479)

Other cleanup and improvements

Co-authored-by: Keegan Caruso <[email protected]>

* Update tests to new behavior + 401 on AuthorizationHeader when we are sure it's an auth issue (#3480)

* Improvements

* fix tests with new behavior

* Apply suggestion from @keegan-caruso

Co-authored-by: Keegan <[email protected]>

* Apply Keegan's suggestion

---------

Co-authored-by: Keegan <[email protected]>

* add downstream api (#3483)

* add downstream api

---------

Co-authored-by: Keegan Caruso <[email protected]>

* Enable container workflow (#3496)

* containerization

* Enable container workflows

---------

Co-authored-by: Jean-Marc Prieur <[email protected]>
Co-authored-by: Keegan Caruso <[email protected]>

* Add more e2e tests (#3504)

Co-authored-by: Keegan Caruso <[email protected]>

* Authorization Header and downstream API endpoint updates (#3507)

* Authorization Header and downstream API endpoints updates
* Fix open api generation

---------

Co-authored-by: Keegan Caruso <[email protected]>

* Sidecar endpoint descriptions (#3510)

* Add more descriptions to open api document
* Adjust for open api doc generation

---------

Co-authored-by: Keegan Caruso <[email protected]>

* Current implementation of Open API description generation is not trim friendly

* Add agentuserid to list of params (#3514)

* Add agentuser id to list of params

* Apply suggestion from @jmprieur

Co-authored-by: Jean-Marc Prieur <[email protected]>

* Apply suggestion from @jmprieur

Co-authored-by: Jean-Marc Prieur <[email protected]>

* update openapi file

---------

Co-authored-by: Keegan Caruso <[email protected]>
Co-authored-by: Jean-Marc Prieur <[email protected]>

* Add windows container (#3516)

Co-authored-by: Keegan Caruso <[email protected]>

* Sidecar python adapter devapp (#3508)

* Python adapter

* feedback

---------

Co-authored-by: Keegan Caruso <[email protected]>

* Add readme (#3517)

* Add readme

* Move runtime composition details in README

move runtime composition section

---------

Co-authored-by: Keegan Caruso <[email protected]>

* Make the sidecar trim friendlier (#3518)

Co-authored-by: Keegan Caruso <[email protected]>

* Update python adapter (#3519)

Co-authored-by: Keegan Caruso <[email protected]>

* Don't use R2R (#3523)

Co-authored-by: Keegan Caruso <[email protected]>

* Exclude EndpointsE2ETests when ran from GH action

* include FROM_GITHUB_ACTION in sidecar tests

* Use in-memory config for sidecar e2e test

* Apply suggestions from code review

Co-authored-by: Jean-Marc Prieur <[email protected]>

* Move SidecarApiFactory to separate file

* Also needs test filter

---------

Co-authored-by: Keegan Caruso <[email protected]>
Co-authored-by: Jean-Marc Prieur <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jennyf19 jennyf19 jennyf19 approved these changes

@keegan-caruso keegan-caruso keegan-caruso approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jmprieur @jennyf19 @keegan-caruso