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 authflow option #453

Closed
wants to merge 3 commits into from
Closed

Conversation

LucienHH
Copy link
Contributor

Allow users to specify their own Authflow instance. Useful if you have a project utilising prismarine-auth and want to share credentials / caching solutions.

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.

Are you sure this is a good idea? Seems not to me. Any changes here would need to be propagated to node-minecraft-protocol also. The idea has always been that authentication has been an implementation detail, so changes can be made internally without breaking API changes. Expecting outside implementation here seems constraining should we need to make changes to prismarine-auth. Prior to that we handled authentication internally in the library, which may happen again in the future if authentication changes again and we need to change authentication implementations.

@LucienHH
Copy link
Contributor Author

LucienHH commented Oct 3, 2023

My original problem was that I was trying to create a client and also utilize Pauth to authenticate requests to the Xbox API within the same app. Originally, I just passed identical options between the Authflow and the client instance so that the user wouldn't have to authenticate twice. But that felt like a hacky solution as two flows were interacting with the same cache. Maybe another solution would be to just attach the Authflow to the client rather than the options object?

In my opinion I don't see an issue with breaking changes in Pauth. If Bedrock-protocol is expecting a certain result from auth and doesn't get it due to the user providing their own, it should just error, and the user should update their dependencies. If these changes required users to provide an Authflow, I think it'd make more sense to be cautious. However with it being an option, the user should be aware of the implications that come with handling auth themselves.

@extremeheat
Copy link
Member

extremeheat commented Oct 3, 2023

My point is that .authflow would become a stable part of the API. Actually at the moment, a lot of undocumented non-underscored props are unstable and maybe should have an underscore prefix to them. Being stable is a symver commitment that we keep it going without needing a major bump bedrock-protocol@v4 (which is ok if needed).

My main concern here is this prevents changing prismarine-auth construction to add additional parameters or other API changes, like the removal of pauth, like we were able to do without any user changes going from mojang to Microsoft auth in node-minecraft-protocol.

Normally things like this are bad API as it can cause 2 different versions of a package to collide, as opposed to accepting a generic object that implements a certain interface. Maybe a custom auth() function like exists in nmp would make more sense.

@LucienHH
Copy link
Contributor Author

LucienHH commented Oct 9, 2023

Ah yeah i see your point, ok I'll put this on the back burner for now. Maybe I'll revisit it later on

@LucienHH LucienHH closed this Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants