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

Update mcs entitlements endpoint #96

Closed

Conversation

frej4189
Copy link

@frej4189 frej4189 commented Apr 12, 2024

@frej4189
Copy link
Author

Tests failing. Not entirely sure how this test suite could ever succeed?

@LucienHH
Copy link
Contributor

Test are failing due to the default clientId:

clientId: '389b1b32-b5d5-43b2-bddc-84ce938d6737', // token from https://github.com/microsoft/Office365APIEditor

No longer being a valid client thus causing the test for fetching deviceCode to fail.

Honestly I'm not sure of the benefit of keeping msal as our default flow, we might as well default to live and set authTitle to Minecraft Nintendo. msal is only really needed if using a custom azure app.

@extremeheat
Copy link
Member

Yeah, it can be changed to default to live. Although we have to default to a Minecraft bedrock token which may not be appropriate for all users, more importantly we want to keep auth API working without breaking auth workflows for downstream users. Will be done with #97

@extremeheat
Copy link
Member

Can you rebase on master?

@extremeheat extremeheat added the waiting for op Waiting for details/action from author label Apr 14, 2024
@frej4189
Copy link
Author

Rebased

Copy link
Member

@extremeheat extremeheat left a comment

Choose a reason for hiding this comment

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

Would be great to know when this changed. Can anyone test to make sure it works?

@frej4189
Copy link
Author

Would be great to know when this changed. Can anyone test to make sure it works?

We run with this endpoint at https://minecraftafk.com (managing thousands of accounts). We have no reported auth issues since making the update.

Copy link
Member

@extremeheat extremeheat 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 tested against both present master and your PR, and they both are working which is a bit odd. They do not return the same data so this could be a breaking change, what do we do about that and do you know why one endpoint would work and not the other?

@extremeheat
Copy link
Member

From a quick check of node-minecraft-protocol we also don't actually verify anything inside entitlements, but only use it for logging debug information. Assuming nothing else big is using it I think we can make this change with a note in the API changelog without breaking anything.

The linked issues PrismarineJS/mineflayer#3286 and PrismarineJS/mineflayer#2890 have to do with the /profile endpoint so any changes to the Minecraft java entitlements endpoint here would be unrelated to them. Do you think the PR is still separately valid?

@frej4189
Copy link
Author

The linked issues PrismarineJS/mineflayer#3286 and PrismarineJS/mineflayer#2890 have to do with the /profile endpoint so any changes to the Minecraft java entitlements endpoint here would be unrelated to them. Do you think the PR is still separately valid?

That's a reasonable assumption but actually incorrect. When refreshing tokens for Xbox Game Pass accounts (and likely other newer accounts) without fetching entitlements, the profile endpoint will return no data. As soon as you make the entitlements request, the profile endpoint is populated. Mojang at their best.

@frej4189
Copy link
Author

They do not return the same data so this could be a breaking change, what do we do about that and do you know why one endpoint would work and not the other?

I guess the old endpoint is preserved for backwards compatibility (why?). As (kind of) mentioned above the current endpoint only seems to cause issues when trying to refresh the token of a newer account (my assumption is post-migration).

@extremeheat
Copy link
Member

Can you update the types?

miosenpai added a commit to miosenpai/hoplite-stats that referenced this pull request Apr 24, 2024
@extremeheat
Copy link
Member

Finished in #101, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for op Waiting for details/action from author
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Error: Failed to obtain profile data for X, does the account own minecraft?
3 participants