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

Two factor authentication needs to be implemented #20590

Closed
peppy opened this issue Oct 6, 2022 Discussed in #20589 · 9 comments · Fixed by #25480
Closed

Two factor authentication needs to be implemented #20590

peppy opened this issue Oct 6, 2022 Discussed in #20589 · 9 comments · Fixed by #25480
Assignees
Labels
priority:1 Very important. Feels bad without fix. Affects the majority of users. type:online

Comments

@peppy
Copy link
Member

peppy commented Oct 6, 2022

Is currently skipped in lazer. This was fine back when lazer was read-only, but should be considered a priority now that things have evolved. Was kind of forgotten about.

Discussed in #20589

Originally posted by htmlpaws October 6, 2022
Hello there!

I recently got a new laptop, and after logging in, I realized there is currently no account verification feature like there is on osu!stable. I feel it should be added because without it, random people are able to log into an account that might not be theirs and engage in social functions (chat, score upload, etc.) without any proof it's actually said user, which can be used to target users.

I think additionally in the future there should be a built in GUI for the code which could allow for input to be easier on mobile for example, where sometimes opening another app can fully close out the game, along with not having to open a popup browser which is annoying in general. Another security thing that I wana put in an as idea if support for 2fa apps, as having to use email can be kinda annoying if you get the code lost in a pile of emails or shoved in a random folder.

Thanks again,
@htmlpaws (Ethacc on osu.)

@peppy peppy added type:online priority:1 Very important. Feels bad without fix. Affects the majority of users. labels Oct 6, 2022
@peppy peppy added this to the Solo leaderboards milestone Oct 6, 2022
@gustarmartins
Copy link

Duplicate of #9655.

@BlestLace
Copy link

Didn't peppy open this issue xD

@peppy peppy mentioned this issue Oct 16, 2022
@peppy
Copy link
Member Author

peppy commented Oct 16, 2022

Will keep this one open as metadata is more up-to-date.

@peppy peppy moved this to Needs implementation in Path to osu!(lazer) ranked play Aug 20, 2023
@peppy peppy removed this from the Solo leaderboard persistence milestone Aug 20, 2023
@LiquidPL
Copy link
Contributor

I've looked into this a little, and there's one thing concerning me, mainly that the password grant we're using for auth on osu-web side appears to be disallowed/deprecated in the most recent versions of oauth. With that in mind I have a few questions:

  • Would it be acceptable to implement a separate auth flow in web for lazer? It might be difficult to graft 2FA in a sane way onto password grant auth, especially since we might have limited library support going forward.
  • Does lazer use any of the oauth scopes, or does it have full acccess to the API?

@peppy
Copy link
Member Author

peppy commented Aug 31, 2023

I discussed this with @nanaya in recent times, and his opinion was that we should be using a standard oauth flow. A lot of other apps seem to do that these days (requiring visiting a browser to login) but I'm not sure that's the UX we want to go for.

Either way, I think @nanaya would be able to give you better answers here than I can.

In terms of UX:

  • I'd hope we can keep the current login flow intact.
  • For the 2FA part, the game would show a message while waiting for email 2FA, and poll for completion, similar to web.

There's also consideration of ppy/osu-web#5163, which I think would also have an in-game entry for the code to keep the UX in line with expectations. Not sure if that should come first (probably not since we just want to get things working first).

@LiquidPL
Copy link
Contributor

I agree that we should first aim for stable parity (login -> type in code in osu-web that is), so that's what I'm gonna focus on now, before doing full on TOTP auth for ppy/osu-web#5163.

That said, I might have worded myself poorly in regards to "login flow". I meant to ask specifically about the endpoints that lazer uses to authenticate. If we want to keep the current login UX as is, would it be okay for me to implement a separate login endpoint for lazer in osu-web that's not using oauth, if implementing 2FA with password grant turns out to be too much of a pain?

Speaking of using a standard oauth flow, I think we're probably fine with not using since we're effectively the only consumer of the lazer oauth client, since everybody else who's integrating with the API is already using other flows that do auth via browser anyway.

@peppy
Copy link
Member Author

peppy commented Sep 10, 2023

I think that sound fine, see how it goes. We'll probably want to be securing the lazer auth endpoint with extra checks in the future, so this may be the better path.

@peppy peppy moved this to In Progress in Path to osu!(lazer) ranked play Nov 16, 2023
@peppy peppy self-assigned this Nov 16, 2023
@peppy peppy moved this from In Progress to In Review in Path to osu!(lazer) ranked play Nov 16, 2023
@peppy
Copy link
Member Author

peppy commented Dec 20, 2023

As an update, osu-web pieces are now in place for this. I hope to revisit this week to implement the API part at our end.

ppy/osu-web#10800

@peppy peppy moved this from In Review to In Progress in Path to osu!(lazer) ranked play Dec 21, 2023
@peppy
Copy link
Member Author

peppy commented Dec 27, 2023

Moving out of project. This will happen early next year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:1 Very important. Feels bad without fix. Affects the majority of users. type:online
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants