Skip to content

Replace the policy name attribute in MapSpaYarp with IEndpointConventionBuilder#31

Open
bve-wd wants to merge 1 commit into
berhir:masterfrom
bve-wd:feature/make-endpoint-convention-builder-accessible
Open

Replace the policy name attribute in MapSpaYarp with IEndpointConventionBuilder#31
bve-wd wants to merge 1 commit into
berhir:masterfrom
bve-wd:feature/make-endpoint-convention-builder-accessible

Conversation

@bve-wd

@bve-wd bve-wd commented Mar 14, 2024

Copy link
Copy Markdown

Return the IEndpointConventionBuilder instead of passing a policyName.
This allows more flexibility in the configuration of the endpoint such as AllowAnonymous or RequireAuthorization.

Also added a section in the Readme.md and fixed some small issues which prevented me from building (e.g. the version in global.json).

Increased the version from 2.0.1 to 2.0.2.

…pointConventionBuilder instead. This allows more flexibility in the configuration of the endpoint such as AllowAnonymous or RequireAuthorization.

@HanoRossouw HanoRossouw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed code changes for exposing IEndpointConventionBuilder to pass additional config on the endpoint up to the implementer. No faulty code changes found and logic seem sound and allows for greater capabilities in usage while reducing concerns of this package also

@bve-wd

bve-wd commented Aug 19, 2024

Copy link
Copy Markdown
Author

I just looked again at my changes and saw that I introduced a breaking change.
Even re-adding the policyName parameter would be a breaking change though, since the return value changed, which could break existing code if the IEndpointRouteBuilder is used in a chained configuration.
Thus, this change should increase the major version, if semantic versioning is used.

However, there was no change for a long time, so I am not sure if it makes sense to change anything in my PR.

@HanoRossouw

Copy link
Copy Markdown

I just looked again at my changes and saw that I introduced a breaking change. Even re-adding the policyName parameter would be a breaking change though, since the return value changed, which could break existing code if the IEndpointRouteBuilder is used in a chained configuration. Thus, this change should increase the major version, if semantic versioning is used.

However, there was no change for a long time, so I am not sure if it makes sense to change anything in my PR.

I can confirm it does cause an issue if you do not handle the null in the pipeline when the spa.proxy.json file is not present.

var point = app.MapSpaYarp();

if(point != null)
{
    point.AllowAnonymous();
}

but honestly is a minor issue compared to having to send in a policyname or allow anonymous boolean flag to control endpoint auth

@bve-wd

bve-wd commented Aug 20, 2024

Copy link
Copy Markdown
Author

Currently there is not even a possibility to call AllowAnonymous() for the SpaYarp catch all path. That was the main reason for the change, since it is currently unconvenient to allow anonymous access if MapSpaYarp() is combined with AddAuthorization() when an authenticated user is requested by default. It would be possible though, to create a custom authorization policy without rules, such that it behaves like AllowAnonymous, but that is not an elegant solution if the convention builder can be exposed instead.

@bve-wd

bve-wd commented Oct 29, 2024

Copy link
Copy Markdown
Author

I consider this projekt as dead and will not continue pushing changes to it.
Feel free to accept or reject my PR.

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.

3 participants