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

Add Bilibili provider #1044

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

Add Bilibili provider #1044

wants to merge 11 commits into from

Conversation

Loongle
Copy link
Contributor

@Loongle Loongle commented Mar 21, 2025

martincostello
martincostello reviewed Mar 22, 2025
View reviewed changes
martincostello
martincostello approved these changes Mar 27, 2025
View reviewed changes
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.

LGTM, but pinging @kevinchalet for a review after the conversation in the other PR recently.

kevinchalet
kevinchalet reviewed Mar 27, 2025
View reviewed changes
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.

Thanks for your PR! ❤️

src/AspNet.Security.OAuth.Bilibili/BilibiliAuthenticationHandler.cs Outdated
{
var parameters = new Dictionary<string, string?>
{
["client_id"] = Options.ClientId, // Used instead of "client_id"
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused, isn't the parameter named client_id here too? 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After removing the ["client_id"] = Options.ClientId, the API returned Key: 'AccessTokenReq.ClientId' Error:Field validation for 'ClientId' failed on the 'required' tag.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't suggest not sending the client_id parameter (which is standard and mandatory), it's just that the comment is wrong since the standard client_id (and not something else) is used here.

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 has enabled me to learn about the standardization of code comments. Thank you. ❤️

I have commit.

kevinchalet
kevinchalet reviewed Mar 30, 2025
View reviewed changes
src/AspNet.Security.OAuth.Bilibili/BilibiliAuthenticationHandler.cs Outdated
request.Headers.Add("x-bili-accesskeyid", Options.ClientId);
request.Headers.Add("x-bili-content-md5", "d41d8cd98f00b204e9800998ecf8427e");
request.Headers.Add("x-bili-signature-method", "HMAC-SHA256");
request.Headers.Add("x-bili-signature-nonce", utcNow.ToUnixTimeMilliseconds().ToString(CultureInfo.InvariantCulture));
Copy link
Member

Choose a reason for hiding this comment

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

Let's generate a real/random 256-bit nonce here using Base64Url.EncodeToString(RandomNumberGenerator.GetBytes(256 / 8)) instead of using the current date (which may not be unique if multiple authentication demands are processed in parallel).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have commit.

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

@kevinchalet kevinchalet kevinchalet left review comments

@martincostello martincostello martincostello approved these changes

Assignees
No one assigned
Labels
new provider
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Loongle @martincostello @kevinchalet