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

nodejs connector #30

Closed
jlaustill opened this issue Aug 11, 2016 · 32 comments
Closed

nodejs connector #30

jlaustill opened this issue Aug 11, 2016 · 32 comments

Comments

@jlaustill
Copy link
Contributor

I just looked over the code base, and I realized this is going to be a LOT tougher than I expected. The code expects the connector to be on the web server, and not an api server. My environment is separating the api from the web server. So my question is, are you willing to work with these changes to set an api url instead of a connector path? It requires setting credentials and cors on the ajax calls as well.

I would solve this by factoring all the ajax calls into functions that emulate jqueries $.get, $.put, etc., and either calling the connector, or the api endpoint based upon the configuration. I would be willing to do this, but I wanted to check and see if anyone had issues with it?

@psolom
Copy link
Owner

psolom commented Aug 11, 2016

It sounds good if you are going to make it optional without breaking the current implementation, because most of FM users utilizes web server indeed. Also I have to say that most of ajax calls will be overwritten (wrapped) to use Deffered objects. Take a look at PR #21 filemanager.js file, if you are interested. It's just a note for you to be sure you are aware of the upcoming changes.

Before you create your PR you could share your thoughts and code snippets of how you want to implement your idea, modify ajax calls and so on. Write the things right here and we will discuss them.

@fabriceci
Copy link
Contributor

fabriceci commented Aug 11, 2016

Maybe I misunderstood something but it's already possible to use a custom URL for the connector.

If you set in the config

fileConnector": "http://www.google.be/api",

Then the file manager will call

Request URL:http://www.google.be/api?mode=getfolder&path=%2F&config=filemanager.config.json&showThumbs=false&time=1470901701222&_=1470901700948
Request Method:GET

We only needs to implement two optional parameters to the configuration object , something like :

 window._FMConfig = {
     pluginPath: "/resources/cms/libs/RichFilemanager-master",
     getParams :[object],
     postParams: [object]
};

(I already need something like this in my case, to add a CRSF parameter for the post requests.)

@psolom
Copy link
Owner

psolom commented Aug 11, 2016

@fabriceci Thank you for the example.
@jlaustill Let us know if you need something more than that

@jlaustill
Copy link
Contributor Author

I totally missed the fileConnector option digging through the code, I'll look for that today. This is a pretty large code base, so excuse me while I get familiar with it :) I think fabriceci's right on the money, all I would need is to add credentials true and crossDomain true to the getParams and postParams and I think I'd be set to get to work on the node api. I'll get my environment setup when I get to work today and start plugging away and see what I can figure out.

@fabriceci
Copy link
Contributor

fabriceci commented Aug 11, 2016

The getParams/postParams was a suggestion, those parameters doesn't exist yet. I plan to add in a PR this weekend (like I said I need to pass a CSRF token for POST request)

It can be easy, simply add the "getParams" in the $.Ajax function and "postParams" in the upload function.

( But the best way is like you said wrap all the Ajax calls in one function, cleaner but take more time.)

@servocoder Could you tell me why all requests are not sent asynchronously? (I never used this param)

    $.ajax({
        url: config.globals.pluginPath + '/themes/' + config.options.theme + '/styles/ie.css',
        async: false,
        success: function(data)
        {
            $('head').append(data);
        }
    });

@psolom
Copy link
Owner

psolom commented Aug 11, 2016

It is required because some Ajax requests supposed to be invoked in a chain. But I plan to rewrite this behavior, remove sync flag and utilize $.Deferred() as it was suggested in PR #21

@jlaustill
Copy link
Contributor Author

fabriceci,

I understood that it was a suggestion, I just thought it to be a very good one and I was just rolling with it as an assumed solution.

I tried the fileConnector option and it works, I can't believe I missed that :) Oddly, it didn't complain about my CORS, so I wonder if it's going to pick that up from my websites default jquery config? That would actually be pretty awesome.

Either way, I don't see any roadblocks to me getting to work on a nodejs api connector, I already have the first request sent, so it's just a matter of working through each option I guess, yay!

@jlaustill
Copy link
Contributor Author

Ok, I have my api started, and files and folders loading :) However, I ran into an issue where either the documentation is wrong, or the code is wrong. In the documentation is says to return an array of documents in the form

