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.

Sign up for GitHub

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

Jump to bottom

Move player auth to CServer::ConnectClient #548

Merged
merged 15 commits into from
Nov 22, 2023
Merged

Move player auth to CServer::ConnectClient #548

merged 15 commits into from
Nov 22, 2023

Conversation

catornot
Copy link
Member

@catornot catornot commented Sep 14, 2023 •
edited by GeckoEidechse
Loading

Fixes bots crashing servers when they are the first to connect to it. Also moves player auth to CServer::ConnectClient. This allows the removal of iNextPlayerUid and pNextPlayerToken which were the cause of issues previously since they were not initialized by bots.

I have tested this quickly and it worked for me :)

@catornot catornot added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Sep 14, 2023
@Alystrasz
Copy link
Contributor

Could you provide a detailed test scenario for me to test your PR?

@catornot
Copy link
Member Author

catornot commented Sep 18, 2023 •
edited
Loading

how to test this

  1. try to connect to a local server to test if this still works
  2. spawn bots on a dedicated server before any real players ( crash == doesn't work D: )
    bots plugin: bots_plugin_v2.zip

@Alystrasz
Copy link
Contributor

Gotta check the code of this pull request yet.
I already gave it a try though, and it seems like it's working as intented.

Without this PR

spawn_bot_without_pr

With this PR

spawn_bot_with_pr

@ASpoonPlaysGames ASpoonPlaysGames removed the needs testing Changes from the PR still need to be tested label Oct 21, 2023
ASpoonPlaysGames
ASpoonPlaysGames requested changes Oct 21, 2023
View reviewed changes
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Code mostly looks good, just a couple of things

NorthstarDLL/server/auth/serverauthentication.cpp Outdated Show resolved Hide resolved
NorthstarDLL/server/auth/serverauthentication.cpp Outdated Show resolved Hide resolved
ASpoonPlaysGames
ASpoonPlaysGames approved these changes Oct 22, 2023
View reviewed changes
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Code looks good to me

@ASpoonPlaysGames ASpoonPlaysGames added READY TO MERGE This mergeable right now and removed needs code review Changes from PR still need to be reviewed in code labels Oct 22, 2023
Alystrasz
Alystrasz approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Code looks good, confirmed working in testing.

@GeckoEidechse
Copy link
Member

Merging based on reviews.

@GeckoEidechse GeckoEidechse merged commit 17217a3 into R2Northstar:main Nov 22, 2023
2 checks passed
@catornot catornot deleted the remove_uninit branch November 25, 2023 18:53
@catornot catornot restored the remove_uninit branch November 25, 2023 18:53
@catornot catornot deleted the remove_uninit branch November 25, 2023 18:53
@catornot catornot restored the remove_uninit branch November 25, 2023 18:53
GeckoEidechse added a commit that referenced this pull request Dec 1, 2023
@GeckoEidechse
Revert "Move player auth to CServer::ConnectClient (#548)"
527d824 
This reverts commit 17217a3.
@GeckoEidechse GeckoEidechse mentioned this pull request Dec 1, 2023
Merged
GeckoEidechse added a commit that referenced this pull request Dec 1, 2023
@GeckoEidechse
Revert "Move player auth to CServer::ConnectClient (#548)" (#610)
a27c702 
This reverts commit 17217a3 (PR #548) which introduced a regression allowing auth to progress further than intended.
GeckoEidechse added a commit that referenced this pull request Dec 1, 2023
@GeckoEidechse
Revert "Move player auth to CServer::ConnectClient (#548)" (#610)
9c35f10 
This reverts commit 17217a3 (PR #548) which introduced a regression allowing auth to progress further than intended.

(cherry picked from commit a27c702)
@GeckoEidechse
Copy link
Member

Reverted in #610 as it made it trivial to crash a server by just running connect IP_ADDRESS as it somehow allowed player to bypass the auth check until they reached script where it would script error presumably due to missing pdata stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeckoEidechse GeckoEidechse GeckoEidechse left review comments

@ASpoonPlaysGames ASpoonPlaysGames ASpoonPlaysGames approved these changes

@Alystrasz Alystrasz Alystrasz approved these changes

Assignees
No one assigned
Labels
READY TO MERGE This mergeable right now
Projects
Northstar Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@catornot @Alystrasz @GeckoEidechse @ASpoonPlaysGames