Name and unpair individual clients

[xanderfrangos]

Description

Introduces the option to name clients during PIN pairing, along with a UI to remove individual client certificates. The format of sunshine_state.json was updated to include a named_certs array instead of the previous certs array. The old array is used for migration to the new format on startup.

Here's an example of the new format:

{
    ...
    "root": {
        "uniqueid": "2258BFE6-698A-422C-5F0F-55B49A69C69E",
        "devices": [
            {
                "uniqueid": "0123456789ABCDEF",
                "named_certs": [
                    {
                        "name": "NVIDIA Shield",
                        "cert": "-----BEGIN CERTIFICATE-----\nCERT\n-----END CERTIFICATE-----\n",
                        "uniqueid": "41310952-60C8-1B01-A615-033CC8275E5F"
                    },
                    {
                        "name": "Steam Deck",
                        "cert": "-----BEGIN CERTIFICATE-----\nCERT\n-----END CERTIFICATE-----\n",
                        "uniqueid": "AE113204-32D9-6607-6285-7F3C200440EB"
                    }
                ]
            }
        ]
    }
}

The named_certs array includes a user-defined name and a generated UUID. The UUID allows for the same name to be used multiple times. It also helps target the correct client over the API without exposing the certificate.

The UI for managing clients has been added to the PIN page, under the PIN input. If a client wasn't named during the pairing process, or the certificate was imported from the old format, the client will be listed as "Unknown". After removing a paired client from the new UI, the user will be prompted to restart Sunshine in order to finish removing the certificate.

While working on this, I was made aware of the TheElixZammuto/Sunshine-1/state-revamp fork, which has a similar goal. However, it looks like it's still missing the UI, API, and server code to deal with "unpairing". It also hasn't been updated since September, so it can't merge cleanly as-is. I understand if you prefer to use what that fork over mine, but I figured I would clean up my code and submit it, in case it's useful

Screenshot

Issues Fixed or Closed

https://discord.com/channels/804382334370578482/1197647357030965328

Type of Change

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] Dependency update (updates to dependencies)
• [ ] Documentation update (changes to documentation)
• [ ] Repository update (changes to repository files, e.g. .github/...)

Checklist

• [x] My code follows the style guidelines of this project
• [x] I have performed a self-review of my own code
• [x] I have commented my code, particularly in hard-to-understand areas
• [x] I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch must be updated before it can be merged. You must also Allow edits from maintainers.

• [x] I want maintainers to keep my branch updated

[CLAassistant]

All committers have signed the CLA.

[ReenigneArcher]

@TheElixZammuto any thoughts on using this over what were working on?

[TheElixZammuto]

@TheElixZammuto any thoughts on using this over what were working on?

Just two things:

