Skip to content
This repository has been archived by the owner on Feb 19, 2021. It is now read-only.

Enable CORS for localhost #387

Merged
merged 4 commits into from
Aug 17, 2018

Conversation

LukaszSolo
Copy link
Contributor

Hi,

I'm working on the new (minimalistic for now) frontend. As I don't want to change anything in the existing Django app (for now at least), I'm running it as a separate app that consumes the Django API.

To make it a replacement for current UI, I needed to do some tiny changes:

  • enable CORS for localhost calls
  • add Filter to allow API calls that can select Documents without any tag

I don't foresee any issues with the above, but if you think it's not a good idea, let me know!

- enable CORS for localhost calls
- add Filter to allow API calls that can select Documents without any tag
@danielquinn
Copy link
Collaborator

This looks pretty good, and I'm stoked about a new frontend! Travis has pointed out that you need to use == instead of =, so that needs to change before I can merge it for sure.

I have one other question though. As some of our users host Paperless on a domain other than localhost and/or a different port, can this be made a little more accommodating? I'm thinking maybe use what you've got as the default, and allow for using a different value by setting PAPERLESS_CORS_WHITELIST?

 CORS_ORIGIN_REGEX_WHITELIST = (r'^(https?:\/\/)?localhost(:[0-9]{4})?$', ) 

@LukaszSolo
Copy link
Contributor Author

Hey, thanks for a very quick response!

I'm a little stumped about the == as the settings file should use = . I think I know the issue and I'll double check in a couple of hours.

I see your point about the PAPERLESS_CORS_WHITELIST I think it's a good idea, let me add that too.

Thanks!

@danielquinn
Copy link
Collaborator

requirements.txt uses a format where you have to set one of ==, >=, or something elaborate like <x.y,>=a.b. However, now that I think about it, in this project requirements.txt file is actually generated from the Pipfile anyway. So if you're familiar with pipenv (the new hotness in Pythonland) jus django-cors-headers = "<2.5,>=2.4" to Pipfile and then run pipenv lock && pipenv lock -r > requirements.txt.

Barring that, I can just do that right on master and accept the rest of your PR once we work out the WHITELIST question.

- fix requirements.txt
- change static CORS regex into configurable tuple list
@LukaszSolo
Copy link
Contributor Author

LukaszSolo commented Aug 16, 2018

Hi,

I fixed the requirements by adding missing =. I'm not too familiar with pipenv (at least for now ;) ) so I think it might be better to leave it to you if you have a moment.

I also removed the regex and replaced it with less-powerful, but harder to mess up list. It should cover most of the common use cases like a subdomain, different ports etc.

(edit) And a late fix for 80 characters.

CORS_ORIGIN_WHITELIST = ("localhost:8080")
_allowed_cors_hosts = os.getenv("PAPERLESS_CORS_ALLOWED_HOSTS")
if _allowed_cors_hosts:
CORS_ORIGIN_WHITELIST = tuple(_allowed_cors_hosts.split(","))
Copy link
Contributor

Choose a reason for hiding this comment

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

All the 4 lines are the same as... :
CORS_ORIGIN_WHITELIST = os.getenv("PAPERLESS_CORS_ALLOWED_HOSTS", "localhost:8080").split(",")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the code pattern of ALLOWED_HOSTS as I didn't want to introduce a different style of writing. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like @sbrunner's suggestion, as it's more conscise. I see what you mean about the ALLOWED_HOSTS though. We can fix that later. For now, having this in one statement is better, so let's go with that.

@danielquinn
Copy link
Collaborator

Looks good! Thanks for your contribution! (and your flexibility :-)) Merging now.

@danielquinn danielquinn merged commit fcd36c8 into the-paperless-project:master Aug 17, 2018
@LukaszSolo
Copy link
Contributor Author

Thank you! I'll let you know when I think the Vue frontend is in a state worth sharing :)

@stgarf stgarf mentioned this pull request Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants