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

Add tests for authentication #1170

Merged
merged 8 commits into from May 24, 2021
Merged

Add tests for authentication #1170

merged 8 commits into from May 24, 2021

Conversation

rikhuijzer
Copy link
Collaborator

@rikhuijzer rikhuijzer commented May 17, 2021

In #529, authentication was added. This PR adds some basic tests.

The benefit of these tests is that they are much easier to verify than the implementation in src/webserver/Static.jl.

test/Configuration.jl Outdated Show resolved Hide resolved
@rikhuijzer
Copy link
Collaborator Author

The failing test is due to a front end test being flaky. The back end tests all pass.

@fonsp

This comment has been minimized.

@rikhuijzer
Copy link
Collaborator Author

rikhuijzer commented May 19, 2021

@fonsp, you said that I could either disable auth and check that or verify auth with secret. I did the latter, because that is Pluto's default. Unfortunately, the commit I just pushed fails on two paths and I'm unsure whether this is meant to be or accidentally. With the logging (@show) as added in the commit, the output for the access_granted tests contains

url = "http://127.0.0.1:1238/?secret=sIT0NIxe"
r.status = 200
url = "http://127.0.0.1:1238/edit/?secret=sIT0NIxe"
r.status = 200
url = "http://127.0.0.1:1238/foo/?secret=sIT0NIxe"
r.status = 404
url = "http://127.0.0.1:1238/new/?secret=sIT0NIxe"
r.status = 403
authentication: Test Failed at /home/rik/git/Pluto.jl/test/Configuration.jl:83
  Expression: access_granted(url)
[...]
url = "http://127.0.0.1:1238/notebookfile/?secret=sIT0NIxe"
r.status = 400
url = "http://127.0.0.1:1238/notebookexport/?secret=sIT0NIxe"
r.status = 400
url = "http://127.0.0.1:1238/open/?secret=sIT0NIxe"
r.status = 400
url = "http://127.0.0.1:1238/sample/Interactivity.jl/?secret=sIT0NIxe"
r.status = 500
authentication: Test Failed at /home/rik/git/Pluto.jl/test/Configuration.jl:83
  Expression: access_granted(url)
[...]

Could you let me know what is the desired behaviour for these failing routes? I don't know enough about Pluto to make a good decision about this.

@fonsp
Copy link
Owner

fonsp commented May 19, 2021

My first reaction is to try removing the trailing slashes, i.e. host/new?secret=asdf instead of host/new/?secret=asdf, etc. Any remaining failing URLs are probably because they only work if you provide an existing notebook ID. e.g. the host/notebookfile?id=lkjsdflkjsd&secret=jasdfj endpoint returns the file of the given notebook.

Feel free to take a look, but I can also finish this myself! I think you want to add a small notebook to the session before testing the routes, see our other tests on how to do that.

@fonsp
Copy link
Owner

fonsp commented May 19, 2021

The sample URL should be URL-encoded. Open the Pluto main menu, go to samples and right click on one of the links to see what it looks like. (You can also use a running Pluto instance to take a look at the /edit url and such.) Note that you won't see the secret there because normal Pluto usage keeps it in cookies, instead of in each URL.

@rikhuijzer
Copy link
Collaborator Author

@fonsp, thanks for the feedback. I've applied most of it.

I think you want to add a small notebook to the session before testing the routes, see our other tests on how to do that.

Could you pick this one up? I didn't figure it out and it's then probably quicker if you pick it up in a commit then we talking about it some more :)

@fonsp fonsp merged commit 37f78a8 into fonsp:main May 24, 2021
@fonsp
Copy link
Owner

fonsp commented May 24, 2021

@rikhuijzer dankjewel!

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.

None yet

3 participants