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

Multiple features (clans, leaderboard etc.) not working. #445

Closed
fcaps opened this issue Nov 4, 2023 · 6 comments · Fixed by #523
Closed

Multiple features (clans, leaderboard etc.) not working. #445

fcaps opened this issue Nov 4, 2023 · 6 comments · Fixed by #523
Assignees

Comments

@fcaps
Copy link
Collaborator

fcaps commented Nov 4, 2023

Due some changes (Auth required for the DataController) in the java-api, the leaderboard, clans and other portions are not working anymore.

Issues:

  1. the website calls the java-api to "cache" some of the results here, so we need the client-credentials flow added to the existing hydra-client. -> maybe even create a new client in hydra
  2. there is already a PR refactoring some portions of the code that would simplify the bugfix. (moved api-calls to the node backed - currently some are called directly in the browser

i could work on a bugfix, but don't know if i should fix/finish the other PR, creating a bugfix for that what is worth fixing, or something else bc. you guys already decided to remove it completely.

Since nobody really complained about this issue in 2 weeks, i wonder how often the clan management is used.

@Brutus5000
Copy link
Member

It's even worse than that. Since this app has almost no exception handling, so clan calls make it hard crash.
There is already a OAuth client in Hydra since the API handles registration, password change etc.

Feel free to continue on the existing PR. I don't think anybody minds you finishing it ;)
By the way, I don't recognize your username, please PM me in Discord for Zulip invite if interested.

@JaviTrek
Copy link
Collaborator

JaviTrek commented Nov 6, 2023

@fcaps I'd recommend finishing my PR, afaik, most of the work should be done but I was too lazy to test out everything. I might finish it soon myself since it breaks my heart seeing the website broken.

Let me know if you do start on it because I want to make sure we don't work on the same thing!

@fcaps
Copy link
Collaborator Author

fcaps commented Nov 7, 2023

TL:DR
Someone has to decide on a solution before we can start, i tried my best to show the issues/implications here.

@JaviTrek would be nice if we could break down tasks of the ongoing PR, so working on this together is possible.
As a start splitting the branch in account and clan stuff would be great, so we don't have a big PR with multiple features going on.

When we have a decision about the solution, we could figure it out what base-branch to use (develop or account-management). Creating merge conflicts is surely nothing we wanna have here.

Issue that i analyzed:

Clan List (Link)

depends on a cache file public/js/members/allClans.json that is created/updated via extractor.js to reduce load on the java-api.
The endpoint used by extractor/getAllClans was updated and needs authentication now. Since the extractor.js is running without a Webuser, reusing the current implementation and hydra client is not an option due the missing Webuser (@Brutus5000 correct me if i am wrong about this).
Even if we create a new hydra client with client_credentials_flow, the java-api is not ready for authentications without a faf-user-id (UserID of a real user) due the limitation in the FafAuthenticationConverter.java
where the Integer.parseInt(source.getSubject()); will throw an Error because subject will be "faf-website" and not an integer (UserID). we could update this to fallback to -1 with something like

  private int extractUserId(Jwt source) {
    return NumberUtils.toInt(source.getSubject(), -1);
  }

to make calls to the DataController possible with client_credentials_flow clients, but this would be just a workaround and not the best solution.
There could be a possibility to change the subject for the client in hydra to '-1', but i am not sure about this, since the documentation says the subject is the client-name if called without real Webuser.


Solution 1:

Everything stays the same for the enduser.

  • create a new hydra client (in ory hydra) with client_credentials_flow (or update the existing one to accept client_credentials_flow)
  • update the faf-java-api to allow authentication without an user (machine2machine calls)
  • update the extractor.js to make it able to get a token by client_credentials flow and use it for the calls to the java-api

Downsides:

  • faf-java-api needs an update and i don't know if the solution with the '-1' will be accepted
  • new hydra client needed in ory

Upsides:

  • everything stays the same for the enduser (or at least how it was before the change)

Solution 2:

  • update all clan related routes (show all / show single) to be protected and use the Webuser token to query the java-api.
  • remove getAllClans from extractor.js

Downsides:

  • no cache
  • clans are not publicly available anymore
  • possible long request time for the user
  • needs also some refactoring regarding protected routes an how calls are made

Upsides:

  • no change required in the faf-java-api to allow machine2machine tokens
  • no new hydra client needed
  • reduced complexity due no "cronjob" running to extract clans

Solution 3:

  • remove the list of all clans completely (for now)
  • move the clan-view to be protected and query the java-api with the user-token.

Downsides:

  • no view of all clans possible
  • clan view only for logged in users

RecentUsers / Leaderboard

pretty much the same solutions as in clans:

  1. make it like it was before (with a new client, java-api changes etc.)
  2. protect and show it for logged in users only
  3. remove it

Sorry for the long post^^

@fcaps
Copy link
Collaborator Author

fcaps commented Nov 8, 2023

Update from zulip:
We kinda agreed to change the java-api to accept machine2machine tokens. (Solution 1)
Need some time to think about a concept/technical tasks for this.

@fcaps
Copy link
Collaborator Author

fcaps commented Nov 9, 2023

java-api changes:
FAForever/faf-java-api#793

@fcaps
Copy link
Collaborator Author

fcaps commented Dec 2, 2023

UPDATE:
leaderboard is working again with the new m2m client.

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 a pull request may close this issue.

3 participants