Add token authentication #780

stenlan · 2020-11-30T22:57:12Z

(this assumes PrismarineJS/node-yggdrasil#51)
Fixes #519. It allows the user to specify a profilesFolder property. If given, a launcher_profiles.json file will be read or created if it doesn't yet exist, and it will then act approximately like the minecraft launcher. The given username is searched within the launcher profiles and if found, it attempts to use this information to login. If not found, or the login fails, it can optionally use a password if it's given. If not, it will fail. On any successful login, if the profilesFolder property is given, it will save it properly to the file. This works for preexisting and non-preexisting profiles and is compatible with the official launcher. It will only save it if the clientToken matches so as to not corrupt the existing launcher_profiles.json. This will match by default, and might only mismatch if another clientToken or session with clientToken is passed as an options property.

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#519: add an option to store and use auth token directly in nmp

DeltaEvo

Nice feature !
In your code a lot of let can be transformed to const
And I don't think we need theses extra dependencies

src/client/auth.js

DeltaEvo · 2020-12-01T10:46:31Z

src/client/auth.js

+          || Object.values(auths.authenticationDatabase[key].profiles)[0].displayName.toLowerCase() == lowerUsername
+        )
+        if (err) {
+          if (profile) { // profile is invalid, remove


Are we sure that the profile is invalid here ? We should check the error since a lot of kind of error can happen here

I have thought about this, and it should be fine like this IMO. The minecraft launcher itself does the same thing I think, if your login doesn't succeed for whatever reason, it invalidates it

src/client/auth.js

Co-authored-by: David Duarte <deltaduartedavid@gmail.com>

stenlan · 2020-12-01T15:50:35Z

@DeltaEvo I think I fixed most of the stuff you mentioned, any thoughts?

src/client/auth.js

DeltaEvo · 2020-12-01T16:11:02Z

Yep it seems better, only

In your code a lot of let can be transformed to const

is still valid :)

wvffle · 2020-12-01T16:14:02Z

You should resolve things that you've already done

stenlan · 2020-12-01T16:17:27Z

Okay I resolved most, the remaining ones have to do with error handling. That's mainly because I'm not sure what the preferred way to go about that is in this project. If I know this I can properly implement it

stenlan · 2020-12-01T16:17:54Z

And transforming let to const I guess...

stenlan · 2020-12-01T19:00:26Z

Ok I changed that. I still left it in an async function because I prefer the await operator over ugly .then chaining. If you ask me, this pr is ready to merge now

stenlan · 2020-12-04T15:08:36Z

Welp, idk why the java env setup is failling in the tests, but lmk what needs to be done to get this merged. Seems like it would be a useful feature, and it's also on the "big prismarine projects" list I think. If you don't want it just tell me and I'll close this

https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ see also PrismarineJS#780

Gjum · 2020-12-04T18:39:57Z

The java env setup seems to be failling because of a recent (Oct. 1) security vulnerability fix on Github's CI. It's not really in-scope of this PR, so I've attempted a fix in #785. If that fix works, just re-run the CI (if that's possible? or just push an empty commit to trigger it).

https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ see also #780

Heath123 · 2020-12-04T19:37:41Z

src/client/auth.js

+      return JSON.parse(await fs.readFile(options.profilesFolder + '/launcher_profiles.json', 'utf8'))
+    } catch (err) {
+      await fs.mkdir(options.profilesFolder, { recursive: true })
+      await fs.writeFile(options.profilesFolder + '/launcher_profiles.json', '{}')


Will this mess with the launcher at all?

Yes, 'mess' as in it will modify that file. It's compatible with the official launcher, which means it properly deletes and also saves new accounts to the file for the launcher to use

Also, one could just specify a path that is not the launcher folder and it will just use its own isolated file instead

Merge in original repo again

stenlan · 2020-12-04T19:49:41Z

Thanks a lot @Gjum, the tests are all passing now

rom1504 · 2020-12-05T16:01:42Z

looks good.
Missing:

fix tests in Add ability to request user from token refresh node-yggdrasil#51
add new option in API.md doc
why not use the home folder by default so it's enabled by default ? this would remove hundred of opened issues and discord complaints (ie people getting temporarily banned by mojang due to too often auth requests)

Heath123 · 2020-12-05T16:43:17Z

I have a report of this error:

(node:22920) UnhandledPromiseRejectionWarning: TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at module.exports (C:\Users\*****\Desktop\pakkit\node_modules\minecraft-protocol\src\client\auth.js:81:30)

(happens at https://github.com/ph0t0shop/node-minecraft-protocol/blob/master/src/client/auth.js#L81)

Merge newest master

stenlan · 2020-12-05T17:11:56Z

why not use the home folder by default so it's enabled by default ? this would remove hundred of opened issues and discord complaints (ie people getting temporarily banned by mojang due to too often auth requests)

@rom1504 I was a bit hesitant to do this, firstly because then we need to find the minecraft folder on the user's system, and secondly because then how would one specify that they just don't want to save credentials? Also, it might be unexpected for the user that it suddenly uses their local minecraft folder

rom1504 · 2020-12-05T17:17:41Z

https://github.com/PrismarineJS/node-minecraft-wrap/blob/master/lib/wrap_client.js#L11 works pretty well to find the path
To specify not to save the user could put this option as false

About it being unexpected that it would use their folder, I think it's actually what most people would expect by default.
We can add a note in the readme that we're doing that and how to disable it.

I think it's really beneficial to enable this by default, as the current behavior is to call the mojang server too many times which is actually not great.

rom1504 · 2020-12-05T17:19:42Z

https://github.com/simonmeusel/minecraft-folder-path/blob/master/index.js we could also just copy these few lines instead of depending on this package

…

On Sat, Dec 5, 2020, 18:12 ph0t0shop ***@***.***> wrote: why not use the home folder by default so it's enabled by default ? this would remove hundred of opened issues and discord complaints (ie people getting temporarily banned by mojang due to too often auth requests) @rom1504 <https://github.com/rom1504> I was a bit hesitant to do this, firstly because then we need to find the minecraft folder on the user's system, and secondly because then how would one specify that they just don't want to save credentials? Also, it might be unexpected for the user that it suddenly uses their local minecraft folder — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#780 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR437UTKO7SEVZZCSAEQE3STJSWRANCNFSM4UIGXIAQ> .

Karang · 2020-12-05T17:27:03Z

What needs to be enabled by default is saving the token, not necessary the credentials. What I would do is:

if the user specifies mc directory: use it to load/store credentials and token
by default: save/load the token only in the local or tmp directory

stenlan · 2020-12-05T17:28:17Z

Okay. I will do this in a bit, and also add some error handling for invalid launcher profile files

rom1504 · 2020-12-05T18:18:24Z

Hmmm does the vanilla client save the credentials ? I think it doesn't
Saving the token is good but saving the password not so much

rom1504 · 2020-12-05T18:22:28Z

Yeah and I see you also don't store the password.
What did you mean by credentials @Karang ?

Karang · 2020-12-05T19:09:31Z

Yeah and I see you also don't store the password.
What did you mean by credentials @Karang ?

I was thinking about username + password but yeah thats probably not a good idea.

stenlan · 2020-12-05T19:13:32Z

Oh yeah you definitely don't wanna do that. Currently, the password can be specified, and it will be used to login only if it can't use the launcher_profiles for the account, and then saves it as well. This IMO is the most intuitive and mimics the launcher behavior pretty pragmatically

…protocol

examples/client_token_auth/client_token_auth.js

This reverts commit 0277091. This commits introduced a bug in offline mode. We don't know yet how to fix it, so better revert first and fix later

This reverts commit 14c4886. Adds back token auth now that mineflayed is fixed

added in PrismarineJS/node-minecraft-protocol#780

https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ see also PrismarineJS#780

* Add token authentication * Update package.json * Update src/client/auth.js Co-authored-by: David Duarte <deltaduartedavid@gmail.com> * Remove some deps * Fix merge conflict + fix some token stuff * Fix even more stuff + testing * Change back echo example * Let -> const + bugfix * Fixed error handling * Revert client_echo again... * Fix linter errors * Profile improvements * Stupid linter errors * Update yggdrasil, remove token auth example, move to client_echo instead * Update client_echo * Linter error... Co-authored-by: David Duarte <deltaduartedavid@gmail.com>

This reverts commit 0277091. This commits introduced a bug in offline mode. We don't know yet how to fix it, so better revert first and fix later

This reverts commit 14c4886. Adds back token auth now that mineflayed is fixed

stenlan added 2 commits November 30, 2020 23:49

Add token authentication

3a4b6c4

Update package.json

508f1cc

DeltaEvo suggested changes Dec 1, 2020

View reviewed changes

stenlan and others added 6 commits December 1, 2020 15:55

Update src/client/auth.js

e9f75ab

Co-authored-by: David Duarte <deltaduartedavid@gmail.com>

Remove some deps

c416a15

Fix merge conflict + fix some token stuff

970e4b2

Fix even more stuff + testing

ed90606

Fix merge conflict

09d38e5

Change back echo example

b3d3a14

wvffle requested changes Dec 1, 2020

View reviewed changes

src/client/auth.js Outdated Show resolved Hide resolved

src/client/auth.js Outdated Show resolved Hide resolved

src/client/auth.js Outdated Show resolved Hide resolved

DeltaEvo approved these changes Dec 1, 2020

View reviewed changes

stenlan added 2 commits December 1, 2020 18:22

Let -> const + bugfix

a46670a

Fixed error handling

10ebd98

stenlan added 2 commits December 1, 2020 20:02

Revert client_echo again...

5277930

Fix linter errors

81a1c07

Gjum added a commit to Gjum/node-minecraft-protocol that referenced this pull request Dec 4, 2020

[CI] update to actions/setup-java@v1.4.3

13ffd63

https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ see also PrismarineJS#780

Gjum mentioned this pull request Dec 4, 2020

[CI] update to actions/setup-java@v1.4.3 #785

Merged

rom1504 pushed a commit that referenced this pull request Dec 4, 2020

[CI] update to actions/setup-java@v1.4.3 (#785)

0d0b4d5

https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ see also #780

Heath123 reviewed Dec 4, 2020

View reviewed changes

Merge pull request #1 from PrismarineJS/master

466d65a

Merge in original repo again

rom1504 added the waiting for info label Dec 5, 2020

Merge pull request #2 from PrismarineJS/master

f1f021b

Merge newest master

stenlan added 3 commits December 5, 2020 22:08

Profile improvements

f91c57e

Merge branch 'master' of https://github.com/ph0t0shop/node-minecraft-…

71867f1

…protocol

Stupid linter errors

64eaccc

rom1504 reviewed Dec 5, 2020

View reviewed changes

examples/client_token_auth/client_token_auth.js Outdated Show resolved Hide resolved

rom1504 reviewed Dec 5, 2020

View reviewed changes

examples/client_token_auth/client_token_auth.js Outdated Show resolved Hide resolved

stenlan added 3 commits December 5, 2020 23:20

Update yggdrasil, remove token auth example, move to client_echo instead

113c0dc

Update client_echo

eb2a4f0

Linter error...

26392e2

rom1504 approved these changes Dec 5, 2020

View reviewed changes

rom1504 merged commit 0277091 into PrismarineJS:master Dec 5, 2020

rom1504 added a commit that referenced this pull request Dec 6, 2020

Revert "Add token authentication (#780)"

14c4886

This reverts commit 0277091. This commits introduced a bug in offline mode. We don't know yet how to fix it, so better revert first and fix later

rom1504 added a commit that referenced this pull request Dec 6, 2020

Revert "Revert "Add token authentication (#780)""

011b6a1

This reverts commit 14c4886. Adds back token auth now that mineflayed is fixed

MrGeorgen added a commit to themoonisacheese/2bored2wait that referenced this pull request Jan 8, 2021

We don't need to use prismarine-tokens because the same feature was

f4ecd89

added in PrismarineJS/node-minecraft-protocol#780

matthi4s pushed a commit to aternosorg/node-minecraft-protocol that referenced this pull request Jun 9, 2021

Revert "Revert "Add token authentication (PrismarineJS#780)""

f89d26c

This reverts commit 14c4886. Adds back token auth now that mineflayed is fixed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add token authentication #780

Add token authentication #780

stenlan commented Nov 30, 2020 •

edited by issuehunt-oss bot

DeltaEvo left a comment

DeltaEvo Dec 1, 2020

stenlan Dec 1, 2020

stenlan Dec 4, 2020

stenlan commented Dec 1, 2020

DeltaEvo commented Dec 1, 2020

wvffle commented Dec 1, 2020

stenlan commented Dec 1, 2020

stenlan commented Dec 1, 2020

stenlan commented Dec 1, 2020

stenlan commented Dec 4, 2020

Gjum commented Dec 4, 2020 •

edited

Heath123 Dec 4, 2020

stenlan Dec 4, 2020

stenlan Dec 4, 2020

stenlan commented Dec 4, 2020

rom1504 commented Dec 5, 2020

Heath123 commented Dec 5, 2020

stenlan commented Dec 5, 2020 •

edited

rom1504 commented Dec 5, 2020

rom1504 commented Dec 5, 2020 via email

Karang commented Dec 5, 2020

stenlan commented Dec 5, 2020

rom1504 commented Dec 5, 2020

rom1504 commented Dec 5, 2020

Karang commented Dec 5, 2020

stenlan commented Dec 5, 2020

Add token authentication #780

Add token authentication #780

Conversation

stenlan commented Nov 30, 2020 • edited by issuehunt-oss bot

Referenced issues

DeltaEvo left a comment

Choose a reason for hiding this comment

DeltaEvo Dec 1, 2020

Choose a reason for hiding this comment

stenlan Dec 1, 2020

Choose a reason for hiding this comment

stenlan Dec 4, 2020

Choose a reason for hiding this comment

stenlan commented Dec 1, 2020

DeltaEvo commented Dec 1, 2020

wvffle commented Dec 1, 2020

stenlan commented Dec 1, 2020

stenlan commented Dec 1, 2020

stenlan commented Dec 1, 2020

stenlan commented Dec 4, 2020

Gjum commented Dec 4, 2020 • edited

Heath123 Dec 4, 2020

Choose a reason for hiding this comment

stenlan Dec 4, 2020

Choose a reason for hiding this comment

stenlan Dec 4, 2020

Choose a reason for hiding this comment

stenlan commented Dec 4, 2020

rom1504 commented Dec 5, 2020

Heath123 commented Dec 5, 2020

stenlan commented Dec 5, 2020 • edited

rom1504 commented Dec 5, 2020

rom1504 commented Dec 5, 2020 via email

Karang commented Dec 5, 2020

stenlan commented Dec 5, 2020

rom1504 commented Dec 5, 2020

rom1504 commented Dec 5, 2020

Karang commented Dec 5, 2020

stenlan commented Dec 5, 2020

stenlan commented Nov 30, 2020 •

edited by issuehunt-oss bot

Gjum commented Dec 4, 2020 •

edited

stenlan commented Dec 5, 2020 •

edited