osu icon indicating copy to clipboard operation
osu copied to clipboard

Add two factor authentication flow

Open peppy opened this issue 2 years ago • 3 comments

Draft as this requires osu-web implementation.

Flows required:

  • Endpoint to perform verification
  • Endpoint to re-request verification code
  • Response from a request (either error or otherwise) to let us know that verification hasn't been performed. ie. consider the case where the user does user/password login then quits the game before 2FA authenticating – to handle this, osu-web needs to let us know that we should show the flow again.

@nanaya is looking into the above requirements

Closes https://github.com/ppy/osu/issues/20590


https://github.com/ppy/osu/assets/191335/1012f75d-7c0b-484d-994a-c1beae88cc79

peppy avatar Nov 16 '23 09:11 peppy

Is proper TOTP also going to be considered in the future? I feel like it could be more convenient for a lot of people and is more secure.

AAGaming00 avatar Dec 20 '23 14:12 AAGaming00

Is proper TOTP also going to be considered in the future? I feel like it could be more convenient for a lot of people and is more secure.

https://github.com/ppy/osu-web/issues/5163 searching is not hard

peppy avatar Dec 20 '23 14:12 peppy

Oh I searched the wrong repo oops.

AAGaming00 avatar Dec 20 '23 14:12 AAGaming00

I've pushed some stuff here and the basic flows appear to work. I'm not sure I'm super happy with the code, though.

First of all, the current "notification client" is almost glued to chat in its construction. Like have a look at WebSocketNotificationClient on master. It's half websocket handling logic, half chat specific stuff (things like fetching chat updates in ConnectAsync() via... calling base). I've attempted to basically step over that in https://github.com/ppy/osu/pull/25480/commits/62a0c236bc2eb706bb1f2840e80f7ed7df9c3b78 by splitting out the part I care about (the osu! ws glue code) from the rest but not sure how to feel about adding another layer to that lasagna.

Secondly there's https://github.com/ppy/osu/pull/25480/commits/e3eb7a8b4281c7d3b2170b10b2681cde8ee7f066: turns out that the previous flow of consumption of the notification websocket (a) was half-reliant on the user API state being online, (b) required to request the user's notifications to retrieve the address of the notification websocket endpoint:

https://github.com/ppy/osu/blob/b272d34960e701a844135cd558fab2fb95b5126d/osu.Game/Online/Notifications/WebSocket/WebSocketNotificationsClientConnector.cs#L30-L35

I'm not sure that's going to work if the user's session isn't verified, and it also seems turbo weird to do this. So I just hardcoded the websocket address. I'll likely want to ask the web team as to whether this is anywhere near acceptable.

There's also the part where any failure to set up the websocket listener basically is met with "welp, there was an attempt, enter the code manually or go away". Not sure how much I should care about that.

Tests will probably fail. And there are no new tests exercising the new logic. I'll come back to that tomorrow.

bdach avatar Jan 24 '24 21:01 bdach

the main idea for /endpoint is to avoid hardcoding the url in web (web build/deployment constraints etc). Whether or not it should be hardcoded in client would be just deployment consideration (both for client and notification server).

There's no particular reason why it requires authentication apart of there's no exclusion for it from requiring one and that it's not usable without authentication anyway.

nanaya avatar Jan 25 '24 06:01 nanaya

I think I'm okay with where this is at right now, but this is dependent on https://github.com/ppy/osu/pull/26724 now.

bdach avatar Jan 26 '24 10:01 bdach

Seems to work well. @bdach I've deployed the required changes to staging if you want to give it a final test, but I think this is good to go from my end.

peppy avatar Jan 29 '24 08:01 peppy