-
-
Notifications
You must be signed in to change notification settings - Fork 588
Add Alibaba Cloud (Aliyun) to the list of supported providers #2256
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
Merged
Merged
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
2cc49d8
Add Alibaba Cloud (Aliyun) to the list of supported providers
gehongyan 147a70a
Amend grant types
gehongyan 82e2418
Attach client id for revocation
gehongyan 29c66b8
Removes redundant handlers
gehongyan c42ed85
Add parameters access_type and prompt
gehongyan d2209f8
Use predefined standard OIDC parameters instead
kevinchalet 22db6c8
Adjust AmendClientAuthenticationMethods to add methods manually
gehongyan a134f39
Use the property accessor instead of the dedicated property
kevinchalet 4938ddf
Use the dedicated resources property but guard it with a null/empty c…
kevinchalet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Note: OpenIddict is expected to take care of sending the client credentials automatically (client_id/client_secret/client_assertion). Were you seeing an error without that custom handler?
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.
In my recollection, the first time I invoked the token revocation, it returned an error saying the request was missing the
client_idparameter. So, I immediately tried adding this handler. After that, the error message changed to indicating that onlyrefresh_tokencould be revoked.Strangely, after removing the handler, I haven't been able to reproduce the original error. Furthermore, after adjusting the console sample, the token revocation request succeeded. Therefore, given the current situation, I'll remove it for now.
Uh oh!
There was an error while loading. Please reload this page.
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.
Maybe the first time you tried, your client was confidential and was assigned a client secret?
When OpenIddict doesn't find an explicit
revocation_endpoint_auth_methods_supportednode in the server configuration, it usesclient_secret_basic- i.e the default method - if both a client identifier and client secret are expected to be sent. In that case, the client credentials are not sent as regular OAuth 2.0 parameters but are sent using the HTTPWWW-Authenticateheader. If no client secret is configured, theclient_idis sent alone as a regular OAuth 2.0 parameter.Can you please check the type of client you configured with Alibaba's portal?
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 tested both WebApp and NativeApp, and both can be revoked successfully.
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.
After changing to web app, you updated your OpenIddict registration to call
SetClientSecret(...), right?Anyway, I took a look at the docs and they don't mention basic authentication support.
Just in case, we should probably tweak the returned configuration response to add
client_secret_postto bothtoken_endpoint_auth_methods_supportedandrevocation_endpoint_auth_methods_supportedso that client credentials are always sent as part of the request form.Can you please give it a try?
openiddict-core/src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationHandlers.Discovery.cs
Lines 262 to 361 in 7517252
Uh oh!
There was an error while loading. Please reload this page.
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.
Actually, I added registrations for both application types in my test code to conduct these tests.
Test code
Sounds good. Completed. All of the registrations above still work well.