Skip to content

Conversation

@joskraps
Copy link
Contributor

Allows the usage of custom extensions via inspecting the authentication property parameter collection.

This is to fix issue #72

@joskraps joskraps requested a review from kevinchalet February 18, 2020 19:20
Copy link
Member

@kevinchalet kevinchalet left a comment

Choose a reason for hiding this comment

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

I'm afraid this is not the kind of fix I would take: the Attributes collection is meant to be used as a way to request AX attributes (see https://openid.net/specs/openid-attribute-exchange-1_0.html), not to define arbitrary extensions.

To support sending custom parameters (defined in custom extensions), consider implementing #13 instead.

@joskraps joskraps requested a review from kevinchalet February 18, 2020 22:11
@joskraps
Copy link
Contributor Author

Reverted code and implemented the RedirectToProvider notification. Tested it out in our app and was able to accomplish what I needed via that event.

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

Any tests you can add to validate this scenario?

@joskraps
Copy link
Contributor Author

@kevinchalet Is this capable of being tested via the test project? I've looked through and did not see where the other implemented event has any tests or an example.

@joskraps
Copy link
Contributor Author

@kevinchalet Are you waiting on anything from me to get this pull request through?

@joskraps
Copy link
Contributor Author

@martincostello @kevinchalet Any update on what is holding this up or if you will be moving forward with accepting this change? I'd prefer to use an official package from this project vs a forked version with the change.

@kevinchalet kevinchalet merged commit 16118b4 into aspnet-contrib:dev Mar 10, 2020
@kevinchalet
Copy link
Member

Merged, thanks for your PR. A new version should be released soon, but in the meantime, feel free to use the nightly builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants