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

[enh] add settings option to enable/disable search formats #99

Merged
merged 2 commits into from May 28, 2021

Conversation

return42
Copy link
Member

@return42 return42 commented May 26, 2021

What does this PR do?

  • eff337a [enh] add settings option to enable/disable search formats

How to test local

In settings.yml edit

search:
    ...
    formats: [html]

The denied formats should no longer available in the link box:

image

Request a format: ( http://127.0.0.1:8888/search?q=foo&format=csv ) should result in a 403

image

Related issues

Closes #95

@dalf
Copy link
Member

dalf commented May 26, 2021

Is it possible to have 192a6f4 / e9a38ff and e9a38ff in another PR ?


About e9a38ff

Instead of access_denied.search/format , what do you think of search.formats with a default value of [csv, json, rss] (since html is always included) ?

@return42
Copy link
Member Author

Is it possible to have 192a6f4 / e9a38ff and e9a38ff in another PR ?

if you want .. and we review this PR first.

what do you think of search.formats with a default value of [csv, json, rss] (since html is always included) ?

I can change to search.formats.

@dalf
Copy link
Member

dalf commented May 26, 2021

About the get_value function: should the PR 2501 from searx should be merged?

@return42
Copy link
Member Author

should the PR 2501 from searx should be merged?

Hm 2501 LGTM .. but can't rate it in deep .. to get #95 done I vote to merge 2501 in another PR.

@dalf
Copy link
Member

dalf commented May 26, 2021

I vote to merge 2501 in another PR.

Yes sure.

@return42 return42 changed the title [enh] add ability to disable RSS/JSON/CSV [enh] add settings option to enable/disable search formats May 27, 2021
@return42 return42 force-pushed the webapp-misc branch 3 times, most recently from eff337a to 23f6c4c Compare May 27, 2021 15:36
@return42
Copy link
Member Author

what do you think of search.formats with a default value of [csv, json, rss] (since html is always included) ?

All formats can be (de)selected / including html:

23f6c4c#diff-e95e3f454925bc2344d3cf538950b11a1255935b47a609c9589e6ac33d74802fR21

HINT If admins maintain their own settings.yml file, they need to add these formats to the search section:

 search:
      ...
      formats: [html, csv, json, rss]

Otherwise they will get only "403 Forbidden". The robot test is one example, where I needed to add this line:

23f6c4c#diff-ceeab343071eef75af2f6bf9d5fa7a3f73802313f03b519291e75ba55f1c91d2

searx/webapp.py Outdated
@@ -686,6 +691,9 @@ def search():
if output_format not in ['html', 'csv', 'json', 'rss']:
output_format = 'html'

if output_format not in get_value(settings, 'search', 'formats', default=[]):
Copy link
Member

@dalf dalf May 27, 2021

Choose a reason for hiding this comment

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

default=[] --> default=['html', 'csv', 'json', 'rss'] otherwise it will break existing setup (without search.formats).

Looking at the code above:

OUTPUT_FORMATS = ['html', 'csv', 'json', 'rss']

...
    allowed_formats = get_value(settings, 'search', 'formats', default=OUTPUT_FORMATS)
    output_format = request.form.get('format')
    if output_format not in OUTPUT_FORMATS:
        # undefined or unknown format: fallback to the first allowed format
        # default setting: 'html' 
        output_format  = allowed_formats[0]
    elif output_format not in allowed_formats:
        # the format is valid but not allowed: error 403
        flask.abort(403)

The fallback part keeps the API compatibility (if output_format not in OUTPUT_FORMATS)

Copy link
Member Author

Choose a reason for hiding this comment

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

otherwise it will break existing setup (without search.formats).

good idea to set the default including all formats .. but should we set the default in the code above or is it better placed nearby loading the settings?

Next thought: is it good to have defaults in settings.yml and other defauts in the code (e.g. nearby loading settings).

What I want is a single point of definitions .. ATM this single point is the settings,yml .. should we really add a new place where defaults could be located? (hope I was clear)

Copy link
Member

Choose a reason for hiding this comment

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

placed nearby loading the settings

I don't understand the location.

A simple solution for now: a the top of webapp.py (?)


With the searx PR 2501:

  • the default value is in searx/settings_defaults.py
  • allowed_formats = get_value(settings, 'search', 'formats', default=OUTPUT_FORMATS) --> allowed_formats = settings['search']['formats'] (we are sure there is a value)

As discuss in this PR, a Settings class would be better, then the Preferences can use a Settings instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the location.

I mean a file like ..

With the searx PR 2501:
the default value is in searx/settings_defaults.py

I think a solution like PR2501 will help ...

As discuss in this PR, a Settings class would be better, then the Preferences can use a Settings instance.

PR2501 also implements static_path handling .. for me it is a bit hard to merge all this into tthe current searxng without to fail ... If you have time, do you like to merge this PR? If so, I would vote to implement a class.

searx/utils.py Outdated Show resolved Hide resolved
return42 added a commit to return42/searxng that referenced this pull request May 27, 2021
In a comment [1] dalf suggested to avoid a recursion of get_value()

[1] searxng#99 (comment)

Suggested-by: Alexandre Flament <alex@al-f.net>
Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
Access to formats can be denied by settings configuration::

    search:
        formats: [html, csv, json, rss]

Closes: searxng#95
Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
In a comment [1] dalf suggested to avoid a recursion of get_value()

[1] searxng#99 (comment)

Suggested-by: Alexandre Flament <alex@al-f.net>
Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
@return42
Copy link
Member Author

searx/webapp.py Show resolved Hide resolved
@dalf dalf merged commit 83ccf7b into searxng:master May 28, 2021
@return42 return42 deleted the webapp-misc branch May 28, 2021 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to disable RSS/JSON/CSV
2 participants