Skip to content
This repository has been archived by the owner on May 29, 2022. It is now read-only.

Authentication implementation. #321

Closed
wants to merge 36 commits into from
Closed

Authentication implementation. #321

wants to merge 36 commits into from

Conversation

sadcenter
Copy link
Contributor

Description

Changes auth implementation from yggdrasil one to mine one. (toggleable in configuration)
It uses https://github.com/Electroid/mojang-api and caches the player's texture to the file.

Fixes/Adds #127 (Not completely due it's impossible, more info in Additional comments section.)

How has this been tested?

Tested by generating skulls and toggling online-mode

Additional Comments

It's impossible to remove the delay completely in async action (sync would be even worse). However, we can try to reduce the delay by using better API, cache, etc.
Default cache time is 1 day.
If there's something wrong with changes, let me know.

Checklist:

  • I have reviewed my code thoroughly.
  • I have tested my code. (I recommend testing it again, it's a big update so I might have skipped something)
  • My changes generate no new warnings

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good! This is just a brief review, I'll do a full review and test when I can. Also you should try to not edit the formatting of the files, I see that there's a lot of files with just formatting changes.

@Blyrex
Copy link

Blyrex commented Jan 10, 2022

its not a good idea to use private apis in this project

@sadcenter
Copy link
Contributor Author

its not a good idea to use private apis in this project

I personally trust this API. I haven't ever met with a situation when this API is down (I'm using this in every project that needs data from Mojang). But not everyone has to trust it, that's why I added an option in the configuration to disable that.
IMO it would be great to add an option that allows you to toggle between ashcon API and Mojang's API

@ytnoos
Copy link
Contributor

ytnoos commented Jan 19, 2022

its not a good idea to use private apis in this project

I personally trust this API. I haven't ever met with a situation when this API is down (I'm using this in every project that needs data from Mojang). But not everyone has to trust it, that's why I added an option in the configuration to disable that.

IMO it would be great to add an option that allows you to toggle between ashcon API and Mojang's API

I think it would be good to automatically switch to Mojang's API if any exception is caught with ashcon API

@CyberFlameGO
Copy link
Contributor

its not a good idea to use private apis in this project

The API seems open source (as declared on the issue init message) and claims to be hosted on Cloudflare Workers

@CyberFlameGO
Copy link
Contributor

Oh also the person running and maintaining the API works at Cloudflare

@sadcenter sadcenter marked this pull request as draft January 19, 2022 13:00
@sadcenter sadcenter marked this pull request as ready for review January 19, 2022 20:42
public static boolean alwaysUseMojang;

private static void alwaysUseMojang() {
alwaysUseMojang = getBoolean("settings.authenticator.always-use-mojang", true);
Copy link

Choose a reason for hiding this comment

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

Imo this should be inverted and be use-ashcon-api

@Sculas
Copy link
Collaborator

Sculas commented Jan 19, 2022

Looks good! Awesome PR :)
Since some reviews are not yet resolved, I'll wait until this is fully ready. Since I do not look at this repo often because I don't have much time, please ping me if possible so I can merge it :)

@sadcenter
Copy link
Contributor Author

oops Intelij IDEA kinda messed up I believe

@ghost
Copy link

ghost commented Jan 20, 2022

oops Intelij IDEA kinda messed up I believe

Rebasing on master should partially fix it

@sadcenter sadcenter marked this pull request as draft January 20, 2022 00:34
backups = getBoolean("settings.authenticator.backups", true);
uuidMojangPriority = getString("settings.authenticator.uuid-priority", "ashcon")
.equalsIgnoreCase("ashcon");
texturesMojangPriority = getString("settings.authenticator.textures-priority", "mojang")
Copy link

@ghost ghost Jan 21, 2022

Choose a reason for hiding this comment

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

We should use integers booleans to specify the api

@Sculas
Copy link
Collaborator

Sculas commented Feb 15, 2022

Is this PR stale or not?

@HowardZHY
Copy link

is #353 related to this?

@sadcenter
Copy link
Contributor Author

sadcenter commented Feb 20, 2022

is #353 related to this?

it might fix it, but it's unlikely possible since authlib wasn't touched in nacho

@sadcenter
Copy link
Contributor Author

Is this PR stale or not?

yea, I will resolve the conflicts tomorrow (if tram won't hit me or smth)

@sadcenter
Copy link
Contributor Author

CodeQL uses Java 11 where exceptionallyCompose and exceptionallyComposeAsync doesn't exist :/

@sadcenter sadcenter marked this pull request as ready for review March 2, 2022 20:02
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants