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.

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

Add Xiaomi provider #675

Closed
wants to merge 4 commits into from
Closed

Add Xiaomi provider #675

wants to merge 4 commits into from

Conversation

ArcherTrister
Copy link
Contributor

@ArcherTrister ArcherTrister commented May 5, 2022

Add an OAuth 2.0 Xiaomi provider.

@martincostello
Copy link
Member

Just one outstanding comment, otherwise this looks good: #675 (comment)

@kevinchalet
Copy link
Member

Thanks for your contribution!

In Europe and in the USA, the "official" casing is generally "Xiaomi", not "XiaoMi". Any chance you could change that?


var address = QueryHelpers.AddQueryString(Options.TokenEndpoint, tokenRequestParameters);

using var response = await Backchannel.GetAsync(address, Context.RequestAborted);
Copy link
Member

Choose a reason for hiding this comment

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

I see the documentation says the token endpoint uses GET, but have you tried using POST to see what it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will rename to Xiaomi, I didn't try to use post request, can you tell me the necessity of using it? I can try.

@ArcherTrister ArcherTrister changed the title Add XiaoMi provider Add Xiaomi provider May 6, 2022
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.

I just realised you also haven't updated the README to add the new provider to the table.

@@ -0,0 +1,20 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

I can't comment on file-names in git diffs, but you also need to fix the casing of the directory and file names.

Copy link
Member

Choose a reason for hiding this comment

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

This is why the ubuntu build is failing due to the case-sensitive file system.

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 confirmed that it was all renamed, but nothing changed after the commit, and I don't know why, so I decided to re-commit, and I will update the README to add the new provider to the table.

xiaomiprovider

Copy link
Member

Choose a reason for hiding this comment

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

Windows is a case-insensitive file system, so just changing the case in the file names won't make them change in git.

You can achieve a rename either by renaming to something else and then back to the intended casing, e.g. XiaoMiAuthenticationOptions2 -> XiaomiAuthenticationOptions, or by using the git mv command, e.g. git mv XiaoMiAuthenticationOptions.cs XiaomiAuthenticationOptions.cs.

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 know the first way but don't like it, I will try the second way in the future, thanks.

@ArcherTrister ArcherTrister deleted the dev-xiaomi branch May 6, 2022 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants