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

ALL: Add Cloud storage support #788

Merged
merged 437 commits into from Aug 30, 2016

Conversation

Projects
None yet
7 participants
@Tkachov
Contributor

Tkachov commented Jul 21, 2016

This adds the Cloud storages support feature. Four providers supported: Dropbox, OneDrive, Google Drive and Box. These are controlled by CloudManager, which could be accessed as CloudMan singleton. CloudMan saves the information in scummvm.ini within "cloud" section.

Storages use OAuth2 and interact with providers' API by giving the access_token. OneDrive, Google Drive and Box has to refresh token from time to time. To send the request, libcurl is used. There is a small system of Requests built on top of it. Requests are managed by ConnectionManager, which works in a separate thread (implemented with Timer) and keeps them running. If there are no more Requests, ConnectionManager stops the timer. While Storage is working (i.e. Requests are running), a pulsating Cloud icon is shown in the corner of the screen.

Cloud icon

Cloud-related information is available at the new Cloud tab of the Options dialog. One can switch connected storages there, refresh the information about these or connect a new one. The main features of the Cloud support are automatic saves sync (starts on each manual save, autosave, startup and on SaveLoadDialog opening) and games download (could be used from the Cloud tab). Before downloading ScummVM checks whether connection is "limited" and notifies user about that. Networking::Connection::isLimited() has only Android backend yet, where it checks whether Wi-Fi or a mobile network is used.

Options dialog Cloud tab

The Wi-Fi sharing feature is also added. ScummVM now can start a local webserver (using SDL_Net), which provides the Files Manager: users can navigate through directories, download files, create new directories or upload files using the browser on another device. Information about the server is available on the same Cloud tab. Users could change the webserver's port there.

AJAX Files Manager
LocalWebserver's port in Cloud tab

To connect a Storage, users must navigate to the link we show on the screen, allow our app access to their account, and receive the code which would be used by ScummVM to get the access_token. Some features were added in order to simplify that. First of all, we show a special short scummvm.org link. Secondly, there is a special "Open URL" button, which opens that link in a default browser, if the corresponding Networking::Browser::openUrl() backend is implemented.

Storage Connection Dialog: "Open URL" button, 8 fields

The code is quite long, so scummvm.org page transforms it into a few short groups with hashsum added. ScummVM dialog shows 8 fields, where user should type these groups, and checks that hashsum is OK. Then I also added SDL2 clipboard support in EditableWidget, so now users could paste these short groups into these fields.

scummvm.org code page

Finally, if there is LocalWebserver available, a bit different link is shown and cloud providers redirect user to the localhost:12345 page instead of scummvm.org. This way ScummVM can get the code without user typing it in the fields, which makes connecting a Storage extremely simple.

Storage Connection Dialog: no fields


It lacks iOS implementations of Networking::Browser::openUrl() and Networking::Connection::isLimited().

There are some commits from @sev- 's gui-improvement. This code was in my container-box branch while I was working on it, so there are clipping versions for transparent surfaces from gui-improvement. Yet, I had to remove it while preparing PR with Container box, so when I merged that back into my cloud branch, I didn't add those clipping versions back. In short, there are commits for transparent pictures, but those don't get clipped.

There is no code overriding loading Storage's KEY and SECRET from scummvm.ini.

All the short links are redirecting to giving access to my test applications. Actual ScummVM applications must be created with their real keys and secrets, and links must be edited to redirect to these.

Some TODOs are still in the code, I'm working on these.

@Tkachov Tkachov changed the title from Add Cloud storages support to ALL: Add Cloud storages support Jul 21, 2016

@Tkachov Tkachov changed the title from ALL: Add Cloud storages support to ALL: Add Cloud storage support Jul 21, 2016

@peterbozso peterbozso changed the title from ALL: Add Cloud storage support to CLOUD: Add cloud storages support Jul 21, 2016

@Tkachov Tkachov changed the title from CLOUD: Add cloud storages support to ALL: Add Cloud storage support Jul 21, 2016

Show outdated Hide outdated engines/sci/detection.cpp
Show outdated Hide outdated engines/metaengine.h
resolveAddress(&ip);
_serverSocket = SDLNet_TCP_Open(&ip);

This comment has been minimized.

@wjp

wjp Jul 21, 2016

Member

I'm not too familiar with SDLNet specifically, but why not only listen on 127.0.0.1?

@wjp

wjp Jul 21, 2016

Member

I'm not too familiar with SDLNet specifically, but why not only listen on 127.0.0.1?

This comment has been minimized.

@Tkachov

Tkachov Jul 21, 2016

Contributor

Well, passing NULL up there means we want to use 0.0.0.0 ("listen on every available network interface").

Address resolving is needed because local webserver (in case of Wi-Fi sharing feature) is meant to be accessed from another device on the same network, thus actual local IP is required instead of plain 127.0.0.1.

@Tkachov

Tkachov Jul 21, 2016

Contributor

Well, passing NULL up there means we want to use 0.0.0.0 ("listen on every available network interface").

Address resolving is needed because local webserver (in case of Wi-Fi sharing feature) is meant to be accessed from another device on the same network, thus actual local IP is required instead of plain 127.0.0.1.

This comment has been minimized.

@wjp

wjp Jul 21, 2016

Member

What is a wifi sharing feature, why do we need it, and where I can find information about your use of it?

@wjp

wjp Jul 21, 2016

Member

What is a wifi sharing feature, why do we need it, and where I can find information about your use of it?

This comment has been minimized.

@sev-

sev- Jul 21, 2016

Member

This is a local webserver running on top of ScummVM, so a user can open the provided URL in their local network and have direct access to the device filesystem. Then they can upload the game files or download saves from their device running ScummVM.

This is an alternative to the external cloud usage.

@sev-

sev- Jul 21, 2016

Member

This is a local webserver running on top of ScummVM, so a user can open the provided URL in their local network and have direct access to the device filesystem. Then they can upload the game files or download saves from their device running ScummVM.

This is an alternative to the external cloud usage.

This comment has been minimized.

@Tkachov

Tkachov Jul 21, 2016

Contributor

Some information about it is in the PR description and in my blog.

We need it because it's cool and some other apps - for example, VLC - have such feature.

Almost all "handlers" in the backends/networking/sdl_net/ are implementing this feature.

In short, it allows one to access files via network - upload and download files from the device ScummVM is working on. For example, one can upload a game from a PC onto a smartphone without USB cable.

@Tkachov

Tkachov Jul 21, 2016

Contributor

Some information about it is in the PR description and in my blog.

We need it because it's cool and some other apps - for example, VLC - have such feature.

Almost all "handlers" in the backends/networking/sdl_net/ are implementing this feature.

In short, it allows one to access files via network - upload and download files from the device ScummVM is working on. For example, one can upload a game from a PC onto a smartphone without USB cable.

This comment has been minimized.

@wjp

wjp Jul 21, 2016

Member

That has the unfortunate side effect of opening a large attack surface unnecessarily when not using this feature.

@wjp

wjp Jul 21, 2016

Member

That has the unfortunate side effect of opening a large attack surface unnecessarily when not using this feature.

This comment has been minimized.

@Tkachov

Tkachov Jul 21, 2016

Contributor

Webserver has to be turned on manually with a special button in the Cloud tab.

It's also turned on automatically while connecting a Storage, but it's turned off when Storage is connected.

As ScummVM isn't working on routers, I doubt someone could access files with that feature through the Internet. If the user is smart enough to configure port forwarding, I guess he/she is smart enough to use that with caution.

If the malefactor is on the same local network... well, user shouldn't have run the server while using a non-trusted network, then.

@Tkachov

Tkachov Jul 21, 2016

Contributor

Webserver has to be turned on manually with a special button in the Cloud tab.

It's also turned on automatically while connecting a Storage, but it's turned off when Storage is connected.

As ScummVM isn't working on routers, I doubt someone could access files with that feature through the Internet. If the user is smart enough to configure port forwarding, I guess he/she is smart enough to use that with caution.

If the malefactor is on the same local network... well, user shouldn't have run the server while using a non-trusted network, then.

This comment has been minimized.

@wjp

wjp Jul 21, 2016

Member

Is that really your attitude towards security?

@wjp

wjp Jul 21, 2016

Member

Is that really your attitude towards security?

This comment has been minimized.

@sev-

sev- Jul 21, 2016

Member

We need to ensure that there is no way to go beyond a sandboxed directory where all game-related files could be stored. Also, ScummVM is not supposed to execute anything directly, all code in the engines so far is using VMs.

My recommendation is to enforce the server close once the dialog with upload files is closed. In this case this will be yet another safety feature.

However, we are using the local server for accepting keys from the local browser (e.g. we provide 127.0.0.1:12345 as a callback URL). In this case I would enforce additional checks on the incoming URL formats. We are using it only for extracting the keys provided in the GET URLs

@sev-

sev- Jul 21, 2016

Member

We need to ensure that there is no way to go beyond a sandboxed directory where all game-related files could be stored. Also, ScummVM is not supposed to execute anything directly, all code in the engines so far is using VMs.

My recommendation is to enforce the server close once the dialog with upload files is closed. In this case this will be yet another safety feature.

However, we are using the local server for accepting keys from the local browser (e.g. we provide 127.0.0.1:12345 as a callback URL). In this case I would enforce additional checks on the incoming URL formats. We are using it only for extracting the keys provided in the GET URLs

This comment has been minimized.

@Tkachov

Tkachov Jul 21, 2016

Contributor

As I mentioned, server is turned off automatically after it's used in the Storage connection. As Eugene said, we can additionally turn off the server when user closes the Options dialog, so it would be available only while user actually works with it.

@Tkachov

Tkachov Jul 21, 2016

Contributor

As I mentioned, server is turned off automatically after it's used in the Storage connection. As Eugene said, we can additionally turn off the server when user closes the Options dialog, so it would be available only while user actually works with it.

@wjp

This comment has been minimized.

Show comment
Hide comment
@wjp

wjp Jul 21, 2016

Member

Given the big impact of this, is there any technical design document? (In particular about the various security considerations.)

Member

wjp commented Jul 21, 2016

Given the big impact of this, is there any technical design document? (In particular about the various security considerations.)

@sev-

This comment has been minimized.

Show comment
Hide comment
@sev-

sev- Jul 21, 2016

Member

Yes, it was highlighted in the Alex's blog. For instance, see here: http://tkachov.ru/gsoc/post_05_28_2016_file_downloading_week

Member

sev- commented Jul 21, 2016

Yes, it was highlighted in the Alex's blog. For instance, see here: http://tkachov.ru/gsoc/post_05_28_2016_file_downloading_week

void BaseBackend::clearOSD() {
warning("BaseBackend::clearOSD not implemented"); //TODO
//what should I do? Remove all TimedMessageDialogs?

This comment has been minimized.

@sev-

sev- Jul 21, 2016

Member

Yes, skip those. Additionally, we could add another backend feature: kFeatureOSD.

@sev-

sev- Jul 21, 2016

Member

Yes, skip those. Additionally, we could add another backend feature: kFeatureOSD.

@bluegr

This comment has been minimized.

Show comment
Hide comment
@bluegr

bluegr Jul 21, 2016

Member

From a quick glance: quite impressive work, overall! Well done! :)

I'll check this out later on

Member

bluegr commented Jul 21, 2016

From a quick glance: quite impressive work, overall! Well done! :)

I'll check this out later on

@wjp

This comment has been minimized.

Show comment
Hide comment
@wjp

wjp Jul 21, 2016

Member

I mean a design document for the whole general cloud functionality. This feature will require long-term maintainability more than most of our current features, and also potentially urgent security fixes
sometimes. I don't think blog posts or a pull request description (while nice) are really sufficient for this.

Member

wjp commented Jul 21, 2016

I mean a design document for the whole general cloud functionality. This feature will require long-term maintainability more than most of our current features, and also potentially urgent security fixes
sometimes. I don't think blog posts or a pull request description (while nice) are really sufficient for this.

request->addPostField("client_id=" + Common::String(KEY));
request->addPostField("client_secret=" + Common::String(SECRET));
/*
if (Cloud::CloudManager::couldUseLocalServer()) {

This comment has been minimized.

@sev-

sev- Jul 21, 2016

Member

Is this working?

@sev-

sev- Jul 21, 2016

Member

Is this working?

This comment has been minimized.

@Tkachov

Tkachov Jul 26, 2016

Contributor

Short links work fine. And it looks like Box doesn't check the redirect_uri parameter when token is requested.

But I just checked Box docs/app editing page... Box allows only one redirect_uri. And localhost option is available for apps in development only. I guess we'd have to use https://www.scummvm.org/c/code option for Box all the time, even if LocalWebserver is available.

@Tkachov

Tkachov Jul 26, 2016

Contributor

Short links work fine. And it looks like Box doesn't check the redirect_uri parameter when token is requested.

But I just checked Box docs/app editing page... Box allows only one redirect_uri. And localhost option is available for apps in development only. I guess we'd have to use https://www.scummvm.org/c/code option for Box all the time, even if LocalWebserver is available.

@Tkachov

This comment has been minimized.

Show comment
Hide comment
@Tkachov

Tkachov Jul 21, 2016

Contributor

Sure, I'd compile the detailed information on a wiki page.

Contributor

Tkachov commented Jul 21, 2016

Sure, I'd compile the detailed information on a wiki page.

@sev-

This comment has been minimized.

Show comment
Hide comment
@sev-

sev- Jul 22, 2016

Member

I just rebased everything to the tip of the master

Member

sev- commented Jul 22, 2016

I just rebased everything to the tip of the master

@Tkachov

This comment has been minimized.

Show comment
Hide comment
@Tkachov

Tkachov Jul 22, 2016

Contributor

I've added a detailed description of Cloud and Local Webserver / Wi-Fi Sharing features to the wiki:
Cloud Storage Support
Local Webserver

Contributor

Tkachov commented Jul 22, 2016

I've added a detailed description of Cloud and Local Webserver / Wi-Fi Sharing features to the wiki:
Cloud Storage Support
Local Webserver

@peterbozso

This comment has been minimized.

Show comment
Hide comment
@peterbozso

peterbozso Jul 22, 2016

Member

I also added the two documentation pages to our Developer Central: http://wiki.scummvm.org/index.php/Developer_Central#Networking

Member

peterbozso commented Jul 22, 2016

I also added the two documentation pages to our Developer Central: http://wiki.scummvm.org/index.php/Developer_Central#Networking

break;
case UFH_READING_BLOCK_CONTENT:
// _contentStream is created by handleBlockHeaders() if needed

This comment has been minimized.

@wjp

wjp Jul 22, 2016

Member

I don't see how you're handling the error case where it isn't created.

@wjp

wjp Jul 22, 2016

Member

I don't see how you're handling the error case where it isn't created.

This comment has been minimized.

@Tkachov

Tkachov Jul 26, 2016

Contributor

Woah, I thought the comment is about new up there and handling a situation when object is not allocated. Well, I'm not handling such situations mostly, because it's difficult to do something when you can't allocate more memory.

As for _contentStream, if it isn't created, it's nullptr. It's passed to Client::readBlockContent(). As mentioned in the wiki (and also in Client's description comment), Client uses Reader internally.

And when you pass nullptr as a stream to Reader, it reads contents into the void (reads, sees there is no stream, does nothing). So, passing nullptr here is similar to > /dev/null in a terminal.

@Tkachov

Tkachov Jul 26, 2016

Contributor

Woah, I thought the comment is about new up there and handling a situation when object is not allocated. Well, I'm not handling such situations mostly, because it's difficult to do something when you can't allocate more memory.

As for _contentStream, if it isn't created, it's nullptr. It's passed to Client::readBlockContent(). As mentioned in the wiki (and also in Client's description comment), Client uses Reader internally.

And when you pass nullptr as a stream to Reader, it reads contents into the void (reads, sees there is no stream, does nothing). So, passing nullptr here is similar to > /dev/null in a terminal.

@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Jul 24, 2016

Member

I have not looked at the code yet, but from the description in this PR I have one suggestion and one question.

The suggestion would be to have a "Paste from Clipboard" button in the tab where we need to enter the access token. From what I understand, the way pasting is implemented currently we would have to do it eight time for each pieces fo the access token? The possibility to paste the full token in one go would be great. Also I am assuming currently pasting is done using a keyboard shortcut? In that case the button would help on devices that do not have a keyboard (it suspect it would at least be easier to implement than the long press usually used to popup the copy/paste menu, although that would be great too :P).

The question is about the local server and how tied it is to SDL Net? Is it implemented in such a way (e.g. with abstraction layers) that it would be easy for porters for platforms that do not use SDL to implement it in a different way?

Member

criezy commented Jul 24, 2016

I have not looked at the code yet, but from the description in this PR I have one suggestion and one question.

The suggestion would be to have a "Paste from Clipboard" button in the tab where we need to enter the access token. From what I understand, the way pasting is implemented currently we would have to do it eight time for each pieces fo the access token? The possibility to paste the full token in one go would be great. Also I am assuming currently pasting is done using a keyboard shortcut? In that case the button would help on devices that do not have a keyboard (it suspect it would at least be easier to implement than the long press usually used to popup the copy/paste menu, although that would be great too :P).

The question is about the local server and how tied it is to SDL Net? Is it implemented in such a way (e.g. with abstraction layers) that it would be easy for porters for platforms that do not use SDL to implement it in a different way?

@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Jul 24, 2016

Member

There are various warnings and compilation errors.

Some examples of warnings:

engines/testbed/cloud.cpp:121:82: warning: format specifies type 'unsigned int' but the argument has type 'const char *' [-Wformat]
                Testsuite::logPrintf("Info! First directory is '%u' and first file is '%s'\n", directory.c_str(), file.c_str());
                                                                ~~                             ^~~~~~~~~~~~~~~~~
                                                                %s
engines/testbed/cloud.cpp:137:95: warning: format specifies type 'unsigned long' but the argument has type 'uint32' (aka 'unsigned int')
      [-Wformat]
        Testsuite::logPrintf("Info! It's id = '%s' and size = '%lu'\n", response.value.id().c_str(), response.value.size());
                                                               ~~~                                   ^~~~~~~~~~~~~~~~~~~~~
                                                               %u
engines/testbed/cloud.cpp:154:95: warning: format specifies type 'unsigned long' but the argument has type 'size_type' (aka 'unsigned int')
      [-Wformat]
                Testsuite::logPrintf("Warning! %lu files were not downloaded during folder downloading!\n", response.value.size());
                                               ~~~                                                          ^~~~~~~~~~~~~~~~~~~~~
                                               %u
./gui/storagewizarddialog.h:49:7: warning: private field '_stopServerOnClose' is not used [-Wunused-private-field]
        bool _stopServerOnClose;
             ^

And some example of errors (disclaimer: I have disabled the use of SDL Net):

gui/options.cpp:1322:28: error: no member named 'LocalWebserver' in namespace 'Networking'
        uint32 port = Networking::LocalWebserver::getPort();
                      ~~~~~~~~~~~~^
gui/storagewizarddialog.cpp:198:7: error: use of undeclared identifier 'kStorageCodePassedCmd'
        case kStorageCodePassedCmd:
             ^
gui/storagewizarddialog.cpp:199:39: error: use of undeclared identifier 'LocalServer'
                CloudMan.connectStorage(_storageId, LocalServer.indexPageHandler().code());
                                                    ^

I am aware this is all code in progress, so I will stop there in error reporting. But they should all be fixed before this PR is merged.

Member

criezy commented Jul 24, 2016

There are various warnings and compilation errors.

Some examples of warnings:

engines/testbed/cloud.cpp:121:82: warning: format specifies type 'unsigned int' but the argument has type 'const char *' [-Wformat]
                Testsuite::logPrintf("Info! First directory is '%u' and first file is '%s'\n", directory.c_str(), file.c_str());
                                                                ~~                             ^~~~~~~~~~~~~~~~~
                                                                %s
engines/testbed/cloud.cpp:137:95: warning: format specifies type 'unsigned long' but the argument has type 'uint32' (aka 'unsigned int')
      [-Wformat]
        Testsuite::logPrintf("Info! It's id = '%s' and size = '%lu'\n", response.value.id().c_str(), response.value.size());
                                                               ~~~                                   ^~~~~~~~~~~~~~~~~~~~~
                                                               %u
engines/testbed/cloud.cpp:154:95: warning: format specifies type 'unsigned long' but the argument has type 'size_type' (aka 'unsigned int')
      [-Wformat]
                Testsuite::logPrintf("Warning! %lu files were not downloaded during folder downloading!\n", response.value.size());
                                               ~~~                                                          ^~~~~~~~~~~~~~~~~~~~~
                                               %u
./gui/storagewizarddialog.h:49:7: warning: private field '_stopServerOnClose' is not used [-Wunused-private-field]
        bool _stopServerOnClose;
             ^

And some example of errors (disclaimer: I have disabled the use of SDL Net):

gui/options.cpp:1322:28: error: no member named 'LocalWebserver' in namespace 'Networking'
        uint32 port = Networking::LocalWebserver::getPort();
                      ~~~~~~~~~~~~^
gui/storagewizarddialog.cpp:198:7: error: use of undeclared identifier 'kStorageCodePassedCmd'
        case kStorageCodePassedCmd:
             ^
gui/storagewizarddialog.cpp:199:39: error: use of undeclared identifier 'LocalServer'
                CloudMan.connectStorage(_storageId, LocalServer.indexPageHandler().code());
                                                    ^

I am aware this is all code in progress, so I will stop there in error reporting. But they should all be fixed before this PR is merged.

@Tkachov

This comment has been minimized.

Show comment
Hide comment
@Tkachov

Tkachov Jul 25, 2016

Contributor

Good suggestion about the "Paste from clipboard" button! Right now it works when built with SDL2, via Ctrl+V shortcut.

LocalWebserver class itself uses SDL_Net sockets (and socket set). Client does too. Other classes interact with these, so these would use sockets and return some result. SDL_Net sockets are not really complex, and I believe could be easily replaced with usual sockets (IIRC there were socket sets of some kind in POSIX). Yet, in order to use other sockets instead of SDL_Net's, we'd have to introduce a few own classes, as I didn't.

I fixed compilation without SDL_Net or libcurl a few times, but I guess I've broken that again. One of the warnings you've posted is really useful (%u for string somehow). I've used a lot of uint64, so I guess I used %lu automatically in the other two. We could avoid the last one, if we'd hide the field with #ifdef, but after that we might end up having an undefined identifier error instead. I'll try to fix these.

Contributor

Tkachov commented Jul 25, 2016

Good suggestion about the "Paste from clipboard" button! Right now it works when built with SDL2, via Ctrl+V shortcut.

LocalWebserver class itself uses SDL_Net sockets (and socket set). Client does too. Other classes interact with these, so these would use sockets and return some result. SDL_Net sockets are not really complex, and I believe could be easily replaced with usual sockets (IIRC there were socket sets of some kind in POSIX). Yet, in order to use other sockets instead of SDL_Net's, we'd have to introduce a few own classes, as I didn't.

I fixed compilation without SDL_Net or libcurl a few times, but I guess I've broken that again. One of the warnings you've posted is really useful (%u for string somehow). I've used a lot of uint64, so I guess I used %lu automatically in the other two. We could avoid the last one, if we'd hide the field with #ifdef, but after that we might end up having an undefined identifier error instead. I'll try to fix these.

Tkachov added a commit to Tkachov/scummvm that referenced this pull request Jul 25, 2016

ALL: Make simpleSaveNames() a MetaEngineFeature
Added it into hasFeature() of all engines which returned `true` in
simpleSaveNames() before.

As mentioned in #788, SCI is not always using simple names, so it
doesn't have such feature now.
@criezy

This comment has been minimized.

Show comment
Hide comment
@criezy

criezy Jul 25, 2016

Member

I had some time to look a bit more at the code. Great work!
However I have one more warning and a couple of remarks/suggestions you might want to discuss with your mentor.

First the warning is:

gui/storagewizarddialog.cpp:50:43: warning: commas at the end of enumerator lists are a C++11 extension [-Wc++11-extensions]
        kStorageWizardContainerReflowCmd = 'SWCr',
                                                 ^

The first remark is about the copy/paste feature. I am thinking that rather than use directly SDL2 in the gui code it might be better to extend OSystem and let backends implement the functionality (with for example SDL2, but not necessarily). For example you might want in OSystem to add:

  • kFeatureClipboard
  • bool hasTextInClipboard
  • String getTextFromClipboard

(and if we need copy at some point we can add a copyTextToClipboard)

The second remark is about the use of CURLOPT_XFERINFOFUNCTION, which was added in curl 7.32 (which is "only" 3 years old :P ). This causes compilation to fails on my system. I suppose I could compile a more recent curl manually and use that, but I am wondering about the possibility to use CURLOPT_PROGRESSFUNCTION rather than CURLOPT_XFERINFOFUNCTION to support a wider range of curl versions and ease the pain for port maintainers.

Member

criezy commented Jul 25, 2016

I had some time to look a bit more at the code. Great work!
However I have one more warning and a couple of remarks/suggestions you might want to discuss with your mentor.

First the warning is:

gui/storagewizarddialog.cpp:50:43: warning: commas at the end of enumerator lists are a C++11 extension [-Wc++11-extensions]
        kStorageWizardContainerReflowCmd = 'SWCr',
                                                 ^

The first remark is about the copy/paste feature. I am thinking that rather than use directly SDL2 in the gui code it might be better to extend OSystem and let backends implement the functionality (with for example SDL2, but not necessarily). For example you might want in OSystem to add:

  • kFeatureClipboard
  • bool hasTextInClipboard
  • String getTextFromClipboard

(and if we need copy at some point we can add a copyTextToClipboard)

The second remark is about the use of CURLOPT_XFERINFOFUNCTION, which was added in curl 7.32 (which is "only" 3 years old :P ). This causes compilation to fails on my system. I suppose I could compile a more recent curl manually and use that, but I am wondering about the possibility to use CURLOPT_PROGRESSFUNCTION rather than CURLOPT_XFERINFOFUNCTION to support a wider range of curl versions and ease the pain for port maintainers.

Tkachov and others added some commits Jul 26, 2016

CLOUD: Update GoogleDriveStorage
More JSON checks in callbacks.
CLOUD: Update GoogleDriveUploadRequest
JSON checks in callback.
CLOUD: Update OneDrive
Added JSON checks.

New jsonContainsObject() method added to CurlJsonRequest.
CLOUD: Update SavesSyncRequest
Add JSON checks in the callback.
CLOUD: Fix Requests
Remove unnecessary JSON warnings, fix a few places.
CLOUD: Add custom User-Agent
Full version is used like in Eugene's Google Analytics stub. Plus, on
PS3 that string contains "PlayStation", and that would be cool to know
that ScummVM+libcurl+PS3 work together.
CLOUD: Fix UploadFileClientHandler
It now redirects user on success not only when file was the last field
in the content, but also when it was uploaded already and Handler worked
further to search for more files.
CLOUD: Use overriden handle() instead of ClientHandlerCallback in pag…
…e handlers

Using a dedicated callback object for this was an unnecessary overhead.
CLOUD: Add "minimal mode" in LocalWebserver
StorageWizardDialog now runs LocalWebserver in "minimal mode" for
security reasons. In this mode server uses only those handlers which
state to support it.

There are two handlers which support minimal mode: IndexPageHandler
(which handles `code` requests needed by StorageWizardDialog) and
ResourceHandler (which provides inner resources like `style.css` or
`logo.png` from `wwwroot.zip` archive).
CLOUD: Handle paths in marked places
Paths containing '../' are forbidden to use in Files Manager. There is
also a special inner black list of paths which are not used and a check
that specified path is under "savepath" or "rootpath" (from "cloud"
domain).
CLOUD: Add GUI for "rootpath" selection
Cloud tab now contains a button to select path, path label and a clear
button.
CLOUD: Update handlers
Now if there is no "rootpath" specified, it's not even listed by
FilesPageHandler and ListAjaxHandler. And, of course, not available to
use anywhere else.
CLOUD: Use forbidden combinations
I accidentally tried "folder../" instead "folder/../" and understood
that I made "folder../" forbidden too, though it's a valid folder name.
CLOUD: Update LocalWebserver
Reader now reads headers into stream, and some checks are added there
and in UploadFileClientHandler, so if headers are too long, they are
treated as bad request.
COMMON: Fix WriteStream::pos() once again
MemoryReadWriteStream now returns int32, not uint32. It actually doesn't
ever return -1 to indicate that an error occured, so uint32 was a better
choice, but that's what is used in WriteStream base class now.

That method is abstract, so that's also why OutSaveFile had to override
it.
@peterbozso

This comment has been minimized.

Show comment
Hide comment
@peterbozso

peterbozso Aug 25, 2016

Member

Sorry for not stating my opinion on this PR sooner! Somehow I assumed since I am the mentor, everybody automatically thought that I reviewed and approved all of the code before advising Alexander to start the PR... :) Obviously it was not the case, sorry for this false assumption.
So in my opinion the code is in a very good shape and ready for merge. I think the documentation both on our wiki and on Alexander's blog is sufficient and thorough and it'll help future developers to start hacking on this part of ScummVM. I also don't have any concerns regarding the security of the local webserver code that haven't been dealt with so far, thanks to the valuable comments above.
If nobody else has any objections (mainly @sev- and @wjp), I think it's time to merge this PR, before it starts to rot.

Member

peterbozso commented Aug 25, 2016

Sorry for not stating my opinion on this PR sooner! Somehow I assumed since I am the mentor, everybody automatically thought that I reviewed and approved all of the code before advising Alexander to start the PR... :) Obviously it was not the case, sorry for this false assumption.
So in my opinion the code is in a very good shape and ready for merge. I think the documentation both on our wiki and on Alexander's blog is sufficient and thorough and it'll help future developers to start hacking on this part of ScummVM. I also don't have any concerns regarding the security of the local webserver code that haven't been dealt with so far, thanks to the valuable comments above.
If nobody else has any objections (mainly @sev- and @wjp), I think it's time to merge this PR, before it starts to rot.

@sev-

This comment has been minimized.

Show comment
Hide comment
@sev-

sev- Aug 30, 2016

Member

Thus, after discussing with @wjp, we're merging this PR.

I do not think it will go into 1.9.0, which will be started soon, but we need to work on it so it ends up in 1.10.0

Tkachov, congratulations!

Member

sev- commented Aug 30, 2016

Thus, after discussing with @wjp, we're merging this PR.

I do not think it will go into 1.9.0, which will be started soon, but we need to work on it so it ends up in 1.10.0

Tkachov, congratulations!

@sev- sev- merged commit bfbfbd3 into scummvm:master Aug 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Tkachov

This comment has been minimized.

Show comment
Hide comment
@Tkachov

Tkachov Aug 30, 2016

Contributor

Hooray! =)

Contributor

Tkachov commented Aug 30, 2016

Hooray! =)

@peterbozso

This comment has been minimized.

Show comment
Hide comment
@peterbozso

peterbozso Aug 30, 2016

Member

Congratulations Alexander, great job! :)

Member

peterbozso commented Aug 30, 2016

Congratulations Alexander, great job! :)

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