Skip to content
This repository has been archived by the owner on Jul 30, 2024. It is now read-only.

SECURITY_UNAUTHORIZED_VIEW doesn't default to None #724

Open
nalzok opened this issue Oct 25, 2017 · 4 comments · May be fixed by #726
Open

SECURITY_UNAUTHORIZED_VIEW doesn't default to None #724

nalzok opened this issue Oct 25, 2017 · 4 comments · May be fixed by #726

Comments

@nalzok
Copy link

nalzok commented Oct 25, 2017

Hi there!

The document says SECURITY_UNAUTHORIZED_VIEW defaults to None, but actually it's default to lambda: None.

def _get_unauthorized_view():
    view = utils.get_url(utils.config_value('UNAUTHORIZED_VIEW'))
    if view:
        if callable(view):
            view = view()
        else:
            try:
                view = url_for(view)
            except BuildError:
                view = None
        utils.do_flash(*utils.get_message('UNAUTHORIZED'))
        return redirect(view or request.referrer or '/')
    abort(403)

Since lambda: None is interpreted as true, by default _get_unauthorized_view() won't abort(403), but the docs says it should.

@jirikuncar
Copy link
Contributor

@sunqingyao thank you reporting. Indeed it looks like an inconsistency between docs and code. Do you prefer the default to abort(403) or redirect to referrer or /?

@nalzok
Copy link
Author

nalzok commented Oct 26, 2017

Personally I prefer a simple abort(403), because

  1. URLs to protected views shouldn’t be presented to users without certain roles at the first place, so under normal circumstances, a user won’t get a 403 error.
  2. Redirecting without a proper message can be confusing, whereas 403 always means Unauthorized.
  3. This makes testing easier: just assert resp.status_code to be 403.

@jirikuncar
Copy link
Contributor

Let's change the lambda: None to None then.

@nalzok
Copy link
Author

nalzok commented Oct 27, 2017

Great! :smiles:

@jirikuncar jirikuncar linked a pull request Oct 27, 2017 that will close this issue
jirikuncar added a commit to jirikuncar/flask-security that referenced this issue Jan 5, 2018
* Changes default to `None` to be consistent with documentation.
  (closes pallets-eco#724)
jwag956 referenced this issue in pallets-eco/flask-security May 8, 2019
* Changes default to `None` to be consistent with documentation.
  (closes #724)
jwag956 referenced this issue in pallets-eco/flask-security May 8, 2019
* core: fix default for UNAUTHORIZED_VIEW

* Changes default to `None` to be consistent with documentation.
  (closes #724)

* Fix url generation of UNAUTHORIZED_VIEW
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging a pull request may close this issue.

2 participants