"/userfiles/images/logo.png": {
    "Path": "/userfiles/images/logo.png",
    "Filename": "logo.png",
    "File Type": "png",
    "Thumbnail": "/connectors/[lang]/filemanager.[lang]?mode=getimage&path=%2Flogo.png&time=1463130804&thumbnail=true",
    "Protected": 0,
    "Properties": {
      "Date Created": null,
      "Date Modified": "05/05/2016 11:38:42",
      "filemtime": 1462441126,
      "Height": 14,
      "Width": 14,
      "Size": 384
    },
    "Error": "",
    "Code": 0
  }

However, the code is assuming that the key : value pair at the root level is NOT there, it is doing this

for (var key in data) {
                item = data[key];
                props = item['Properties'];

So, if I leave out the root level key which is the filename, it works. I am assuming the documentation is wrong since this code matches the old behavior, but I wanted to run it past y'all before moving forward with that assumption. Here's what works

{
    "Path": "/userfiles/images/logo.png",
    "Filename": "logo.png",
    "File Type": "png",
    "Thumbnail": "/connectors/[lang]/filemanager.[lang]?mode=getimage&path=%2Flogo.png&time=1463130804&thumbnail=true",
    "Protected": 0,
    "Properties": {
      "Date Created": null,
      "Date Modified": "05/05/2016 11:38:42",
      "filemtime": 1462441126,
      "Height": 14,
      "Width": 14,
      "Size": 384
    },
    "Error": "",
    "Code": 0
  }

@jlaustill
Copy link
Contributor Author

Also, it is ignoring my fileRoot and always sending me "/". This is my settings that have always worked with previous versions
"serverRoot": false,
"fileRoot": "/filemanager",
"fileRootSizeLimit": false,
"baseUrl": false,

@jlaustill
Copy link
Contributor Author

So I moved onto downloading the file, and oh boy. What is this crazyness? It's checking for code == 0 on a downloaded file? So I have to check for a query force === 'true' and either respond with {Code: 0} or the actual file. Then it's just setting windows.location.href = the entire endpoint, so I have to set weird headers.

This works, but are there plans to change it? Sorry for the bombardment, I'm working on this fulltime until it's done :)

@psolom
Copy link
Owner

psolom commented Aug 12, 2016

  1. If you are talking about "getfolder" response then it's not an array, but an object which contains file and folder objects. I agree, the description in API for this method a bit confusing and I'm going to fix it, but the code snippet that you can see in the API is correct. Here is how it is:
{
  "/userfiles/audio.mp3": {
    /* file data */
  },
  "/userfiles/image.jpeg": {
    /* file data */
  }
}

Btw, you forgot to add "Preview" key in your response for each item. It was discussed and approved just a few days ago so it has not yet been added to the API. Check this thread for the details and, of course, I am going to describe it in API.

  1. The current version tends to separate client-side from server-side parts as much as possible. So now fileRoot is an option for the server-side solely and doesn't affects in js file. Check it here as an example.

If you want to change web path to the connector, use fileConnector option as @fabriceci suggested.
If you want to change web path which should be treated to display files which you recieve in your response, use baseUrl option. It will be concatenated like baseUrl + file["Thumbnail"] or baseUrl + file["Preview"]

  1. Yeah, download logic looks complicated. It was done in that way to check for errors at the step 1 and display them to user if exist, and force download at the step 2 if there are no errors. I have no plans to change it currently, but I always appreciate any help how to improve the code. You can come with a new PR to suggest a better solution if you have.

Thanks for your efforts in nodejs connector!

@fabriceci
Copy link
Contributor

  1. I noticed this behaviour for download, the first Ajax call seems useless. Like the other calls, the server should send an error object if an error was thrown. (I don't look but it seem easy.)

@jlaustill have you already implemented the getParams/postParams, if not, do you plan to do it?

@psolom
Copy link
Owner

psolom commented Aug 12, 2016

@fabriceci Ok, I will take a look deeply.

@jlaustill
Copy link
Contributor Author

jlaustill commented Aug 12, 2016

Fabriceci,

I have not implemented the getParams/postParams at all. I will let you do that if you want :)

servocoder,

  1. I will go ahead and implement getfolder that way, but it is HIGHLY non-standard, so I would recommend taking a look at JSON API STANDARDS and planning for future changes to move closer to the standard. In this case that would mean returning getfolder as an array inside a data element, like so
{
    data: [
        ...
    ]
}

and errors inside an errors array exactly the same way. This has many advantages in javascript, but we can discuss them later, I'll stick to the current implementation for now :) Also, I didn't even notice the preview attribute, so I'll check that out today

  1. (2, why are numbers messed up? haha)I don't understand this at all, I will have to check out your link and step through the code. The documentation for these attributes has never made sense to me, and honestly, I got it working in my current project by trial and error :)
  2. Fabriceci is right on here, the first ajax call is useless. There are better ways to handle this in future as well, but for now I would recommend sticking with how it is and getting everything working. Once we get to a point that everything works we can work through issues like this one at at time as improvements. One step at a time lol

Lastly, I've been giving some thought to the config file. As it is, with the api and ui on seperate servers, it looks like I'll have to do an ajax request back to origin to grab the config file for every single request. This isn't great, it opens up to spoofing attacks. I would recommend maybe in future seperating the config file into two, server side config, and ui side config. For now, I may just require a second copy of the config on the api server for the nodejs connector, it's just as messy as an http request back to origin though. This issue doesn't exist when api and ui are on the same server because the connector can just open the file directly.

@jlaustill
Copy link
Contributor Author

Ok, after today I am mostly functional. I have the nodejs api browsing, uploading, renaming, moving, deleting, and adding folders. I still need to do the replace and I ran into a few new issues :)

  1. It is returning absolute urls to tinymce instead of the fileConnector path, so obviously that will need fixed in the ui
  2. The preview load is also ignoring the fileConnector path and trying to load images from the ui path
  3. It is kind of annoying the way it passes information for moving and renaming. I wouldn't call this an issue, just food for thought for future work. For renaming, it would be great if it passed either matching paths with a new name, or just the names and the path separately. But I have it working as is, so not an issue, just not ideal I would think.

I think one more good day on this and I'll have it ready to upload so you all can start looking at it. The QA process is going to be crazy because I'm writing this insanely fast. After two days it's 397 lines of code...

@psolom
Copy link
Owner

psolom commented Aug 13, 2016

Hi @jlaustill

Answer for 1 and 2 points:

As FM was initially designed to be utilized for web servers it always tends to return absolute path for a preview file, thumbnail or WYSIWYG editor. PHP connector is designed to return fileConnector path only if userfiles folder is located beyond document root folder. We could modify this logic like:

$beyondDocRoot = (stripos(realpath($this->path_to_files), realpath($this->doc_root)) !== 0) ||
/* your logic here to define whether connector has to return fileConnector path instead of absolute web path  */

We could add condition to check it by domain name or some other way. Do you have any proposals?

Another way is to create new option in config file to force fileConnector path to return. But it would be better to keep this logic inside connector if possible.

@psolom
Copy link
Owner

psolom commented Aug 13, 2016

As to splitting config file to UI and server files. I like this idea and some basics have been implemented for PHP connector in fact. There is a config file which contains server specific config solely. The same thing for AWS S3 plugin, because it's not safe to keep AWS credentials in json file for sure.

As far as I can see FM was initially desined to keep all config options in one place because it's extremely convenient for users. On the other hand there are some options defined in json file, which are relate to server-side exclusively, for instance fileRoot and serverRoot. It may be reasonable to move them to separate config file due to security concerns. But what about fileConnector option? It's utilized for both UI and connector files. The same with many other options. We must protect FM against vulnerabilities, but try to keep initial ideas.

@jlaustill
Copy link
Contributor Author

I'm going to respond in two separate responses here to try to keep this less confusing :)

I think I'm beginning to see how this came to since FM started as a totally all in one thing. To make this work with nodejs, I HAVE to seperate the api logic from the ui logic 100%. The ui will have zero idea about the path on the api server. It's also best practice to keep these separate, and I think we can do this with very little interruption to working code. I will try to draw this out in an example to make it as clear as possible.

API server -> http://api.com
UI server -> http://ui.com

Our api server exposes an endpoint /filemanager, so the fileConnector will become http://api.com/filemanager . The ui sends a request for getfolder(maybe not the easiest example) and the api responds with 3 files as such
/folder/file1.jpg
/folder/file2.doc
/folder/file3.png

These are stored "somewhere" on the api server, but the ui doesn't have any way to know, or care because the ONLY way the ui can access them is through the endpoint. So lets say the api saves them in /var/share/filemanager/files/folder/file1.jpg etc etc. It generates previews in /var/share/filemanager/previews/folder/file1.jpg etc. It then returns the following

path -> preview
/folder/file1.jpg -> /preview/folder/file1.jpg
/folder/file2.doc -> /preview/folder/file2.doc
/folder/file3.png -> /preview/folder/file3.png

The ui can ONLY get these files from its endpoint because it has zero physical access to the api server, so for the first preview it needs to ask for
http://api.com/filemanager?mode=download&path=/preview/folder/file1.jpg&etc=etc...

This can work exactly the same way for the normal php connector on the same server, there is no reason it has to work differently, the fileConnector will simply be the local server. I see all over the code in filemanager.js that it calculates this over and over. I would change the fileConnector upon loading the config file and if it's set to false, just replace the calculated url into once and only to it once, then everywhere in the UI code you can always just reference the fileConnector + '?mode=download...'

This would move you much closer to having a clear separation of ui and api.

@jlaustill
Copy link
Contributor Author

There is zero reason that the api should even know the fileConnector, it means nothing to it whatsoever, that is a client side config only. If the current implementation is using it, it clearly violates the separation of concerns. The api should only care about its fileRoot location, and the UI should never even know that location, ever. Even in the case of the php connector running on the same server, the ui should always ask the connector for its files, and the the connector should always return a relative path to itself. The good news is that the way the code is currently written, this will be very easy.

I can work on a roadmap for making this happen, and it will only take a few days for the ui code and the nodejs code. I don't know php at all, so I can't speak to that :)

@jlaustill
Copy link
Contributor Author

@fabriceci I did up a quick example of what we were talking about consolidating ajax requests and extending with getParams. Check out repo and search filemanager.js for _$.apiGet to find the definition and usage I put in place. Let me know what you think.

@psolom
Copy link
Owner

psolom commented Sep 11, 2016

Hi @jlaustill
How are you going with the NodeJS connector?
I want to notify you that I have made many things at the "dev" branch from the list we had discussed: config file separation, preview with absolute and connector paths and others mentined in the #41.

I have created "api" section in config file and prepared "requestParams" param inside of it. Initially I though to set all params for both "get" and "post"(put) methods to "requestParams" and now I tend to split it to two different options to handle each method separately. Is that will be enough for your idea of "apiGet" and "apiPut" options?

@psolom psolom removed this from the 1.1.0 milestone Nov 4, 2016
@psolom
Copy link
Owner

psolom commented Nov 4, 2016

Hi @jlaustill
I have released v2.0.0 with a lot of modifications and improvements, including many of your suggestions. You can see new API is standartized and completely rewritten. I am wondering whether you are going to work on NodeJS connector? I would glad to consider you as NodeJS connector maintainer. Looking forward for your reply.

@psolom
Copy link
Owner

psolom commented Apr 7, 2017

Closed since there is no activity for a long time. If you want to implement nodejs connector I will gladly reopen this issue.

@psolom
Copy link
Owner

psolom commented Nov 24, 2017

The latest commit should refer to issue #210 (mismatch)

@dekyfin
Copy link

dekyfin commented Mar 14, 2018

I have modified the NodeJS connector from the repo into a new project https://github.com/dekyfin/RichFilemanager-NODE .

@psolom
Copy link
Owner

psolom commented Mar 14, 2018

@dekyfin Awesome! Thanks a lot.

Have you checked all API functions to work well?

@dekyfin
Copy link

dekyfin commented Mar 14, 2018

All API functions have been implemented with the exception of the following

  1. seekfolder
  2. summarize
  3. extract

@psolom
Copy link
Owner

psolom commented Mar 14, 2018

Ok, I will include link to your repo into the wiki.

Are you going to implement the rest of API methods?

@psolom
Copy link
Owner

psolom commented Mar 15, 2018

Btw, I have got been reported with a following issue: #300

Сan you confirm or deny this statement?

@dekyfin
Copy link

dekyfin commented Mar 15, 2018

The issue hasn't yet occurred in my instance. I'll go through the code to check it

@psolom
Copy link
Owner

psolom commented Mar 17, 2018

Thanks.
Btw, I have added link to your repo in the Wiki

@hathemi
Copy link

hathemi commented Dec 24, 2018

Hi there, i tried to run your project using nodejs coonector https://github.com/dekyfin/RichFilemanager-NODE . But, it doesn't work to me. i always get this error TypeError: Cannot read property 'trim' of undefined in rich-filemanager-node\index.js
i can't figure out a solution for this !!!!!!

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

No branches or pull requests

5 participants