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

Authorization checks in server.py #5386

Merged
merged 8 commits into from
Aug 21, 2023

Conversation

ndmlny-qs
Copy link
Contributor

This commit adds a single parameter used in the
config.authorize_callback that allows the user supplied method to check if an app user is authorized to view the requested app at the given path.

Resolves #3179

This commit adds a single parameter used in the
`config.authorize_callback` that allows the user supplied method to
check if an app user is authorized to view the requested app at the
given path.

Resolves holoviz#3179
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #5386 (56893bb) into main (229f90e) will decrease coverage by 0.05%.
Report is 14 commits behind head on main.
The diff coverage is 36.84%.

@@            Coverage Diff             @@
##             main    #5386      +/-   ##
==========================================
- Coverage   72.69%   72.65%   -0.05%     
==========================================
  Files         274      274              
  Lines       39862    39922      +60     
==========================================
+ Hits        28978    29005      +27     
- Misses      10884    10917      +33     
Flag Coverage Δ
unitexamples-tests 72.65% <36.84%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
panel/config.py 68.90% <ø> (ø)
panel/io/server.py 66.82% <36.84%> (-1.07%) ⬇️

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@philippjfr
Copy link
Member

Thanks, this makes sense but it's also not backward compatible. Could we maybe add a try/except or explicitly check the signature of the function to make sure the old signature is still supported?

Also we should update the docstring of the config. authorize_callback parameter.

@ndmlny-qs
Copy link
Contributor Author

thanks @philippjfr I'll make those changes and update the docstring. If it makes sense, I'll also add a blurb to the documentation page with an example.

- `panel/config.py`'s docstring updated to include the ability to check
  a user is authorized for the requested path.
- `doc/how_to/authentication/user_info.md` updated giving an example
  with the new authentication parameter.
- `panel/io/server.py` updated to include a check for the number of
  parameters needed for `pn.config.authorize_callback`. If only one is
  given, then it checks specifically for the username authorization. If
  two is given, then it will check the authorization for both the
  username and the requested path by the user. The default is to fail
  authorization.
panel/io/server.py Outdated Show resolved Hide resolved
Moves the check for when `config.authorize_callback` is not `None`
before any use of `inspect` is done on the object.
panel/io/server.py Outdated Show resolved Hide resolved
ndmlny-qs and others added 3 commits August 17, 2023 15:47
The previous commit did not allow an authorized user to see the
requested page, if a `config.authorize_callback` was given due to faulty
logic. This commit moves page creation above all authorization checks,
and only modifies the page object if a `config.authorize_callback` is
given, and if the user (or user and requested path) are authorized.
@philippjfr philippjfr merged commit b6d6b34 into holoviz:main Aug 21, 2023
@ndmlny-qs ndmlny-qs deleted the issue3179/admin-custom-url-auth branch August 23, 2023 13:36
@el-abcd
Copy link

el-abcd commented Sep 11, 2023

Hi,

I was trying to add oath to a small panel app (to run in "google cloud run").
It seems like with panel==1.2.2 that the authorize() method MUST have BOTH parameters?

def authorize(user_info, request_path):

i.e. when I tried
def authorize(user_info):
which is working for panel=1.2.1 I see:

2023-09-11 14:07:44,242 Uncaught exception GET /220514_01_awesome_panel (::1)
HTTPServerRequest(protocol='http', host='localhost:5006', method='GET', uri='/220514_01_awesome_panel', version='HTTP/1.1', remote_ip='::1')
Traceback (most recent call last):
File "/Users/ericlee/.pyenv/versions/3.10.4/lib/python3.10/site-packages/tornado/web.py", line 1704, in _execute
result = await result
File "/Users/ericlee/.pyenv/versions/3.10.4/lib/python3.10/site-packages/panel/io/server.py", line 517, in get
raise RuntimeError(
RuntimeError: Authorization callback must accept either one or two arguments.

Perhaps the check here:
b6d6b34#diff-aaf5dc2e57ff38e7660dbe2c7e08cd2b8a34242b382d40f049987b95f605e628R513
should be changed from
if len(auth_params) == 2:
to something like
if len(auth_params) in [1,2]:
?

Regards,
Eric

@maximlt
Copy link
Member

maximlt commented Sep 11, 2023

Hi @el-abcd and thanks for reporting that. Indeed it seems there's a bug! I believe your suggestion not to be quite the right one, when len(auth_params) == 1 you do not want to expand to expand auth_args. The logic could be rewritten as:

                    if len(auth_params) == 1:
                        auth_args = (state.user_info,)
                    elif len(auth_params) == 2:
                        auth_args == (state.user_info, self.request.path)
                    else:
                        raise RuntimeError(
                            'Authorization callback must accept either one or two arguments.'
                        )

Would you be interested in making that change in a PR?

@el-abcd
Copy link

el-abcd commented Sep 11, 2023

Hi Maxime,

Yes, I can give that a try.
It might take me a couple days to get a development environment setup, etc. but I will see if I can make the PR, etc.

Eric

@ndmlny-qs
Copy link
Contributor Author

@el-abcd I see the problem, and can make a fix tomorrow. thanks for the report

@ndmlny-qs ndmlny-qs mentioned this pull request Sep 12, 2023
@ndmlny-qs
Copy link
Contributor Author

see #5504

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.

Enable serving admin site at custom and secure url
5 participants