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

Onlyoffice #1420

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Onlyoffice #1420

wants to merge 10 commits into from

Conversation

drosoCode
Copy link

Description
Add onlyoffice editor, to be able to edit word, excel and powerpoint docs.
OnlyOffice Document Server is required, the URL to this server can be configured in the General Settings.
To run the document server: docker run -d -p 8081:80 onlyoffice/documentserver

Image:
image

Related issues:

@drosoCode drosoCode requested a review from o1egl as a code owner May 31, 2021 12:45
@nook24
Copy link
Contributor

nook24 commented Oct 22, 2021

Is there a reason why there is no feedback to this?

@ghost
Copy link

ghost commented Nov 4, 2021

onlyoffice is suopported ? it can be integrated with filebrowser?

@youegraillot
Copy link

I would love to see this implemented !

@SaidTorres3
Copy link

Thank you so much, best feature ever

@henryclw
Copy link

This looks great, thank you for your work, hope this could be merged soon.

@Akeid
Copy link

Akeid commented Aug 22, 2022

Can we get this merged?

@o1egl o1egl added the wip Work in progress label Aug 29, 2022
@baur
Copy link

baur commented Jan 31, 2023

Is it possible to open the file in read-only (viewer mode)? (by OnlyOffice of course)

@juvenalandres
Copy link

Hello, is this something that the development team is considering merge it in the near future? If so, that would be a massive update.

@Silloky
Copy link

Silloky commented Feb 13, 2023

This addition would make FileBrowser much better : it was one of the things I preferred in Filestash, before I installed FileBrowser

)}?auth=${this.jwt}`;
let key = Date.parse(this.req.modified).toString() + url.encodePath(this.req.path);
key = key.replaceAll(/[-_.!~[\]*'()/,;:\-%+.]/g, "");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want a comment explaining this regex, it's pretty intense

Copy link

@TCB13 TCB13 Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it looks like he's removing the following chars form the key -, _, ., !, ~, [, ], *, ', (, ), /, ,, ;, :, -, %, +, .. Two of them, namely ] and - need escaping with \.

Copy link

@Rushmore75 Rushmore75 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no security expert or anything, but I think they look good

@drosoCode
Copy link
Author

Nice to see some feedback on this

I've cleaned up this old code and reworked the key generation function to better match the onlyoffice recommandations.

I also added support for JWT auth as this is now enabled by default on onlyoffice (this only works on secure connections as we're using the WebCrypto API, but you can still disable JWT auth on the documentserver and leave the config field blank)

Copy link

@Silloky Silloky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything seems ok

@theautomation
Copy link

Any idea when this PR will be merged? Can't wait to use Onlyoffice with filebrowser

@ngophuong
Copy link

Look forward to this feature 👍🏻

@TCB13
Copy link

TCB13 commented Jun 7, 2023

Thank your for taking the time add this, very nice integration.

@o1egl o1egl force-pushed the master branch 2 times, most recently from 813c2e9 to 66dfbb3 Compare July 28, 2023 22:27
@juanico10
Copy link

Hello!
When will this feature be merged?
Best regards

@fragtion
Copy link

fragtion commented Aug 2, 2023

The latest version of FileRun has built-in support for OnlyOffice and a bunch more plugins (including Google Docs, Office 365 for Web, etc) from what I've seen

@OlegEnot
Copy link

OlegEnot commented Aug 2, 2023

The latest version of FileRun has built-in support for OnlyOffice and a bunch more plugins (including Google Docs, Office 365 for Web, etc) from what I've seen

According to information from the official website FileRun:
Licensing
The free FileRun version is no longer available.
To install and use FileRun you will need an Enterprise license.

And not open-source.

@Rushmore75
Copy link

It may be that filebrowser is supposed to be more suckless, where you just merge your own patches to customize for yourself

@baur
Copy link

baur commented Aug 2, 2023

Maybe use OnlyOffice in view mode only, is for this requires filerun?

@Manu-Devloo
Copy link

Is there a timeline for this, I would love to see this become an option.

@IvanLi-CN
Copy link

This integration is very valuable, hope it will be merged soon.

@NettoHikari
Copy link

NettoHikari commented Sep 13, 2023

This very useful PR is open for such a long time already. When will it be merged?

@Rushmore75
Copy link

yall do realize that you can just use drosoCode's fork if you want the feature rn right?

@TCB13
Copy link

TCB13 commented Sep 13, 2023

yall do realize that you can just use drosoCode's fork if you want the feature rn right?

Not the point.

@m-oezokyay
Copy link

I'd also love to see this merged. There aren't many lightweight alternatives to FileBrowser that have an OnlyOffice integration. NextCloud is overkill for my purposes and Filestash has a different focus.

@TCB13
Copy link

TCB13 commented Sep 29, 2023

@o1egl and @Rushmore75 can you please have a look at this? Thank you.

}
};
if(onlyOffice.jwtSecret != "") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a security breach. What is the purpose of this secret? Security tokens should never be exposed to the clients.

Comment on lines +98 to +102
Date.parse(this.req.modified).valueOf()
+ url
.encodePath(this.req.path.split('/').reverse().join(''))
.replaceAll(/[!~[\]*'()/,;:\-%+. ]/g, "")
).substring(0, 20);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using a hashing algorithm like md5 instead of performing this magical process?

return http.StatusInternalServerError, err
}

if data.Status == 2 || data.Status == 6 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use meaningful constants instead of magic numbers

@arcastro
Copy link

arcastro commented Oct 7, 2023

oh wow, I was looking for exactly this feature and stumbled on this ticket today.
Was initially disappointed to see it'd been open since 2021, but looks like there's been some recent traction (ty!)

@drosoCode , any chance you could follow up on @o1egl 's review? :)

@drosoCode
Copy link
Author

Sure, I'll make the required changes as soon as I have enough time to work on it

@Joly0
Copy link

Joly0 commented Dec 18, 2023

Any news? Would be cool to get this merged soon :D

@arcastro
Copy link

Hi everyone, decided to try addressing the comments on this in order to get some traction again.

I've created a new pull request here:

I rebased @drosoCode 's contributions against the latest master branch and created a new commit on top of it to address the pending comments above.

@drosoCode @o1egl , would you be able to review?

@drosoCode
Copy link
Author

Hi,
Thanks for picking this up.
It's been a pretty long time since I worked on this code but yours is pretty much what I had in mind (and your new additions seems really cool !).
I think all the concerns have been adressed, so to me everything looks good. I hope we can get this merged soon !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet