Conversation
As this is a major update, proposals to change any other API method names are welcome. |
|
Why did you change 'command' to 'api' prefix? |
Not only 'command'. Now all API methods are under |
|
Thx a lot @glassez... again. As per https://github.com/qbittorrent/qBittorrent/pull/7610/files#diff-177e7d5725be515944f83f47a0dc59bb I assume it can be enabled at run time via the preferences file, right? What I couldn't detect from the commit is if the alt webUI is cached somehow at start, or it can be interactively changed while running and be reloaded on page refresh... could you please confirm? @naikel do you mind to open a repository for your current alt webUI so we adapt it and avoid starting from scratch? |
|
My WebUI has almost nothing implemented only the transfer list!! Not even stop/start buttons! Sadly I haven't got the time to implement more features, but it uses ReactJS and Bootstrap for look n' feel and menus and stuff (and jQuery as well). |
|
@WolfganP, you can enable alternative UI either during runtime via Preferences (e.g. via Options dialog) or via configuration file editing (when application is not running). |
AFAIK, the application use file translation that caches already translated files. So if you switch to another UI source folder you can refresh, but if you update files in currently used source folder you need to restart the application. Unless I missed something... Need to be tested. |
src/base/preferences.h
Outdated
There was a problem hiding this comment.
These two new options should be merged. Probably with a "Set default" button in the UI.
There was a problem hiding this comment.
Initially I have only one option. But someone might want to turn "Alternative UI" off, but save the path for further use. Besides, I accept the idea that other settings can appear under "Alternative UI".
There was a problem hiding this comment.
The path can be saved elsewhere, what's the problem with that?
There was a problem hiding this comment.
Can't follow your thoughts...
We need one setting to enable/disable Alt UI, and the second one to store path.
How to disable it but store save path using only one setting?
There was a problem hiding this comment.
IMO, there is no need to save the path. Those who need it, can save it as they like. Or, even better, you may give them a list to choose from. Then there will be no need to save and re-type that path.
There was a problem hiding this comment.
The program is incorrect if it produces the incorrect output for correct input and settings.
I'm talking not about correctness of the program, because it is correctly implemented to contain the designed security hole.
There was a problem hiding this comment.
"Allow the user to do what he has a right" is a security hole?
There was a problem hiding this comment.
If a user is authenticated and passes a correct CSRF token with the request, what's the issue with allowing them to set the webui directory? At that point they have full control over the webui anyway, including the option to specify ANY external program to execute upon torrent completion.
There was a problem hiding this comment.
@Piccirello, shhh... If @evsh reads this, you'd be anathematized and burned at the stake!
There was a problem hiding this comment.
Yes, any binary can be downloaded to the target system and executed via post-download command. Yet I strongly believe that presence of a mistake or design flaw in a program may not be used as an excuse for adding more problems.
|
PR updated: |
@WolfganP, fixed. |
|
I did not read the PR completely, but already want to know what could you, @glassez, comment on the following. I can't see how we separate a Web UI from qBt HTTP API. |
What exactly do you want to know? How is it separated in the code? Or in the request path? |
Consider yet another qBt API, which differs from the HTTP one mainly by marshaling scheme. For example, DBus. Should there be an object |
Ok. I think I undestand you. |
src/icons.qrc
Outdated
There was a problem hiding this comment.
It is possible to move www/favicon.ico to cpp code? it's quite hard to find here.
There was a problem hiding this comment.
It is possible to move www/favicon.ico to cpp code?
It was previous behavior.
or move it to HTML?
What do you mean?
There was a problem hiding this comment.
What do you mean?
There was a problem hiding this comment.
@Chocobo1: how a creator of an alternative UI would get the true icon name?
There was a problem hiding this comment.
@Chocobo1: how a creator of an alternative UI would get the true icon name?
Now I think we should properly introduce an ICO format favicon instead of creating an alias of the png.
We can do it later if you won't handle it now.
There was a problem hiding this comment.
how a creator of an alternative UI would get the true icon name?
He just add the icon he want into his www folder...
There was a problem hiding this comment.
@Chocobo1, <link rel="icon" type="image/png" href="images/skin/qbittorrent16.png"> is used now.
src/webui/abstractwebapplication.cpp
Outdated
There was a problem hiding this comment.
Can you put an upper limit for it?
src/webui/abstractwebapplication.cpp
Outdated
There was a problem hiding this comment.
and should be possible to use QStringRef if you like.
There was a problem hiding this comment.
and should be possible to use QStringRef if you like.
Don't see any sense. It will be converted to QString later.
There was a problem hiding this comment.
It will be converted to QString later.
OK and I'll continue the review later.
src/webui/abstractwebapplication.cpp
Outdated
There was a problem hiding this comment.
hope you don't find marking it a const function
There was a problem hiding this comment.
Can't follow you...
This is static method.
src/webui/webapplication.cpp
Outdated
There was a problem hiding this comment.
When converted to QString this will print 2.0.0 right?
If so, then it will break all existing usage/detection.
There was a problem hiding this comment.
Well, the points of incompatibility creation should be it the development process, otherwise we will always have to live with the mistakes previously...
Really, there should not be a big problem since it a major update and old clients can't work with it anyway (because of changed API method names/paths).
There was a problem hiding this comment.
Really, there should not be a big problem since it a major update and old clients can't work with it anyway (because of changed API method names/paths).
OK, then we should inform the public the changes beforehand, making an announcement on the qbt website news page when this PR is merged, and this should be as early as possible as we have a lot of users using webAPI.
@sledgehammer999
There was a problem hiding this comment.
@glassez
I was thinking this should start following Semantic Versioning.
If so, this should go {<old API_VERSION> + 1, 0, 0};, what do you think?
There was a problem hiding this comment.
I follow it but in another way. I try to follow qBittorrent main versioning rule when we change major version only on some very big changes. So I interpret old version as 1.x.y in new scheme and just perform major update on it.
But if most of you guys prefer "to be measured penises" (as Firefox and Chrome do), then I'll change it to the option you proposed.
There was a problem hiding this comment.
- Using Semantic Versioning is one thing, we can start practicing it if the majority agrees.
IMO Semantic Versioning is invented just for this situation. - The
{2, 0, 0};you proposed is another, problem is: with the current/version/api GETit returns an integer15, but after this PR it drops back to2.0.0which is a confusion for our users.
So from a user perspective I would recommend using16.0.0(even we don't follow Semantic Versioning).
Also why not useUtils::Version<int, 3, 3>?
https://github.com/qbittorrent/qBittorrent/wiki/WebUI-API-Documentation#get-api-version
There was a problem hiding this comment.
it returns an integer 15, but after this PR it drops back to 2.0.0 which is a confusion for our users.
It's not a problem, IMO. But I'll do whatever the team decides.
Also why not use Utils::Version<int, 3, 3>?
Personally I prefer the way when zeros after minor version are truncated. Waiting other reviewers.
There was a problem hiding this comment.
@lgallard
what do you think about the suggestion in: https://github.com/qbittorrent/qBittorrent/pull/7610/files#r145924980
src/webui/webapplication.cpp
Outdated
There was a problem hiding this comment.
personally I prefer using = for string...
There was a problem hiding this comment.
I would prefer leave it as it is now.
src/webui/webapplication.cpp
Outdated
There was a problem hiding this comment.
Just asking, isn't pathItems.contains(".") redundant? It's basically the current/same path, why should it be prohibited?
There was a problem hiding this comment.
I don't know. It's very legacy code. Should I change it?
There was a problem hiding this comment.
Should I change it?
We both unsure, then just leave it.
src/webui/webapplication.cpp
Outdated
There was a problem hiding this comment.
I think we should not allow a requester to get anything they want when they are not authenticated, i.e. they should only be allow to get the login page resources.
There was a problem hiding this comment.
What about images/scripts/css used in login page?
Anyway html files are useless without API access.
There was a problem hiding this comment.
What about images/scripts/css used in login page?
I was thinking of a tighter control scheme but it's not beneficial with user's webUI, so whatever.
There was a problem hiding this comment.
Excellent security hole, BTW! Right from a textbook...
There was a problem hiding this comment.
Look, stop being sarcastic! In your comments some emotions, there is no useful information. What is the problem here?
There was a problem hiding this comment.
There are specific proposals? Or just wait for the requests from real users of this feature?
It's just still a primitive idea in my head, it might not make full sense.
I was thinking there should always be 2 folders under a user specified path (also applies for built-in UI), name them public & private.
- The public folder contains resources that anyone can freely access, html files, js files, or even some API.
Then we can put every resources that is needed for login in this folder. - The private folder contains other resources that is only accessible after login.
The reason for this, is that I want to minimize the exposure to the public.
Why? Because the first step of web security penetration is reconnaissance, i.e. knowing what software & which version it is running, thus reducing information leakage is important IMO.
Also replying #7610 (comment) here.
It's meaningless at least for api version. Client should know it to understand how to authenticate (authentication can be changed in some future version).
Given the above idea, I think the api version is not a critical info at the authentication stage, webAPI users can simply try another login scheme when the previous failed, assuming we aren't inventing a login scheme every year.
There was a problem hiding this comment.
Something like that used in the current (current master) implementation, but it is somehow flawed... so I decided to abandon it in favor of simplicity. But I can return to it if I can see a complete picture of what is what.
Then we can put every resources that is needed for login in this folder.
AFAIK, all common resources (js, css, images) are public in web apps (I mean images used in page formatting etc.).
There was a problem hiding this comment.
AFAIK, all common resources (js, css, images) are public in web apps (I mean images used in page formatting etc.).
Ideally only login-related resources should be in public folder, everything else should go in private folder.
And login-related resources should be independent from other resources, some resource might get duplicated but it's a very small cost IMO.
And as a side effect (bonus?) of this model, you can turn qbt into a dumb public file server by placing any file (with max file size limitation though) in the public folder.
There was a problem hiding this comment.
I don't like to have private/public in request paths. I have idea to make it transparent for the client side, i.e. when client send request with /page.html path we search for page.html in public subfolder if client isn't authorized, otherwise we search in both private/public subfolders (in this order). This approach allows to avoid resources duplicating.
There was a problem hiding this comment.
I don't like to have private/public in request paths.
Me too, yet I haven't consider how to implement it.
I have idea to make it transparent for the client side
👍 seems probable.
|
I'm eagerly waiting for this PR to be merged to master to start experimenting with alternate webUIs and while trying to asses how stable will be to merge it in my local dev env; I looked back at the blocker issues (@evsh requesting changes) and it seems the original request goes back to Oct 22nd 2017. |
|
@WolfganP, this PR is ready and it will be merged once v4.0.4 is released. |
glassez
dismissed
zeule’s stale review
Very overdated "changes request", so GitHub can't even provide linked code.
|
Thanks to all the reviewers! |
|
Excellent work, thx a lot to all devs and reviewers. |
|
@glassez Can you document the updated API urls on the wiki? This will be really useful for other applications that make use of the qBittorrent api. Also, is it a good idea to introduce such a major API overhaul with a minor version bump? 4.1.0 seems far more appropriate. |
|
I fiddled a bit with the current API documentation in the wiki and made sure each article explicitly states which version it applies to, updating also the Home page in preparation for whatever documentation comes up for this new release. I also created https://github.com/qbittorrent/qBittorrent/wiki/Alternate-WebUI-notes to start documenting the feature and its usage (ie files names and locations, program switches / config entries, etc etc), which I'll initially work out from the current code but it will need a review from @glassez for sure. I agree with @Piccirello that this API overhaul should be tied to a major version rather than a v.4.0.5 minor one. |
Please revert the document title (the current API one), as it is tied to the page URL and someone might already bookmark it.
Great! |
|
@Chocobo1 Changes done! |
Guys, what exactly is your concern?
Of course, I'll do it when I have some time for this job. |
| APIController *controller = m_apiControllers.value(scope); | ||
| if (!controller) { | ||
| if (request().path == QLatin1String("/version/api")) { | ||
| print(QString(COMPAT_API_VERSION), Http::CONTENT_TYPE_TXT); |
There was a problem hiding this comment.
This should be QString::number(COMPAT_API_VERSION)
| print(QObject::tr("I/O Error: Could not create temporary file."), Http::CONTENT_TYPE_TXT); | ||
| continue; | ||
| if (request().path == QLatin1String("/version/api_min")) { | ||
| print(QString(COMPAT_API_VERSION_MIN), Http::CONTENT_TYPE_TXT); |
A majority of the legacy apis are no longer working. With some quick testing, I was able to identify these as broken (non-inclusive):
Please test all of the legacy apis to catch any additional ones that are broken. |
OK. I'll fix it. Thank you for testing. |
Fixed via
Fixed via fixing regexp (for some reason I put "control" instead of "command" there)
Work for me.
Fixed. |
|
@Piccirello, see #8515. |
|
@glassez It looks like TransferController::infoAction() is no longer used, and its logic migrated to |
|
If some API isn't used in official Web UI, it's not a reason to drop it out. Some other apps may want to use it, or may want to use it in the future. |
|
Good point, it makes sense to leave it in. |
|
@WolfganP, @Piccirello, @Chocobo1, the documentation for new API is here. I followed the style of legacy API documentation, even though I don't like it (I have no time to mess with it). Please correct any errors or inaccuracies you have noticed. |
|
@glassez do you mind to get a quick review & editing to this article https://github.com/qbittorrent/qBittorrent/wiki/Alternate-WebUI-usage so there's a base in the docs for those who want to start fiddling with alt WebUIs? |
|
@glassez Is there a recommended/durable/invariant method to check appropriate webAPI to use? I'm a tad confused as it seems you need to know/guess the correct API to use to check the webAPI version, eg
vs
Or am I misunderstanding something? |
11 participants
Normalize Web API method names.
Invoke Web API methods via Qt Meta Object system.
Allow to use alternative Web UI.
Switch Web API version to standard form (i.e. "2.0").
Improve Web UI translation code.
Retranslate changed files.