-
Notifications
You must be signed in to change notification settings - Fork 884
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
allow for alternate oauth server to work #5099
Conversation
If I remember correctly, it is a result of this test: And the fact that Jottacloud does have a hard coded client id, here: This means Jottacloud will be included by default when configuring the service, while other providers you need to explicitly enable by inserting a client id/secret. |
Seems the one that was set up for Jottacloud testing is also still available: (see: https://forum.duplicati.com/t/jottacloud-error-401-unauthorized/3929/106) |
FYI: I have written a new OAuth server that is 100% C#. I hope to get it up soon, and hopefully it will help fix this issue (or at least allow debugging the server part). I am still missing a migration path for existing users with active tokens. |
Ah I see, the token is not generated by the server, it's done directly by the user in the Jottacloud Web UI and pasted in the Duplicati OAuth server. I inputted this server address with my Duplicati test setup (including the PR change) and was able to do a backup, interesting test, thanks.
I don't quite understand here, when you floated this idea in private message a few months ago, you talked about a server to install on the user's computer, if it's still the case it just don't see how it could be feasible to extract the true refresh token out of the Google datastore to repatriate it on each user's computer ? Or are you now planning to run it in GAE too, just replacing the existing Python 2 one ? In this case there would be no problem since both servers (old Python2 and new C#) could run in parallel for the migration duration, sharing the database. BTW this is making me think that for people wanting to replace the official Duplicati server by their own home-grown OAuth server, new tokens have to be generated for existing backups. That's obvious only when knowing how the thing works. |
The idea with the C# OAuth server is that it is not ties to GAE so anyone can use it as they see fit. I will set up a shared hosted version similar to the current GAE for those who just want it to work with minimum hassle. Generally, the OAuth tokens use a secret value that is ties to the account hosting the OAuth integration (e.g., the user registering on box.com). The generated token does bot work without this secret value, so the tokens Duplicati use, cannot be shared between servers. This setup allows the provider (e.g., box.com) to close access if an account is compromised. For the migration path I mentioned, I will be using the same accounts (and same secrets) for both the GAE and the new hosted C# version. In this particular case it will be possible to re-use tokens, once I have a good plan for it. Eventually, the GAE solution will shut down, as Python 2 is no longer supported anywhere. |
the alternate server duplicatipy3.ew.r.appspot.com is running under Python 3. |
Nice! Is the updated source available? Then I can perhaps update the running instance and add migration support. |
it was not, I have uploaded it here just now: You'll see quite clearly why I did not make it public, I was afraid that naive people would run with it while it's clearly not ready for prime time (that is, first it has outdated libraries, primarily the obsolete webapp2, second it has not been updated for outdated Js libraries, third it would need to be tested on more code path that a straight backup test with box.com...) I could submit it as a PR for duplicati oauth server but it's unclear if you want to continue to still manage this repo since you did not consider last submitted pull requests. |
@gpatel-fr I have committed the first version here: https://github.com/duplicati/oauth-server
I see. Too bad, I was hoping I could just update. |
I have not said my last word on this server. @albertony What is needed for this Python3 server is to
About the migration, as far as I remember, there is a GAE feature that allows to make run a Python 3 server along a Python 2 server with the same URL, the load balancer redirecting a parameterized % of queries toward the new or old server. Since I have begun by setting up a Python 2 server, I could test this feature. Do you think it could be of interest ? Or would you prefer to just upgrade (after validating the new version with some summary tests) and see what breaks ? |
@@ -61,7 +69,7 @@ backupApp.directive('backupEditUri', function(gettextCatalog) { | |||
var createFolder = function() { | |||
hasTriedCreate = true; | |||
scope.Testing = true; | |||
AppService.post('/remoteoperation/create', uri).then(testConnection, handleError); | |||
AppService.post('/remoteoperation/test', uri + '&autocreate=true').then(testConnection, handleError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason to change this from create to test?
In my opinion, the autocreate parameter should not be part of the URI to test but rather part of the request URI, since it is supposed to be handled by the webserver: /remoteoperation/test?autocreate=true
This way it will be not passed on to the backend, where it is not supported. Also in the current form this requires that uri already has a query string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was done because the RemoteOperation.cs
is a bit incomplete, and only the test
method includes the setup for OAuth server
@@ -66,7 +65,7 @@ private void ListFolder(string uri, RequestInfo info) | |||
|
|||
private void TestConnection(string url, RequestInfo info) | |||
{ | |||
|
|||
bool autoCreate = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said above, I would use the query string of the original request:
bool autoCreate = info.Request.QueryString["autocreate"] == "true";
or something similar
if (oauthUrl.endsWith("/refresh")) | ||
oauthUrl = oauthUrl.substr(0, oauthUrl.length - 7); | ||
$scope.oauth_service_link = oauthUrl; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why it doesn't work with advanced options on step 5 is that this only uses the options in the backend uri. I think that is fine since test connection should only look at that part of the options. I believe any other option in step 5 would also be ignored for the connection test. Most people (of the few that need a different oauth handler) would use a global option anyways.
}; | ||
|
||
$scope.oauth_start_token_creation = function () { | ||
builduri(perform_oauth_start_token_creation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
builduri
will fail in case some validation is not successful. That means you need to specify the authid for some backends before you can create an auth ID. It would be better to use scope.AdvancedOptions
to check for the oauth-url
option instead. That has a binding to the options editor, so it should always be up to date without needing to build the uri.
oauthUrl = oauthUrl.substr(0, oauthUrl.length - 7); | ||
$scope.oauth_service_link = oauthUrl; | ||
} | ||
const token = ur.searchParams.get("authid"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the token here should be the authid. It has a different format and I think it is mostly to connect the fetch request in recheck
. I would keep it at the randomly generated value set in scope.oauth_create_token
.
} else { | ||
scope.oauth_in_progress = false; | ||
if (wnd != null) | ||
wnd.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe if you use the advanced options instead of builduri, it should be possible to keep the oauth_start_token_creation
function in this oauth loader. That way it is a more modular design, rather than having oauth specific code in the general backupEditUri code.
mostly done now, I have made several backups without the obsolete webapp2 replaced by Flask. Not yet uploaded the source or even updated the Gae test server, but it works on my machine (TM).
yes it's possible. The Dropbox, Box configurations are 100% free and easy, the Microsoft (OneDrive) is 100% free and not so easy (but doable), the Google (Gdrive) is not free (needs credit card) and hideously complex. It works but it's surely not done the best way. Given the horror of Google configuration, I have not yet tried Amazon (must be dreadful too)
@kenkendk still waiting for an update. I have not tested at all the workers feature. At this point I have not even looked how it works at the Gae level. @Jojo-1000 |
@gpatel-fr The service is "up": https://oauth-service.duplicati.com I did not do any re-design so it still looks quite dated. I will not be able to provide seamless migration for at least the Microsoft based services, as they have updated their service, and I cannot have both the old and the new server active on the same tokens. Update: Jotta and Google seems to be working. Hope this is sufficient to test more from your side. |
The migration path needs to be clearer. Especially for Google, since there is this weird feature of tying the files to a given connection with the default restricted access - if I understand correctly, a token. Having to recreate a new token for a new OAuth server is annoying. Having to download all files and upload them again with Duplicati.Commandline.BackendTool.exe would be quite a bit more than that. AFAICT there is no visibility with Duplicati stats on the % of Google Drive backups done with restricted access vs full access (where it would be simply annoying to recreate a new token). |
I agree. But, I think this is a bit off-topic, so I will announce the new service on the forum and hopefully get a few brave souls to give feedback. |
In case the announce mentions migration issues, above has one statement worrying about two MS servers on same tokens, then another worrying about Google in a way I hadn't heard before. I thought theirs is connected to an app, however it did its OAuth. Choose Google Drive API scopes however there's no telling what other wrinkles there might be that aren't visible until one tries... |
This pull request has been mentioned on Duplicati. There might be relevant details there: https://forum.duplicati.com/t/replacing-the-oauth-service/17447/7 |
I think the issues raised by @Jojo-1000 were very valid, but it changes the implementation approach. |
should fix #4714 and #2470
This is a rather tentative PR, not very well tested.
There is a lack of an actual alternate oauth server of course, I have one at
https://duplicatipy3.ew.r.appspot.com
it's only for testing, I have a dev account for box.com only (other providers not available here, sorry).
This server is not a long term fact, don't expect it to last more than a few weeks, the migration of the production server is not really thought out yet. If you want to test the whole process, getting an account at box.com for 10 GB is free (only an email address is required).
As this serves me to test the python3 migration, it could even be broken from time to time in the following weeks.
Also, when connecting directly to it, the Box connection is displayed (it works), but a button is displayed for Jottacloud also. I don't know why since I don't have a Jottacloud dev account. There is something special going on there but I have no idea.
@albertony maybe ?
The previous code was relying on an advanced parameter specifying a server with a /refresh suffix, and had in the JS code the same server hard coded (but without a /refresh suffix). To keep the same scheme I have kept the /suffix idea even if can lead to some wretched code adding and removing the /refresh suffix.
This PR allows to set the alternate server in 2 ways:
The alternate server can't be set reliably as an advanced parameter (tab 5 of the Web UI)
Don't ask why, I don't know.