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)
• 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

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• 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.

• 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.

[ReenigneArcher]

In my opinion the client list and the ability to remove them should be in the troubleshooting section, near the unpair all clients option.

The pin/pair page should be lightweight.

[xanderfrangos]

@ReenigneArcher Moved to Troubleshooting 👍

[ReenigneArcher]

Should this be optional?

[xanderfrangos]

Your call. It wasn't required before, so I didn't want to enforce it. If it's going to be required, I think the field name should better indicate that they're coming up with a name for the device. Maybe "Name this device" or "Set device name"?

[ReenigneArcher]

I think it's important to have an identifiable name as user's aren't going to be able to differentiate based on a uuid.

Ideally we could also get the client type during the pair process, but I'm not sure that's possible at the moment.

I think "Device Name" is okay to use as the name of the field.

[xanderfrangos]

It might also be nice to track/display when the client last connected and if there's an active session. But that seems like another PR for another day.

For now I'll make the field required and labeled "Device Name".

[ReenigneArcher]

Agreed!

I'd like to see these items tracked in the future.

• Last known IP address
• Last known network type (LAN or WAN)
• Moonlight Client (e.g. Android, QT, iOS, etc.)
• Last known moonlight version
• Device Name (provided by Moonlight, not sure if feasible... but e.g. Nvidia Shield)
• Last session start and end times
• Last session application launched

[cgutman]

I don't love the idea of making device name required. I'd much rather feed that info into the pairing process from the Moonlight side and allow the user to optionally adjust the name via this UI.

On the Moonlight side, it should be pretty easy to send the client's hostname across during the pairing process. We will run into some issues related to the privacy restrictions on iOS, but that's why we keep the optional field around as an override.

[ReenigneArcher]

If Moonlight can send over enough details, then the name is probably not even necessary.

This is what Plex does. None of these fields can be modified, they are all collected from the client, and everything is pretty easy to identify.

The only issue I see is some of these can change after the pairing process. i.e. the client version (and IP address if we're going to track that somehow).

[xanderfrangos]

It may still be helpful to optionally set a client name during pairing.

I suspect most users don't name their devices at the OS level, which can make the Moonlight-provided name somewhat meaningless without more info. Tracking clients this way could be especially difficult if you're using Sunshine to share your PC with someone else (remote "local" co-op, for example). Personally, I would rather set the client name myself than explain to a friend that they need to set a name in Moonlight (or worse, their device's hostname).

Also, I'd expect (well, "hope" lol) that this PR will get approved before the required additions to Moonlight are implemented. So a name field is still needed (optional or not) for now.

[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 (#2042 (comment)) 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 #2365. Individually unpaired devices can no longer start or resume a session. This should resolve #2042 (comment).

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]

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 ⚠️
src/confighttp.cpp	0.00%	15 Missing and 24 partials ⚠️

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%)	⬇️
Windows	2.61% <0.00%> (-0.02%)	⬇️
macOS-12	8.01% <1.70%> (-0.06%)	⬇️
macOS-13	7.91% <0.86%> (-0.07%)	⬇️
macOS-14	8.21% <0.86%> (-0.06%)	⬇️

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%)	⬇️
src/nvhttp.cpp	1.49% <2.46%> (+0.36%)	⬆️

... 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.

[ReenigneArcher]

Suggested change

	import i18nLocale from './locale.js'
	import i18n from './locale.js'

I'm confused why this was changed? I'm not seeing an i18nLocale in locale.js. Maybe it was just a bad copy pasta?

[xanderfrangos]

Something changed with the localization library since I last worked on the PIN page months ago. The "success"/"failure" messages after pairing were throwing errors.

The i18n import was previously an object with the function i18n.global.t (see line 53 for example). However, that i18n import is now itself an async function that returns the expected object (containing global.t). So I needed to await the import to get the object. This meant that I either needed to rename the import or rename the returned object. I decided to rename the import (from i18n to i18nLocale) instead of having a new name for the returned object.

It technically works fine as-is. locale.js uses the async function as the default export, so it doesn't matter what we call the imported variable. If you prefer the import still be called i18n, we can rename the returned object instead of the import. Or if there's a better way to access the localization library, we can do that.

[ReenigneArcher]

@Hazer do you have any thoughts on this?

[Hazer]

This way you're loading the locale twice, it does some API request behind, so it's not ideal.

When I created the initApp method, I already provided the i18n for injection, so you can inject it there

[Hazer]

My suggestion rewriting like this:

<script type="module">
  import { createApp } from 'vue'
  import { initApp } from './init'
  import Navbar from './Navbar.vue'

  let app = createApp({
    components: {
      Navbar
    },
    inject: ['i18n'],
    methods: {
      registerDevice(e) {
        let pin = document.querySelector("#pin-input").value;
        let name = document.querySelector("#name-input").value;
        document.querySelector("#status").innerHTML = "";
        let b = JSON.stringify({pin: pin, name: name});
        fetch("/api/pin", {method: "POST", body: b})
          .then((response) => response.json())
          .then((response) => {
            if (response.status.toString().toLowerCase() === "true") {
              document.querySelector(
                "#status"
              ).innerHTML = `<div class="alert alert-success" role="alert">${this.i18n.t('pin.pair_success')}</div>`;
              document.querySelector("#pin-input").value = "";
              document.querySelector("#name-input").value = "";
            } else {
              document.querySelector(
                "#status"
              ).innerHTML = `<div class="alert alert-danger" role="alert">${this.i18n.t('pin.pair_failure')}</div>`;
            }
          });
      }
    }
  });

  initApp(app);
</script>

This way you can inject i18n easily, and also make a bit clearer what is this code, without changing much.

[Hazer]

Or, in case you're a bit adventurous, replace the whole pin.html with:

<!DOCTYPE html>
<html lang="en" data-bs-theme="auto">

<head>
  <%- header %>
</head>

<body id="app" v-cloak>
  <Navbar></Navbar>
  <div id="content" class="container">
    <h1 class="my-4 text-center">{{ $t('pin.pin_pairing') }}</h1>
    <pin-card></pin-card>
  </div>
</body>

<script type="module">
  import { createApp } from 'vue'
  import { initApp } from './init'
  import Navbar from './Navbar.vue'
  import PinCard from './PinCard.vue'

  let app = createApp({
    components: {
      Navbar,
      PinCard
    }
  });

  initApp(app);
</script>

and create a new file PinCard.vue:

<script setup>
import { ref } from 'vue'

const inputPIN = ref('')
const inputName = ref('')
const status = ref()

function registerDevice() {
  let b = JSON.stringify({pin: inputPIN.value, name: inputName.value});
  console.log(b)
  fetch("/api/pin", {method: "POST", body: b})
    .then((response) => response.json())
    .then((response) => {
      if (response.status.toString().toLowerCase() === "true") {
        status.value = true
        inputPIN.value = ""
        inputName.value = ""
      } else {
        status.value = false
      }
    });
}
</script>

<template>
  <form class="form d-flex flex-column align-items-center" id="form" @submit.prevent="registerDevice">
    <div class="card flex-column d-flex p-4 mb-4">
      <input
        type="text"
        pattern="\d*"
        v-model.trim="inputPIN"
        :placeholder="`${$t('navbar.pin')}`"
        id="pin-input"
        class="form-control mt-2"
        autofocus
        required />
      <input
        type="text"
        v-model.trim="inputName"
        :placeholder="`${$t('pin.device_name')}`"
        id="name-input"
        class="form-control my-4"
        required />
      <button class="btn btn-primary">{{ $t('pin.send') }}</button>
    </div>
    <div class="alert alert-warning">
      <b>{{ $t('_common.warning') }}</b> {{ $t('pin.warning_msg') }}
    </div>
    <div id="status" v-if="status !== undefined">
      <div class="alert alert-success" role="alert" v-if="status === true">{{ $t('pin.pair_success') }}</div>
      <div class="alert alert-danger" role="alert" v-else>{{ $t('pin.pair_failure') }}</div>
    </div>
  </form>
</template>

<style scoped>

</style>

[xanderfrangos]

@ReenigneArcher I've updated it to @Hazer's code from the latest comment. 👍

[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.

[Hazer]

My suggestion rewriting like this:

<script type="module">
  import { createApp } from 'vue'
  import { initApp } from './init'
  import Navbar from './Navbar.vue'

  let app = createApp({
    components: {
      Navbar
    },
    inject: ['i18n'],
    methods: {
      registerDevice(e) {
        let pin = document.querySelector("#pin-input").value;
        let name = document.querySelector("#name-input").value;
        document.querySelector("#status").innerHTML = "";
        let b = JSON.stringify({pin: pin, name: name});
        fetch("/api/pin", {method: "POST", body: b})
          .then((response) => response.json())
          .then((response) => {
            if (response.status.toString().toLowerCase() === "true") {
              document.querySelector(
                "#status"
              ).innerHTML = `<div class="alert alert-success" role="alert">${this.i18n.t('pin.pair_success')}</div>`;
              document.querySelector("#pin-input").value = "";
              document.querySelector("#name-input").value = "";
            } else {
              document.querySelector(
                "#status"
              ).innerHTML = `<div class="alert alert-danger" role="alert">${this.i18n.t('pin.pair_failure')}</div>`;
            }
          });
      }
    }
  });

  initApp(app);
</script>

This way you can inject i18n easily, and also make a bit clearer what is this code, without changing much.

[Hazer]

With the change proposed above, use this:

Suggested change

	<form action="" class="form d-flex flex-column align-items-center" id="form">
	<form class="form d-flex flex-column align-items-center" id="form" @submit.prevent="registerDevice">

[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.