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

Add File Manager / File Browser based on form.FileUpload #6553

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

stokito
Copy link
Contributor

@stokito stokito commented Aug 31, 2023

Luci needs for a file manager.
The FileUpload CBI element allows to browse files and directories on a router, delete and upload. This already covers a basic needs. The only really missing features are:

  • Download a file. Required.
  • Edit right in a browser. Highly improves UX but not necessary. User can download, edit with a Notepad and upload.
  • See and change permissions. Important.
  • Copy\Move. Good to have.
  • Compress, extract and browse archive. Often used.

So we can extend the FileUpload and add a "browser mode" when you don't select a file but just walking on directories. The PR is PoC of the idea: it adds a new menu item System / File Browser that opens a page with the FileUpload in a browser mode:

luci file browser sample

Here the directory listing was opened automatically and no Cancel or Select buttons.

It's ugly but just something that shows the idea (or even can be used today if you really need).

So my questions are:

  • Would you accept a PR with a file browser?
  • How do you feel about reuse of the FileUploader?

Some other existing solutions:

CC @giaulo @muink @xiaozhuai @helmiau @ozon @stangri: You guys may be interested in the topic.

@stangri
Copy link
Member

stangri commented Aug 31, 2023

Looks good, I hope it gets implemented.

Tagging @jow- and @hnyman for feedback.

@hnyman
Copy link
Contributor

hnyman commented Sep 1, 2023

I am not sure if I like OpenWrt/LuCI moving more toward functionality that is more typical to a full-scale desktop distro. I fear that it will add complexity and new error possibilities.

We already had a file browser "luci-app-rosy-file-server" a few years ago (more like a download site ), but the author disappeared rather soon and the app was left unmaintained. (And it initially had some security flaws.) That app was dropped a few years later.

The basic functionality might be ok, but when you start talking about moving files, compress archives and "browse archive contents", we are soon in a feature swamp, taking into account the various FS types and archive formats possibly in use.

I like the current situation, where LuCI is used for user-friendly high-level config, but if you need to manipulate individual files etc. tasks, you need to familiarize yourself with the SSH console and cmdline in general.

@stokito
Copy link
Contributor Author

stokito commented Sep 1, 2023

I agree with your concerns. Still many packages doesn't have a Luci app or is not merged yet like luci-app-sshtunnel, luci-app-tor ;)
So having the ability to easily edit/upload configs will significantly reduce load on Luci team because users will be able to make many things themselves. Any video tutorial of HomeAssistant contains editing config files right in browser.

The Rosy File server may be just one bad example because it become unsupported. If the File Browser will be part of Luci itself that would be much better but this is an additional load to the team.

@hnyman
Copy link
Contributor

hnyman commented Sep 1, 2023

If the File Browser will be part of Luci itself that would be much better but this is an additional load to the team.

Just be realistic: there is not much "LuCI team". Jow is the main guru, feckert has updated a lot, systemcrash recently something, polynomialdivision something, I have taken care of some apps and some general maintenance, but there is no large "LuCI team". There are then several active app maintainers like dibdot, who take care of their own apps. But for the "general LuCI functionality", Jow is (and has been) pretty much alone, especially regarding the more demanding tasks.

Thatswhy I am sceptical about introducing apps with complex functionality reaching to file permission changes, file moves, archive browsing, compression etc. Taking into account the diversity of the OpenWrt device/FS universe, the scope quickly grows pretty complex.

If a file browser is authored by you, the expectation is that you will take care of it for a few years. Hit-and-run introductions of new apps where the original author quickly loses interest, have gone pretty badly in history.

@muink
Copy link
Contributor

muink commented Sep 1, 2023

Tiny File Manager meet most of your requirements, and only depends on php.
You can introduce this package in openwrt/packages and maintain it as maintainer.
So that it does not introduce complexity to existing systems and easy to maintain (Ignoring CDN issues even works out of the box).
And can also be installed via the software center in a brand new OpenWrt device. @stokito

@muink
Copy link
Contributor

muink commented Sep 5, 2023

@stokito
Copy link
Contributor Author

