You wouldn't think this would be an actual thing that can happen to us,
but it is. The most important one by far is `MaximumStatistics`; that
is the root cause behind why stuff like spinner ticks or slider tails
wasn't showing.
On a better day we should probably do cleanup to unify these models
better, but today is not that day.
This is the first half of a change that *may* fix
https://github.com/ppy/osu/issues/26338 (it definitely fixes *one case*
where the issue happens, but I'm not sure if it will cover all of them).
As described in the issue thread, using the `jti` claim from the JWT
used for authorisation seemed like a decent idea. However, upon closer
inspection the scheme falls over badly in a specific scenario where:
1. A client instance connects to spectator server using JWT A.
2. At some point, JWT A expires, and is silently rotated by the game in
exchange for JWT B.
The spectator server knows nothing of this, and continues to only
track JWT A, including the old `jti` claim in said JWT.
3. At some later point, the client's connection to one of the spectator
server hubs drops out. A reconnection is automatically attempted,
*but* it is attempted using JWT B.
The spectator server was not aware of JWT B until now, and said JWT
has a different `jti` claim than the old one, so to the spectator
server, it looks like a completely different client connecting, which
boots the user out of their account.
This PR adds a per-session GUID which is sent in a HTTP header on every
connection attempt to spectator server. This GUID will be used instead
of the `jti` claim in JWTs as a persistent identifier of a single user's
single lazer session, which bypasses the failure scenario described
above.
I don't think any stronger primitive than this is required. As far as I
can tell this is as strong a protection as the JWT was (which is to say,
not *very* strong), and doing this removes a lot of weird complexity
that would be otherwise incurred by attempting to have client ferry all
of its newly issued JWTs to the server so that it can be aware of them.
At this point its primary usage is the daily challenge event feed, but
the leaderboard will be using this too shortly.
Because the playlists results screen that exists in `master` is
hard-coupled to showing the *local user's* best result on a given
playlist by way of hard-coupling itself to the relevant API request,
allowing show of *arbitrary* score by ID requires a whole bunch of
subclassery as things stand. Oh well.
Class naming is... best effort, due to the above.
Regressed in https://github.com/ppy/osu/pull/28399.
To reproduce, enter a playlist that has an item with a rate-changing mod
(rather than create it yourself).
This is happening because `APIRuleset` has `CreateInstance()`
unimplemented:
b4cefe0cc2/osu.Game/Online/API/Requests/Responses/APIBeatmap.cs (L159)
and only triggers when the playlist items in question originate from
web.
This is why it is bad to have interface implementations throw outside of
maybe mock implementations for tests. `CreateInstance()` is a scourge
elsewhere in general, we need way less of it in the codebase (because
while convenient, it's also problematic to implement in online contexts,
and also expensive because reflection).
(Or local date, in the case of non-deployed builds).
Came up when I was looking at https://github.com/ppy/osu-web/pull/11240
and found that we were still hardcoding this.
Thankfully, this *should not* cause issues, since there don't seem to be
any (documented or undocumented) API response version checks for
versions newer than 20220705 in osu-web master.
For clarity and possible debugging needs, the API response version is
also logged.
First of all, this sort of cleanup isn't really the client's
responsibility, and secondly, at the point the client received this
request to disconnect, *none of its requests will be honored anymore*
(currently the only scenario of this if another client has connected
- the server-side concurrency filter will reject this request).
When disconnection is requested, the only valid thing to do with respect
to talking to the server is to stop doing it.
This will be moved server-side in a follow-up change, although I'm not
even strictly sure that's required - I'd like to think signalr would
know to clean up a disconnecting client from all groups they were in.
There are suspicions that the straight 5s retry could have caused a
situation a few days ago for `osu-server-spectator` wherein it was
getting hammered by constant retry requests. This should make that
a little less likely to happen.
Numbers chosen are arbitrary, but mostly follow stable's bancho retry
intervals because why not. Stable also skips the exponential backoff in
case of errors it considers transient, but I decided not to bother for
now.
Starts off from 3 seconds, then ramps up to up to 2 minutes. Added
stagger factor is 25% of duration, either direction. The stagger factor
helps given that if spectator server is dead, each client has three
separate connections to it which it will retry on (one to each hub).
Closes https://github.com/ppy/osu/issues/26835.
I must have not re-tested this correctly after all the refactors...
Basically the issue is that the websocket connection would only come
online when the API state changed to full `Online`. In particular
the connector would not attempt to connect when the API state was
`RequiresSecondFactorAuth`, giving the link-based flow no chance to
actually work.
The change in `WebSocketNotificationsClientConnector` is relevant in
that queueing requests does nothing before the API state changes to full
`Online`. It also cleans up things a bit code-wise so... win?
And yes, this means that the _other_ `PersistentEndpointClientConnector`
implementations (i.e. SignalR connectors) will also come online earlier
after this. Based on previous discussions
(https://github.com/ppy/osu/pull/25480#discussion_r1395566545) I think
this is fine, but if it is _not_ fine, then it can be fixed by exposing
a virtual that lets a connector to decide when to come alive, I guess.
Closes https://github.com/ppy/osu/issues/26824... I think?
Can be reproduced via something like
diff --git a/osu.Game/Online/API/OAuth.cs b/osu.Game/Online/API/OAuth.cs
index 485274f349..e6e93ab4c7 100644
--- a/osu.Game/Online/API/OAuth.cs
+++ b/osu.Game/Online/API/OAuth.cs
@@ -151,6 +151,11 @@ internal string RequestAccessToken()
{
if (!ensureAccessToken()) return null;
+ for (int i = 0; i < 10000; ++i)
+ {
+ _ = Token.Value.AccessToken;
+ }
+
return Token.Value.AccessToken;
}
The cause is `SecondFactorAuthForm` calling `Logout()`, which calls
`OAuth.Clear()`, _while_ the `APIAccess` connect loop is checking if
`authentication.HasValidAccessToken` is true, which happens to
internally check `Token.Value.AccessToken`, which the clearing of
tokens can brutally interrupt.
Fixes this happening on staging:
[network] 2024-01-29 17:48:24 [verbose]: WebSocketNotificationsClientConnector connected!
[network] 2024-01-29 17:48:24 [verbose]: WebSocketNotificationsClientConnector connect attempt failed: Can't use WaitSafely from inside an async operation.
I'm not sure how I ever allowed that `.WaitSafely()` to be there. It
did feel rather dangerous but then I must have forgotten and never
noticed it failing. Which is weird because you'd think that would be
caught by testing that chat isn't working but I'm pretty sure that I
tested that chat *was* indeed working.
Anyway now that entire flow is replaced by something that should
hopefully be somewhat more sane? It has a whole retry flow with logging
now which should be more robust than what was there previously (failing
to start to listen to chat events killing the entire websocket
connection for very little good reason).
