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

feat: validate auth header in session request if provided #1251

Merged

Conversation

theborakompanioni
Copy link
Contributor

@theborakompanioni theborakompanioni commented Apr 19, 2022

Closes #1244.

Before this PR, the /session endpoint ignores a potentially provided token in the Authorization header.
After this PR, the /session endpoint validates a token provided in the Authorization header and will respond with 401 Unauthorized if the token is invalid.

See #1244 on why this might be beneficial for API users. Summary: It seems like a good alternative to polling an authenticated endpoint and a less error-prone solution than relying on websocket server closes.

@theborakompanioni theborakompanioni marked this pull request as ready for review April 19, 2022 12:23
@theborakompanioni
Copy link
Contributor Author

I guess the tests failing in python3.7 has nothing to do with the changes in the PR.

@AdamISZ
Copy link
Member

AdamISZ commented Apr 19, 2022

@theborakompanioni ignore that CI test fail, it's a known bug that randomly we can get that relay fee failure.

@AdamISZ
Copy link
Member

AdamISZ commented Apr 19, 2022

Yeah :) So, utACK for now, looks correct.

@theborakompanioni
Copy link
Contributor Author

Tested successfully with joinmarket-webui/jam#223 in case you are wondering. Wanted to write tests but the first few attempts failed. I cannot get my local environment to execute the tests successfully : /

Using python3.10 here - downgrading to python3.9 heavily messes up my system. : /

PS: New to python in general. Hopefully not introducing bugs with so little code.

@AdamISZ
Copy link
Member

AdamISZ commented Apr 19, 2022

No worries. Feel free to write down any details of what's going wrong here, if you like. Not only that we can help you but also your experiences might tell us things, especially about 3.10, I'm personally always running 3.8 still and there may be important things to address there.

@kristapsk
Copy link
Member

kristapsk commented Apr 19, 2022

Python 3.10 should work, such tests pass in #1218 (failure with insufficient fee there is known issue happening from time to time, not related to Python versions).

@theborakompanioni
Copy link
Contributor Author

Updated API docs as well. Hope the optional security spec is working (it should be, according to OAI/OpenAPI-Specification#14)

No worries. Feel free to write down any details of what's going wrong here, if you like. Not only that we can help you but also your experiences might tell us things, especially about 3.10, I'm personally always running 3.8 still and there may be important things to address there.

I will revisit this at a later point and hopefully can provide useful insights. 🙏

@AdamISZ
Copy link
Member

AdamISZ commented Apr 21, 2022

Added four checks of /session with a valid token, an invalid token and no token into the existing test flow. I think it does what we want. Once you're happy with this we can merge @theborakompanioni

@theborakompanioni
Copy link
Contributor Author

Added four checks of /session with a valid token, an invalid token and no token into the existing test flow. I think it does what we want.

Thank you, Adam. Great additions. I will strive for all future changes to include tests so you don't have too much additional work on your plate.

Once you're happy with this we can merge @theborakompanioni

Very happy 🙏

@AdamISZ AdamISZ merged commit 5bf64ed into JoinMarket-Org:master Apr 22, 2022
@theborakompanioni theborakompanioni deleted the session-auth-if-present-1244 branch July 19, 2022 08:55
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.

jmwalletd: Check token in route /session if present
3 participants