1. I would rename uniqueid to uuid or a similar name, since uniqueid is a protocol-specific name which can lead to confusion (it's not the same uniqueid as the one the moonlight client sends)
2. I would prefer to have named_certs as a root element instead of inside "devices", which is useless right now since Moonlight always send the same uniqueid

[xanderfrangos]

@TheElixZammuto Easy enough. 👍 Regarding setting it as a "root element", do you mean the root of the JSON object or the root property (that currently contains uniqueid and devices)?

[TheElixZammuto]

My example idea

{
    "root": {
        "uniqueid": "2258BFE6-698A-422C-5F0F-55B49A69C69E",
        "named_devices": [
            {
                "name": "NVIDIA Shield",
                "cert": "-----BEGIN CERTIFICATE-----\nCERT\n-----END CERTIFICATE-----\n",
                "uuid": "41310952-60C8-1B01-A615-033CC8275E5F"
            },
            {
                "name": "Steam Deck",
                "cert": "-----BEGIN CERTIFICATE-----\nCERT\n-----END CERTIFICATE-----\n",
                "uuid": "AE113204-32D9-6607-6285-7F3C200440EB"
            }
        ]
    }
}

[xanderfrangos]

@TheElixZammuto I've updated sunshine_state.json to use the format you've outlined. I've also refactored nvhttp.cpp to no longer map client_ts to uniqueids, since root.devices is gone (and it's always Moonlight's "0123456789ABCDEF" anyway). Only a single client_t is used now.

[xanderfrangos]

@ReenigneArcher Moved to Troubleshooting 👍

[ReenigneArcher]

@xanderfrangos sorry for the delays. Do you have time to resume this? I think it will fix #2305 which is probably a side effect of using a static uniqueID.

[xanderfrangos]

@ReenigneArcher I unfortunately won't be able to work on this much until later this month. But to my understanding, the only outstanding concern (https://github.com/LizardByte/Sunshine/pull/2042#discussion_r1467967479) was something I wasn't sure how to help with anyway.

[xanderfrangos]

@ReenigneArcher I've updated this branch to use the new localization library and (more importantly) utilize https://github.com/LizardByte/Sunshine/pull/2365. Individually unpaired devices can no longer start or resume a session. This should resolve https://github.com/LizardByte/Sunshine/pull/2042#discussion_r1467967479.

Since active sessions aren't automatically disconnected, I've added language to make that clearer for users. Here's an updated screenshot from the Troubleshooting page (taken after unpairing a device):

Also, the unpairing UI now consistently uses the word "device". I think that's a little more user-friendly than "client", but either is fine as long as it's consistent.

[ReenigneArcher]

@xanderfrangos sorry for the delays. This is the next one on our radar. Could you rebase it?

[xanderfrangos]

@ReenigneArcher I've rebased locally, but need to test/check a few things first. Should have it up over the weekend.

[ReenigneArcher]

@xanderfrangos thank you! Also, FYI, in the last 24 hours we changed our default branch back to master. Basically nightly was renamed to master. If you have any problems rebasing, that could be the reason.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 1.66667% with 118 lines in your changes missing coverage. Please review.

Project coverage is 7.01%. Comparing base (287ac4c) to head (6edb4b0). Report is 152 commits behind head on master.

Files	Patch %	Lines
src/nvhttp.cpp	2.46%	36 Missing and 43 partials :warning:
src/confighttp.cpp	0.00%	15 Missing and 24 partials :warning:

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #2042      +/-   ##
=========================================
- Coverage    7.02%   7.01%   -0.02%     
=========================================
  Files          88      88              
  Lines       17900   17987      +87     
  Branches     8514    8579      +65     
=========================================
+ Hits         1258    1261       +3     
- Misses      13924   14091     +167     
+ Partials     2718    2635      -83     

Flag	Coverage Δ
Linux	5.31% <0.00%> (-0.04%)	:arrow_down:
Windows	2.61% <0.00%> (-0.02%)	:arrow_down:
macOS-12	8.01% <1.70%> (-0.06%)	:arrow_down:
macOS-13	7.91% <0.86%> (-0.07%)	:arrow_down:
macOS-14	8.21% <0.86%> (-0.06%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/confighttp.cpp	0.77% <0.00%> (-0.07%)	:arrow_down:
src/nvhttp.cpp	1.49% <2.46%> (+0.36%)	:arrow_up:

... and 26 files with indirect coverage changes

[xanderfrangos]

@ReenigneArcher It got a little messy, so I squashed my changes and rebased onto master. The only new change is fixing localization strings on the PIN page. Everything is now up-to-date and working on my end.

[xanderfrangos]

On a side note: I know you're intentionally using "Pin", but "PIN" is what it should be. They have two different meanings. A "pin" is a small, narrow, sharp object (or the act of "pinning" something in place). A "PIN" is a "personal identification number".

If you're trying to avoid "PIN" in all caps for some reason, you could use "pairing code"/"pairing" instead. But Moonlight already refers to it as a "PIN", so using any other language could confuse users.

[ReenigneArcher]

@xanderfrangos Thanks! This should get merged soon unless we find any last minute things.

I find it easier to try to just have 1 commit and amend it when there are changes. It makes it a lot easier to rebase.

Regarding the PIN, that sounds right. We should probably address that in a separate PR.

[ReenigneArcher]

I don't know if there's a way to handle this without changes to Moonlight, but I did face this small issue.

When starting my testing initially, there was one unnamed device paired. I removed the Sunshine host from the client, and then repaired it. In Sunshine this showed 2 devices, one named and one unnamed.

When I removed the named one, it still had access since it was also the unnamed one.

Once, I removed the unnamed device on Sunshine's side, it was not able to connect.

Otherwise this looks good. I guess we can just recommend that people unpair all if they have any unnamed devices.

[xanderfrangos]

I don't know if there's a way to handle this without changes to Moonlight, but I did face this small issue.

When starting my testing initially, there was one unnamed device paired. I removed the Sunshine host from the client, and then repaired it. In Sunshine this showed 2 devices, one named and one unnamed.

When I removed the named one, it still had access since it was also the unnamed one.

Once, I removed the unnamed device on Sunshine's side, it was not able to connect.

Otherwise this looks good. I guess we can just recommend that people unpair all if they have any unnamed devices.

Yeah, there's not much that can be done about unnamed devices. There's currently no way to figure out which clients/devices they're for and properly name them (or check if they were already paired). But I expect it to be a short-lived issue, since setting a device name for new pairings is now required.

[moi952]

Hi,

Thanks for the dev :) I tried development, and I only see unknown clients, couldn't we rename them as we connect with them?

Because I have a lot of unknown clients and I don't know which clients I'm using and which are old clients (potentially Android emulator, or old OS with the tests I've done)

Thanks

[ReenigneArcher]

It's recommended to unpair all clients one time. Then you can name them as you pair them again.