stokito commented Sep 5, 2023

Ok, so even in the current state the simple file browsing will make life easier. I'll remove the separate module and make it just built-in into mod-system.
I'll get back later to this and will try to implement features like download. I'm pretty sure that we can do that without significant increasing of complexity and size.

But if anyone interested you can go ahead yourself or call me and we can work together.

@muink thank you, the PHP is too big for most routers. But I fill take inspiration from file managers listed here. For sure I can at least grab translations :)

@shodanx2
Copy link

luci-app-tinyfilemanager looks great, but it's too difficult to install and if I did install it, I might have run out of space

But that would have been it, file manager, file editor and a proper terminal would grow openwrt's usability by leaps and bound.

@stokito stokito force-pushed the luc-mod-system_filemanager branch 2 times, most recently from f2ed6f0 to 2a88abc Compare September 21, 2023 20:23
@stokito
Copy link
Contributor Author

stokito commented Sep 21, 2023

I updated the PR and it's not draft anymore but a ready to merge thing. So please review it.

  • New page System / File Browser that internally just uses the FileUpload
  • To the FileUpload UI element added a browser mode that opens automatically the directory listing and doesn't close it when clicking on a file.
  • Added a new Download button.
    Luci file browser Download

The button is hidden by default so existing file selectors won't be affected.

The main problem in the code remains this code:

if (this.options.browser) {
	setTimeout(function() {
			btnOpenFileBrowser.click();
		}, 1000
	);
}

Here I tried to click on the "Select file" button to open the directory listing automatically. I don't know how to do that in a nice way because the render function is called before the page initialization.

This is a most minimal and safe to merge changes without breaking existing UI pattern.

But ideally we need to change layout and move the Delete and Download button to a bottom toolbar. Same should be done for Select/Deselect and buttons.
Then a user should select a folder or a file(s) and perform an operation. Something like this:

image

It looks ugly but just to get an idea.

This is a bigger redesign and we can make it later and test more carefully.

But this PR should be safe to merge and we can make the Luci better just today.

@stokito stokito marked this pull request as ready for review September 21, 2023 21:14
@stokito stokito changed the title [Draft] Add File Manager / File Browser based on form.FileUpload Add File Manager / File Browser based on form.FileUpload Sep 22, 2023
@systemcrash systemcrash added feature pull request adding a new feature fix pull request fixing a bug Needs testing Need testing labels Dec 5, 2023
@rdragonrydr
Copy link

rdragonrydr commented Feb 25, 2024

How do I help test this? I'm not sure I have the expertise to make code changes but I can try it out - this is in Snapshots branch?

This is something I've been looking for from OpenWRT for a long time, the ability to have a file browser (with upload/download) with minimal if any dependencies (my devices are small) so I can more easily use it as a network file backup or host media for sharing with DLNA.

I would love if I could set a default directory that the browser opens to each time, but what you have seems good enough (even if I look forward to the UI improvements).

Edit for more details: Apparently, installing Samba takes up ~8mb on 23.05, so this leaves me with about 0.7mb left out of my 16mb device! Meanwhile, installing a different web file manager requires PHP and usually SQL, and it needs separate password/user setup too, if you can even find one that is easily installed in such an unusual and constrained environment. I would expect a built-in manager to take merely a few hundred K on the other hand.

@stokito
Copy link
Contributor Author

stokito commented Feb 25, 2024

Sorry, if you wish to test it then you have to install a snapshot version and copy the files that I changed.
This is better to make in VM e.g. VirtualBox.
Basically the PR now adds only a Download button and a separate page with the FileUpload control.

If you just want to see how it looks you can open
Network/Firewall/IP Sets then click on Add and click on Include File: Select file….
Then you'll see the exactly same file chooser dialog but without a Download button.

@shodanx2
Copy link

I look forward to try that on my openwrt running router running stable branch.
Please post here how to accomplish when it becomes possible.

I think for the openwrt's most regular challenges, the following abilities are most important.

Upload a file to a place
open a text file, read it and search it
Open a text file, edit it and save it

Hopefully, this lowers the bar for the users which cannot cross the putty/winscp barriers

In the Browser mode the file tree dialog won't be closed when clicking on a file.
The mode is used by a File Browser.

Signed-off-by: Sergey Ponomarev <[email protected]>
@stokito stokito force-pushed the luc-mod-system_filemanager branch from 2a88abc to adbde3f Compare March 8, 2024 17:25
@stokito
Copy link
Contributor Author

stokito commented Mar 8, 2024

I managed to get rid off the setTimeout() to click on the Select button to open the file browser.
The UIElement doesn't have a dedicated stage "after render" so I had to add a then() to a render promise.

if (this.options.browser) {

I think it would be better to introduce such method. Also load would be nice to add because now the file browser get a list of a directory fs.stat() in the render function

var renderFileBrowser = L.resolveDefault(this.value != null ? fs.stat(this.value) : null).then(L.bind(function(stat) {

Anyway, please merge the PR. Even the basic file browser can solve many problems for users not familiar (yet) with SSH and SCP/SFTP

@systemcrash
Copy link
Contributor

OK - since other senior members are AWOL on this change-set, and it looks OK, I'll merge this soon.

Can this get picked for 22 and 23?

@jow-
Copy link
Contributor

jow- commented Mar 13, 2024

  • You need to disable page actions (Reset, Save, Save Apply) on this page
  • You also need to mark the JSON Map as readonly if there's insufficient ACLs (only read scope)

I am also not happy at all with having this by default, it will grant full write access to any file on the system for the ui user.

And no, please don't pick such a big feature addition + ui rework + permission relaxation into the stable branches, at least not as integral part of luci-mod-system.

Maybe it is better to introduce it as application.

@systemcrash
Copy link
Contributor

@jow-

Maybe it is better to introduce it as application.

Does app segregation mean it can be handled by acl permissions?

@jow-
Copy link
Contributor

jow- commented Mar 13, 2024

It means it will not be installed by default and/or it is possible to have builds without it.

@stokito stokito force-pushed the luc-mod-system_filemanager branch 2 times, most recently from 4d24c93 to fdacde3 Compare March 14, 2024 19:57
@stokito
Copy link
Contributor Author

stokito commented Mar 14, 2024

You need to disable page actions

Thank you, fixed and added to the filebrowser view:

	handleSave: null,
	handleSaveApply: null,
	handleReset: null

Force pushed.

mark the JSON Map as readonly

I made the form m.readonly = true but the FileUpload element looks like this:
disabled file browser

it will grant full write access to any file for the ui user.

I fully removed the read access in the acl.
With luci-app-acl I created a user with global permission readonly. In the mode the file browser is not even shown in menu.

Maybe it is better to introduce it as application.

It's small for now and should be nice to have out of the box.

@rdragonrydr
Copy link

My two cents is that I'd be fine with this as an application. I don't, personally, need this on a bridge or a router with tiny space, but if I'm building something with external storage or media it would be a must.

I'm not sure I'm following the permissions discussion, though. It's clearly important, but for an end user like myself, under what modes of operation is it read-only? Do I have to enable it to be editable or is it possible to lock it down and it's default writable?

@stokito
Copy link
Contributor Author

stokito commented Mar 16, 2024

Check luci-app-acl. It allows to create a read only user

@shodanx2
Copy link

I am excited to try this out !

How much space does it take
and can you read a text file in the interface without downloading it ?

As I wrote in this thread
https://forum.openwrt.org/t/adding-a-file-browser-file-text-editor-and-tty-command-shell-for-luci-openwrt/138930

File manager, along with a text editor and console, in luci will remove a major roadblock for new openwrt user to access the complex functions not available in the web interface.

@stokito
Copy link
Contributor Author

stokito commented Apr 9, 2024

Sorry for pushing, but can we merge it?

@jow-
Copy link
Contributor

jow- commented Apr 9, 2024

Not as part of luci-mod-system.

Allow downloading from a file browser.
The Download button is located near to Delete.
It's shown only for files: folders or /dev/ devices can't be downloaded.
The downloading is made via fs.read_direct() which internally calls cgi-download.

Signed-off-by: Sergey Ponomarev <[email protected]>
@stokito stokito force-pushed the luc-mod-system_filemanager branch from fdacde3 to f086bb2 Compare April 9, 2024 07:44
@stokito
Copy link
Contributor Author

stokito commented Apr 9, 2024

Ok, now this is a separate app. Made a force push.
If anything needs to be changed please let me know.

@jow-
Copy link
Contributor

jow- commented Apr 9, 2024

Thanks for taking care of separating it. The only minor thing I see is that the ACL group name should be changed from "luci-mod-system-filebrowser" to "luci-app-filebrowser". Otherwise LGTM.

Add a File Browser based on the FileUpload CBI element.
It allows browsing files and directories on a router, delete and upload.
This covers only basic needs.

Signed-off-by: Sergey Ponomarev <[email protected]>
@stokito stokito force-pushed the luc-mod-system_filemanager branch from f086bb2 to 6afea76 Compare April 9, 2024 08:02
@stokito
Copy link
Contributor Author

stokito commented Apr 9, 2024

fixed, force pushed

@jow- jow- merged commit 1181915 into openwrt:master Apr 9, 2024
4 checks passed
@jow-
Copy link
Contributor

jow- commented Apr 9, 2024

Thanks, merged!

@stokito
Copy link
Contributor Author

stokito commented Apr 9, 2024

Thank you! Later I'll back to this and will try to change a design and add editing.

@stokito stokito deleted the luc-mod-system_filemanager branch April 9, 2024 08:14
@stangri
Copy link
Member

stangri commented Apr 10, 2024

@stokito do you want to create a cherry-pick PR for release?

@stokito
Copy link
Contributor Author

stokito commented Apr 10, 2024

base files were changed (ui.js, form.js), so we can't cherry pick potentially breaking changes

@stokito
Copy link
Contributor Author

stokito commented Apr 13, 2024

Please create a Weblate translation component https://hosted.weblate.org/projects/openwrt/

@hnyman
Copy link
Contributor

hnyman commented Apr 13, 2024

Please create a Weblate translation component.

I did that.

@rdmitry0911
Copy link

I've rewrote filebrowser.js to add editing feature and more convinient navigation.
Screenshot 2024-09-30 at 4 49 57 PM
Screenshot 2024-09-30 at 4 49 17 PM
I forked luci and commited the changes to master branch.

@stokito
Copy link
Contributor Author

stokito commented Sep 30, 2024

Nice work! Here is the commit rdmitry0911@3659619

I used a simplest possible approach to reuse the FileUploader while you implemented it as a separate view. This makes your solution slightly bigger. My initial idea was to add the File Manager as a part of Luci core and maybe this will happen later.
If you with to add your plugin it probably would be better to create a separate plugin because basically it has nothing from the original plugin. You may send a PR and probably it will be merged so that users can install it from the official repo.

@rdmitry0911
Copy link

Nice work! Here is the commit rdmitry0911@3659619

I used a simplest possible approach to reuse the FileUploader while you implemented it as a separate view. This makes your solution slightly bigger. My initial idea was to add the File Manager as a part of Luci core and maybe this will happen later. If you with to add your plugin it probably would be better to create a separate plugin because basically it has nothing from the original plugin. You may send a PR and probably it will be merged so that users can install it from the official repo.

Thank you! It is mostly chatgpt' efforts :) I'll try to make a PR for a new plugin. What name you would propose to distinguish it from yours? filemanager?

@systemcrash
Copy link
Contributor

If you make a PR - don't do it from your main/master branch.

@rdmitry0911
Copy link

If you make a PR - don't do it from your main/master branch.

What is the best way to make a PR?

@muink
Copy link
Contributor

muink commented Oct 1, 2024

@rdmitry0911 branch named based the feature name.

@stokito
Copy link
Contributor Author

stokito commented Oct 25, 2024

FYI: A new luci-app-filemanager is going to be added in the #7300
Please review, test and contribute

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature pull request adding a new feature fix pull request fixing a bug Needs testing Need testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants