Discord/GitHub account linking #665

Beyley · 2024-09-21T06:06:32Z

This PR implements the basic support for Discord/OAuth2 account linking.

This is another doozy of a PR, so I'll break down the changes which you can skip through.

Most changes in the RefreshTests.GameServer folder is just minor refactoring as I fixed some naming when I moved around some code to Refresh.Common.
There are a couple changes in RefreshTests.GameServer which refactor manual DataContext creation to a helper method which already existed but was just unused for one reason or another. This was spurred on from the fact that more fields were added to DataContext, and without this refactor these tests no longer compiled.

Beyley · 2024-09-21T09:31:09Z

while implementing GitHub OAuth integration, i noticed some deficiencies in the current structure of some bits, so I'm going to mark as draft until I resolve those.

Beyley · 2024-09-22T12:25:49Z

Ready for review again
I didn't intend to include GitHub account linking in this PR initially, but the GitHub code became intertwined with the cleanup work, and it'd be a lot of work to separate, so I figured I'd include it here, since its fully functional.

jvyden

Okay, there's a lot to unpack here.

I'd like some better documentation for the new database types, OAuthRequest and OAuthTokenRelation since those aren't really explained by their names.

I like the idea of abstraction here, but it seems under-utilized (see how there's many Discord config options around the place, but no GitHub config options in other places)

The idea of people with linked accounts causing requests to be sent every single time their GameUsers are serialized is pretty ridiculous, and it makes me wonder if it's even worth having that on the main response at all. At worst, this should be something we cache, and at best this can be pulled in independently via a different API endpoint (/api/v3/users/uuid/{uuid}/links maybe?). I hardly see a use for pulling in Discord/GitHub information from anywhere but the profile, for example.

Overall, I can see the use for Discord OAuth, but I don't get having a GitHub integration at all. It would be helpful if you explained some use-cases for those so we can track all that.

jvyden · 2024-09-22T16:31:02Z

Refresh.GameServer/Configuration/IntegrationConfig.cs

@@ -7,7 +7,7 @@ namespace Refresh.GameServer.Configuration;
 /// </summary>
 public class IntegrationConfig : Config
 {
-    public override int CurrentConfigVersion => 6;
+    public override int CurrentConfigVersion => 9;


No reason to jump this far for a config file

Suggested change

public override int CurrentConfigVersion => 9;

public override int CurrentConfigVersion => 7;

jvyden · 2024-09-22T16:33:27Z

Refresh.GameServer/Configuration/IntegrationConfig.cs

+    /// <summary>
+    /// The redirect URL to use for Discord OAuth requests, ex. `https://lbp.littlebigrefresh.com/api/v3/oauth/authenticate`
+    /// </summary>
+    public string DiscordOAuthRedirectUrl { get; set; } = "http://localhost:10061/api/v3/oauth/authenticate";


Couldn't this be removed in favor of concatenating the endpoint path to ExternalUrl?

jvyden · 2024-09-22T16:33:55Z

Refresh.GameServer/Configuration/IntegrationConfig.cs

+    /// <summary>
+    /// The redirect URL to use for GitHub OAuth requests, ex. `https://lbp.littlebigrefresh.com/api/v3/oauth/authenticate`
+    /// </summary>
+    public string GitHubOAuthRedirectUrl { get; set; } = "http://localhost:10061/api/v3/oauth/authenticate";


jvyden · 2024-09-22T16:36:36Z

Refresh.GameServer/Database/GameDatabaseContext.OAuth.cs

+    public OAuthProvider? OAuthGetProviderForRequest(string state) 
+        => this.OAuthRequests.FirstOrDefault(d => d.State == state)?.Provider;
+
+    public GameUser SaveOAuthToken(string state, OAuth2AccessTokenResponse tokenResponse, IDateTimeProvider timeProvider, OAuthProvider provider)


Create or Add sound better here for database function names

jvyden · 2024-09-22T16:37:09Z

Refresh.GameServer/Database/GameDatabaseContext.OAuth.cs

+
+    public GameUser SaveOAuthToken(string state, OAuth2AccessTokenResponse tokenResponse, IDateTimeProvider timeProvider, OAuthProvider provider)
+    {
+        OAuthRequest request = this.OAuthRequests.First(d => d.State == state);


No sanity check?

jvyden · 2024-09-23T11:35:08Z

Refresh.GameServer/Types/Visibility.cs

+    /// <param name="visibility">The intended visibility of the object</param>
+    /// <param name="visibility">The intended visibility of the object</param>


Do you know who else has dementia?

Suggested change

/// <param name="visibility">The intended visibility of the object</param>

/// <param name="visibility">The intended visibility of the object</param>

/// <param name="visibility">The intended visibility of the object</param>

jvyden · 2024-09-23T11:37:09Z

Refresh.GameServer/Types/Visibility.cs

+
+        switch (visibility)
+        {
+            case Visibility.Game when dataContext.Game != TokenGame.Website:


I think technically we'd want to check for platform?

Suggested change

case Visibility.Game when dataContext.Game != TokenGame.Website:

case Visibility.Game when dataContext.Platform != TokenGame.Website:

jvyden · 2024-09-23T11:37:37Z

Refresh.GameServer/Types/Visibility.cs

+    /// <param name="visibility">The intended visibility of the object</param>
+    /// <param name="visibility">The intended visibility of the object</param>


Do you know who else has dementia?

Suggested change

/// <param name="visibility">The intended visibility of the object</param>

/// <param name="visibility">The intended visibility of the object</param>

/// <param name="visibility">The intended visibility of the object</param>

jvyden · 2024-09-23T11:44:19Z

Refresh.GameServer/Endpoints/ApiV3/DataTypes/Response/Users/ApiGameUserResponse.cs

+            //TODO: this data should be cached
+            DiscordProfileInfo = user.DiscordProfileVisibility.Filter(
+                user,
+                dataContext,
+                ApiDiscordUserResponse.FromOld(dataContext.OAuth
+                    .GetOAuthClient<DiscordOAuthClient>(OAuthProvider.Discord)
+                    ?.GetUserInformation(dataContext.Database, dataContext.TimeProvider, user), dataContext)
+            ),
+            GitHubProfileInfo = user.GitHubProfileVisibility.Filter(
+                user,
+                dataContext,
+                ApiGitHubUserResponse.FromOld(dataContext.OAuth
+                    .GetOAuthClient<GitHubOAuthClient>(OAuthProvider.GitHub)
+                    ?.GetUserInformation(dataContext.Database, dataContext.TimeProvider, user), dataContext)
+            ),


Is that making a request every time a user is serialized?

How do I put this...

No.

jvyden · 2024-09-23T11:46:59Z

Refresh.GameServer/Endpoints/ApiV3/DataTypes/Response/OAuth/GitHub/ApiGitHubUserResponse.cs

+    public required string? Username { get; set; }
+    public required string? Name { get; set; }
+    public required string? ProfileUrl { get; set; }
+    public required string? AvatarUrl { get; set; }


Are all of these actually nullable? That doesn't seem right at all.

Beyley · 2024-09-23T20:52:30Z

Overall, I can see the use for Discord OAuth, but I don't get having a GitHub integration at all. It would be helpful if you explained some use-cases for those so we can track all that.

main thing i wanted github auth for was the Molecule pin, which is when you get a job at media molecule, i wanted to write some code to bump the github API to get the list of GitHub contributors to the LittleBigRefresh org, and automatically give you this pin if you contribute to any of our projects while having your GH account linked

jvyden · 2024-10-16T20:17:20Z

@Beyley Are you still interested in working on this or do you want me to take this over? Haven't heard anything in a while...

Beyley · 2024-10-16T20:51:33Z

@Beyley Are you still interested in working on this or do you want me to take this over? Haven't heard anything in a while...

still interested in working on this, but motivation is all over the place right now, you're free to pick this branch up since idk when i'll have the energy to do so

Beyley added 12 commits September 19, 2024 02:15

wip

ca8b006

Fix PlayersInPodCount

4f01da4

wip2

d9f68d9

renames, refactors, test fixes

a4ffedc

cleanup + api docs

1eb79db

doc fixes

f8e239e

fix string in integrationconfig

717226b

Merge branch 'main' into discord-oauth

3e08962

unfuck rider refactors

f76cecb

fix error types

db67b04

Add ability to change discord profile visibility through API

f77673f

unfuck rider formatting

16dc276

Beyley requested a review from jvyden September 21, 2024 06:06

Beyley marked this pull request as draft September 21, 2024 09:31

Beyley added 2 commits September 22, 2024 05:22

Add GitHub OAuth + return discord avatar URL

f156469

clean up docs

372196e

Beyley changed the title ~~Discord account linking~~ Discord/GitHub account linking Sep 22, 2024

Beyley marked this pull request as ready for review September 22, 2024 12:24

Return banner URL and default avatar when not set

3d51242

jvyden requested changes Sep 23, 2024

View reviewed changes

jvyden marked this pull request as draft November 3, 2024 01:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discord/GitHub account linking #665

Discord/GitHub account linking #665

Beyley commented Sep 21, 2024

Beyley commented Sep 21, 2024

Beyley commented Sep 22, 2024

jvyden left a comment

jvyden Sep 22, 2024

jvyden Sep 22, 2024

jvyden Sep 22, 2024

jvyden Sep 22, 2024

jvyden Sep 22, 2024

jvyden Sep 23, 2024

jvyden Sep 23, 2024

jvyden Sep 23, 2024

jvyden Sep 23, 2024

jvyden Sep 23, 2024

Beyley commented Sep 23, 2024 •

edited

Loading

jvyden commented Oct 16, 2024 •

edited

Loading

Beyley commented Oct 16, 2024

	public override int CurrentConfigVersion => 9;
	public override int CurrentConfigVersion => 7;

		/// <param name="visibility">The intended visibility of the object</param>
		/// <param name="visibility">The intended visibility of the object</param>

	case Visibility.Game when dataContext.Game != TokenGame.Website:
	case Visibility.Game when dataContext.Platform != TokenGame.Website:

Discord/GitHub account linking #665

Are you sure you want to change the base?

Discord/GitHub account linking #665

Conversation

Beyley commented Sep 21, 2024

Beyley commented Sep 21, 2024

Beyley commented Sep 22, 2024

jvyden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

No.

Choose a reason for hiding this comment

Beyley commented Sep 23, 2024 • edited Loading

jvyden commented Oct 16, 2024 • edited Loading

Beyley commented Oct 16, 2024

Beyley commented Sep 23, 2024 •

edited

Loading

jvyden commented Oct 16, 2024 •

edited

Loading