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

Add score token to BeginPlaySession() #21588

Merged
merged 3 commits into from
Dec 13, 2022

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Dec 9, 2022

This is a breaking change, and will require an osu-server-spectator build with ppy/osu-server-spectator#153

This will allow osu-server-spectator to lookup the score ID from the score token.

Old Server New Server
Old Client 🟢 🔴
New Client 🔴 🟢

@bdach
Copy link
Collaborator

bdach commented Dec 11, 2022

I'm not entirely sold on the decision of placing the token in SpectatorState like this if I'm honest. The structure is being broadcast to every other connected client. While there may be requisite security checks web-side, broadcasting it like that seems like... an unnecessary risk, I guess? May be just my paranoia talking but I'm a strong believer in defence in depth.

I do realise that not broadcasting it is probably going to be quite annoying (separation of data structures, etc.). You could just blank the token server-side for the time being.

@smoogipoo
Copy link
Contributor Author

smoogipoo commented Dec 12, 2022

Given that #21587 is a breaking change anyway, I've taken the liberty of moving the score token to the BeginPlaySession invocation here, making this PR now also a breaking change.

Now requires ppy/osu-server-spectator#153 for testing/deployment.

@smoogipoo smoogipoo changed the title Add score token to spectator state Add score token to BeginPlaySession() Dec 12, 2022
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Seems simple enough

@bdach bdach enabled auto-merge December 13, 2022 16:15
@bdach bdach disabled auto-merge December 13, 2022 17:19
@bdach bdach merged commit 9d14292 into ppy:master Dec 13, 2022
@smoogipoo smoogipoo deleted the beginplaying-score-token branch September 11, 2023 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants