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

Cloud storage #11582

Closed
wants to merge 27 commits into from
Closed

Cloud storage #11582

wants to merge 27 commits into from

Conversation

njordan64
Copy link
Contributor

@njordan64 njordan64 commented Nov 16, 2020

Description

Adds support for syncing the game saves and game states directories with a storage in a cloud provider. Currently supports Google Drive (App Storage) and MS OneDrive (App Storage).

At a high level the following changes were made:

  • Modified the HTTP support in libretro-common
    • Now has a concept of an HTTP request. Build this up before executing the request
    • Now has a concept of an HTTP response. Returned after executing the request. Contains the HTTP status code, headers and body.
    • If debug is enabled, will log the HTTP requests and responses.
  • Added support for REST calls to libretro-common.
    • Executes the REST request and waits for the response in the current thread
  • Added configuration items for cloud storage
  • Added a configuration menu for cloud storage
  • Implemented the core support for cloud storage
  • Implemented support for Google Drive
  • Implemented support for MS OneDrive

HTTP Changes

HTTP Request

Contains the following:

  • URL
  • HTTP Method
  • URL parameters
  • HTTP headers
  • Request body
  • Request body length

Related functions:

  • net_http_request_new - Creates a blank HTTP request
  • net_http_request_set_url - Set the HTTP URL
  • net_http_request_set_method - Set the HTTP method
  • net_http_request_set_url_param - Add or replace an HTTP URL parameter
  • net_http_request_set_header - Add or replace an HTTP header
  • net_http_request_set_body - Set the request body and length
  • net_http_request_free - Free the memory for a request

HTTP Response

Contains the following:

  • HTTP status code
  • Error - whether the request failed or not
  • HTTP headers
  • Response body
  • Response body length

Related functions:

  • net_http_response_get_status - Get the HTTP status
  • net_http_response_is_error - If the request failed
  • net_http_response_get_header_first_value - Get the first value for a header name
  • net_http_response_get_header_values - Get all values for a header name
  • net_http_response_get_data - Get the response body and length
  • net_http_response_release_data - Release the memory for the response body
  • net_http_response_free - Release the memory for the response

How the REST requests are executed

When the cloud storage support is initialized during startup, it will start up a new thread. There is a queue where cloud storage operations (sync and upload) can be pushed to. When the thread is free, it will pull tasks from the queue and execute them. When possible, it will try to avoid overlapping operations.

New Configuration Items

  • cloud_storage_enable - boolean - Cloud storage support is enabled if true
  • cloud_storage_google_client_id - String - The Google client ID to use
  • cloud_storage_google_client_secret - String - The Google client secret to use
  • cloud_storage_google_refresh_token - String - The refresh token. Can be obtained using RetroArch using the cloud storage configuration menu
  • cloud_storage_onedrive_client_id - String - The MS OneDrive client ID to use
  • cloud_storage_onedrive_refresh_token - String - The refresh token. Can be obtained using RetroArch using the cloud storage configuration menu
  • cloud_storage_provider - int - The cloud storage provider to use. 0 is Google Drive, 1 is MS OneDrive.

Configuration Menu For Cloud Storage

In the cloud storage configuration menu, you can choose a provider, set the client ID and client secret (if needed). You can then choose to authorize RetroArch, which will follow the OAuth2 flow for authorization and open the authorization page in your browser. After you have authorized RetroArch, it will update the refresh token configuration and sync files based on your configured game saves and game states directories.

What It Actually Does

Assuming that everything is configured, it will do the following:

  • On startup sync files in your game saves and game states directories with the cloud provider
  • When you switch cores, if the directories are different for game saves or game states, then they are synced with the cloud provider.
  • When you save a game or game state, it is uploaded to the cloud provider.

Limitations

  • If you roll back a game state, it doesn't do anything
  • Only syncs if you have configured a directory for game saves or game states.
  • Does not work with game saves or game states that are in the same directory as the game.

Further Work Needed

  • Add more cloud storage providers
  • Consider if the new thread is needed. I had tried implemented this with task queue, but caused noticeable slowdown if it was uploading/downloading a large file.
  • Consider better ways of providing the client ID/client secret. Perhaps from configure.
  • Handle rolling back game states.
  • Improve the configuration menu, especially when authorizing.
  • Change to use rjson
  • Update the HTTP support to work on files as well, so it can avoid storing large requests and responses in memory.

@lgtm-com
Copy link

lgtm-com bot commented Nov 16, 2020

This pull request introduces 1 alert when merging fe991ce into 0d2b9fe - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero

@inactive123
Copy link
Contributor

inactive123 commented Nov 16, 2020

Hi there, thanks so much for this PR!

I will try getting some eyeballs on this PR so we can review it.

@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2020

This pull request introduces 1 alert when merging 1b670e6 into 0a3306a - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero

@inactive123
Copy link
Contributor

Hi there @njordan64 ,

just letting you know that once @jdgleaver has wrapped up some of the work he is doing right now, he will be going over this PR as well. It's a big PR so it might take us some time to fully review this but rest assured we have all intent in the world to merge this. Thanks a lot for your work and patience, we'll get it over the finish line.

@njordan64
Copy link
Contributor Author

Thanks for the update. It's good to see that there is interest in this feature.

@jdgleaver
Copy link
Contributor

@njordan64 Thank you for the PR - this is very interesting functionality.

Before we continue, however, could I please ask you to fix all the build errors and warnings? I started doing this myself, but there are too many of them. You can see the issues by building as follows:

make clean && make -j3 C89_BUILD=1
make clean && make -j3 CXX_BUILD=1

After that, one immediate thing that needs fixing is the segfault when enabling cloud storage via the menu. I believe this diff is sufficient:

diff --git a/cloud-storage/cloud_storage.c b/cloud-storage/cloud_storage.c
index 8b2074f4d2..4e15a8e348 100644
--- a/cloud-storage/cloud_storage.c
+++ b/cloud-storage/cloud_storage.c
@@ -186,6 +186,9 @@ void cloud_storage_init(void)
    settings_t *settings;
    struct _operation_queue_item *sync_item;
 
+   if (!_shutdown)
+      return;
+
    _providers[0] = cloud_storage_google_create();
    _providers[1] = cloud_storage_onedrive_create();
    _providers[2] = NULL;
@@ -1003,4 +1006,4 @@ bool cloud_storage_need_authorization(void)
 void cloud_storage_authorize(void (*callback)(bool success))
 {
    _get_active_provider()->authorize(callback);
-}
\ No newline at end of file
+}
diff --git a/menu/menu_setting.c b/menu/menu_setting.c
index 79d3680adb..c17734332d 100644
--- a/menu/menu_setting.c
+++ b/menu/menu_setting.c
@@ -126,6 +126,10 @@
 #include "../play_feature_delivery/play_feature_delivery.h"
 #endif
 
+#if defined(HAVE_NETWORKING) && defined(HAVE_CLOUD_STORAGE)
+#include "../cloud-storage/cloud_storage.h"
+#endif
+
 #define _3_SECONDS  3000000
 #define _6_SECONDS  6000000
 #define _9_SECONDS  9000000
@@ -7513,6 +7517,15 @@ static void general_write_handler(rarch_setting_t *setting)
             }
          }
          break;
+#if defined(HAVE_NETWORKING) && defined(HAVE_CLOUD_STORAGE)
+      case MENU_ENUM_LABEL_CLOUD_STORAGE_ENABLE:
+         if (*setting->value.target.boolean)
+            cloud_storage_init();
+         else
+            cloud_storage_shutdown();
+
+         break;
+#endif
       default:
          break;
    }

Following this, could you please finish wiring up the cloud storage configuration menu, because at present it is entirely non-functional - and I can't really test the code properly if I cannot configure anything :)

@njordan64
Copy link
Contributor Author

I'll take a look at your diff and fix up the C89 issues.

@njordan64
Copy link
Contributor Author

njordan64 commented Nov 20, 2020

I was able to start with a configuration file that had nothing configured for cloud-storage and enable then authorize with Google.

These are the steps that I follow.

  1. Go to Google APIs & Services console
  2. Make a new project
  3. Create an OAuth 2.0 credential to use
  4. Get a configuration with the default cloud-storage values using RetroArch
  5. Edit the file to add your client ID and client secret
  6. Use RetroArch to enable cloud-storage
  7. Choose the provider
  8. Authorize

I don't have any idea what is the best way of handling the client ID and client secret at this point. I doubt the average user will create a Google/MS API project, but one is needed to access Google Drive/OneDrive. These settings could be added to the menu, although they feel too low level to me.

Another problem with an API project is that it could be billed for API usage. Unfortunately this means that the RetroArch project can't just provide one even if the client secret could somehow be kept secret.

@lgtm-com
Copy link

lgtm-com bot commented Nov 20, 2020

This pull request introduces 2 alerts when merging 5bcb485 into 8921d31 - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero
  • 1 for No space for zero terminator

@lgtm-com
Copy link

lgtm-com bot commented Nov 20, 2020

This pull request introduces 1 alert when merging b8e47a4 into 8921d31 - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero

@jdgleaver
Copy link
Contributor

@njordan64 Thank for implementing the fixes.

Could you provide a little more information on setting this up? I created an OAuth 2.0 credential, updated the retroarch config, but all I get is this:

Error 400: redirect_uri_mismatch
The redirect URI in the request, http://127.0.0.1:9000/auth_code, does not match the ones authorized for the OAuth client. To update the authorized redirect URIs, visit: https://console.developers.google.com/apis/credentials/oauthclient/${your_client_id}?project=${your_project_number}

I have no idea what I'm supposed to do.

I don't have any idea what is the best way of handling the client ID and client secret at this point. I doubt the average user will create a Google/MS API project, but one is needed to access Google Drive/OneDrive. These settings could be added to the menu, although they feel too low level to me.

Yes, this is going to be a real problem. I suppose the ID could be entered by selecting the JSON credentials file downloaded from the APIs & Services dashboard (i.e. just open an in-menu file browser). But from my experience thus far, this entire configuration procedure will be entirely inaccessible for 99% of our users. I'm not trying to be negative in any way here - just raising the point that all of us need to think about how this will work in practice, and that the user interface really should take priority. I don't have any ideas yet, but will give this some consideration...

@njordan64
Copy link
Contributor Author

Could you provide a little more information on setting this up? I created an OAuth 2.0 credential, updated the retroarch config, but all I get is this:

Error 400: redirect_uri_mismatch
The redirect URI in the request, http://127.0.0.1:9000/auth_code, does not match the ones authorized for the OAuth client. To update the authorized redirect URIs, visit: https://console.developers.google.com/apis/credentials/oauthclient/${your_client_id}?project=${your_project_number}

When you create the credentials, create them as Desktop application credentials. It looks possible that you created them as something else like Web Application.

@njordan64
Copy link
Contributor Author

I don't have any idea what is the best way of handling the client ID and client secret at this point. I doubt the average user will create a Google/MS API project, but one is needed to access Google Drive/OneDrive. These settings could be added to the menu, although they feel too low level to me.

Yes, this is going to be a real problem. I suppose the ID could be entered by selecting the JSON credentials file downloaded from the APIs & Services dashboard (i.e. just open an in-menu file browser). But from my experience thus far, this entire configuration procedure will be entirely inaccessible for 99% of our users. I'm not trying to be negative in any way here - just raising the point that all of us need to think about how this will work in practice, and that the user interface really should take priority. I don't have any ideas yet, but will give this some consideration...

I'll try a few things over the weekend to see if there any other ways we can authenticate with Google and hopefully MS. It may be possible to simplify things for users.

@njordan64
Copy link
Contributor Author

I did some more research on this and we likely are in better shape than I first thought, at least for Google Drive. Google doesn't bill for Google Drive API requests. They make money on Google Drive through the storage. There are API quotas, but those can be increased by submitting requests.

This means that the RetroArch project can provide an API Project (with client ID and secret) and user's use this API project. A Google account will be needed and the API quotas will likely need to be increased.

There are other open source projects that include the client secret. With RClone, they discussed the issue and it would be very similar to RetroArch in this case. Worst case, someone takes the client ID and secret to impersonate RetroArch. This leads to two possibilities:

  1. The user already authorized RetroArch, which only includes access to Google Drive Application Data (which would only be RetroArch application data)
  2. The user has not authorized RetroArch. The RetroArch API project is configured only for Google Drive Application Data. As a result, the impersonating application is not able request wider permissions.

In either case, the impersonating application is only able to read and write the RetroArch application data in the user's Google Drive account.

@njordan64
Copy link
Contributor Author

It looks like MS OneDrive is similar to Google Drive. The API is free to use. MS makes money from the storage. MS OneDrive only requires a client ID, but in reality this is basically the same as Google Drive along with the same issues. MS OneDrive also allows the API project to be configured to a limited set of APIs (just like Google Drive).

@jdgleaver
Copy link
Contributor

This is very encouraging news!

Looking at RClone as an example, I guess what it will be doing internally is the same setup as described here: https://rclone.org/drive/, only with as much autoconfigured as possible. So the authorisation menu can have client ID/secret entries that are blank by default, with a sublabel recommendation that users provide their own for better performance, but otherwise internal RetroArch credentials will be used. This seems highly practical.

Can you update the PR with a proof of concept of this? (Absolutely no hurry, of course - you're in the driving seat here) I'm certain @twinaphex can look into setting up an API Project on RetroArch's behalf.

@njordan64
Copy link
Contributor Author

I'm working on an update that supports default credentials. Should be ready in a day or two.

@jdgleaver
Copy link
Contributor

Excellent!

We very much appreciate your work. And please take all the time you need! A day or two or more - it's all fine. There are no deadlines here :)

@njordan64
Copy link
Contributor Author

Now this supports default client ID/secrets. There are constants in google.c and onedrive.c that can hold the default client ID/secret. I have left them blank for now. If you populate those, it will be possible to choose to use default credentials in the configuration menu. If they are empty, then you cannot choose to use defaults and must provide the client ID/secret.

There were also some issues with reading the authorization request from the browser. I have improved this somewhat, but it probably still needs some work.

@lgtm-com
Copy link

lgtm-com bot commented Nov 26, 2020

This pull request introduces 1 alert when merging 849a3d5 into 4dcc556 - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero

@Ryunam
Copy link
Contributor

Ryunam commented Nov 26, 2020

Thank you so much for working on this, @njordan64. As an enthusiast user who has been relying on Dropbox for syncing their RA save files, I am really excited at the prospect of having this functionality baked in RA and hopefully being able to use it from all sorts of devices that lack Cloud support at an OS level (namely consoles and handhelds).

I would like to share here one feature request. Do you think it's possible to implement cloud sync support also for the "playtime logs" folder and maybe for the screenshots as well? The logs would be particularly nice for maintaining a global per-game playtime count across multiple devices.

I guess that when the infrastructure is laid out fully this should be feasible, but I am no programmer, so I'm voicing this as purely a practical suggestion based on my personal use case. Looking forward to seeing how this pans out.

@jdgleaver
Copy link
Contributor

@njordan64 Thank you for these new additions! Much appreciated :)

We're currently a bit snowed under with the transition to the new build server, but we'll start testing this as soon as possible.

@twinaphex We're going to need default client ID/secrets for Google Drive and OneDrive, so this will need to be done under 'official' RetroArch accounts. Could you maybe look into this at some point...? (I don't think anyone else has the authority...)

@njordan64
Copy link
Contributor Author

Thank you so much for working on this, @njordan64. As an enthusiast user who has been relying on Dropbox for syncing their RA save files, I am really excited at the prospect of having this functionality baked in RA and hopefully being able to use it from all sorts of devices that lack Cloud support at an OS level (namely consoles and handhelds).

I would like to share here one feature request. Do you think it's possible to implement cloud sync support also for the "playtime logs" folder and maybe for the screenshots as well? The logs would be particularly nice for maintaining a global per-game playtime count across multiple devices.

I guess that when the infrastructure is laid out fully this should be feasible, but I am no programmer, so I'm voicing this as purely a practical suggestion based on my personal use case. Looking forward to seeing how this pans out.

I'll take a look at including the playtime logs and screenshots folders.

On devices without a browser, this will get a little ugly thanks to OAuth 2.0. Everything aside from the authorization flow would work fine. Basically RetroArch could only display a URL that you need to open in a browser on a different device and then enter the refresh code into RetroArch. The URL is probably around 200 characters long (a URL shortener or QR code could help) and the refresh token is about 100 characters long. Could you see yourself entering the refresh code on a device like a Switch or PSP?

@lgtm-com
Copy link

lgtm-com bot commented Nov 27, 2020

This pull request introduces 1 alert when merging 743a8f6 into fe430fd - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero

@lgtm-com
Copy link

lgtm-com bot commented Nov 30, 2020

This pull request introduces 1 alert when merging 5efaad2 into cf854fc - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero

@QuaiGoner
Copy link

Is there any possibility on local SMB sync feature using this as a basse?

@njordan64
Copy link
Contributor Author

Is there any possibility on local SMB sync feature using this as a basse?

SMB is wildly different from REST APIs. Currently I don't see SMB fitting in with the cloud-storage syncing. There are ways of syncing over SMB using features within your operating system.

If there is a lot of desire for SMB support, then it could be explored. I'd like to wait until cloud-storage support is merged in and stable first though.

@lgtm-com
Copy link

lgtm-com bot commented Nov 30, 2020

This pull request introduces 1 alert when merging d6ee556 into 4d2eb18 - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero

@lgtm-com
Copy link

lgtm-com bot commented Dec 1, 2020

This pull request introduces 1 alert when merging 79d5a3f into 4d2eb18 - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero

@lgtm-com
Copy link

lgtm-com bot commented Dec 1, 2020

This pull request introduces 1 alert when merging 64366c3 into 34190cc - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero

…cify credentials, or use defaults. Added support for default credentials. Fixed up some issues with authorization.
…ors in sending HTTP data from a file. Fixed up a null pointer error when creating a remote folder.
Fixed up issues with freeing already freed memory.
Added settings for using default credentials.
@lgtm-com
Copy link

lgtm-com bot commented Jan 3, 2021

This pull request introduces 1 alert when merging 2c6f5c5 into a967d22 - view on LGTM.com

new alerts:

  • 1 for Unsigned comparison to zero

@njordan64
Copy link
Contributor Author

This is likely quite stale at this point. I'll rebase in case anyone wants to test it out.

@gouchi
Copy link
Member

gouchi commented May 13, 2021

@njordan64
Copy link
Contributor Author

This patch is unworkable.

@njordan64 njordan64 closed this Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants