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

Implement JWT authentication in REST API #1785

Merged
merged 29 commits into from
Oct 20, 2022
Merged

Conversation

ankur12-1610
Copy link
Contributor

@ankur12-1610 ankur12-1610 commented Jul 1, 2022

This PR basically involves the implementation of JWT authentication in Specter's REST API, I've divided this PR into different milestones:

  • Add jwt_token field in the UserMixin
  • Create basic endpoints( /v1alpha/token) which return the jwt_token as the JSON response.
  • Add BasicHTTPAuth to the endpoints
  • Implement TokenHTTPAuth on top of the BasicHTTPAuth
  • Add token life functionality, so that user can decide the life of the token generated
  • One time access functionality, which basically will limit user to see the token only once after generation

Demo (latest progress):

final-sob-demo.mp4

@netlify
Copy link

netlify bot commented Jul 1, 2022

Deploy Preview for specter-desktop-docs ready!

Name Link
🔨 Latest commit ded3eb8
🔍 Latest deploy log https://app.netlify.com/sites/specter-desktop-docs/deploys/63510a503689a40008a6dbbf
😎 Deploy Preview https://deploy-preview-1785--specter-desktop-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k9ert k9ert marked this pull request as draft July 1, 2022 13:10
@ankur12-1610
Copy link
Contributor Author

@k9ert can you review the changes?

@k9ert
Copy link
Collaborator

k9ert commented Jul 21, 2022

I've added some more comments. Please also fix the tests. Check the development documentation for something called pip-compile.

@k9ert
Copy link
Collaborator

k9ert commented Oct 6, 2022

If there is no hiccup, this should now be green. I've also refined the documentation. @ankur12-1610 currently no need to do anything on your side other than maybe reviewing my changes in the documentation.
@moneymanolis This is ready for review.

@ankur12-1610
Copy link
Contributor Author

ankur12-1610 commented Oct 6, 2022

Oh! @k9ert you updated the doc:sweat_smile:, I was doing it rn ;)
Thanks for it ;)

Also in the updated version, we've not described python requests any reason for that:thinking:?

Also the changes LGTM!

@k9ert
Copy link
Collaborator

k9ert commented Oct 6, 2022

Yeah, i was a bit impatient 😅

Also in the updated version, we've not described python requests any reason for thatthinking?

What's missing here ?
https://deploy-preview-1785--specter-desktop-docs.netlify.app/api/#python

@ankur12-1610
Copy link
Contributor Author

ankur12-1610 commented Oct 7, 2022

Yeah, i was a bit impatient sweat_smile

Also in the updated version, we've not described python requests any reason for thatthinking?

What's missing here ? https://deploy-preview-1785--specter-desktop-docs.netlify.app/api/#python

Oh yeah it is correct I thought the same curl should be in python requests as well didn't see the line you mentioned which says in python requests we won't use BasicAuth.

Also there is a minor typo over here https://deploy-preview-1785--specter-desktop-docs.netlify.app/api/#python:~:text=%3Ctoken%3E%20and-,%3Ctoekn_id%3E, it should be <token_id>.

@k9ert
Copy link
Collaborator

k9ert commented Oct 10, 2022

Typo fixed.

It might need an explanation: You don't "need" BasicAuth in python because we assume here, that people who are doing python are developing serious integration-solutions which are configurable. Configuration with a token which has been created upfront via ... remember: Ideally we should have done a frontend in order to create the token. We wanted to make it as simple as possible and therefore we skipped the UI and instead created an API endpoint for that as a first fix.

However this (BasicAuth) endpoint should only be used by an administrator of Specter in order to create the token which they then configure in some other (python-) application. But an admin will use curl for that, not python.

So that's the reason why we don't need the BasicAuth endpoint example in python (even though it would be possible).

Or looking it from the other point of view: If you WOULD implement a configuration where you configure Username/Password into the python application and the python application is getting a token and using that one, then the whole exercise would have been completely useless and no security gains would be there.
Therefore: Implementing the BasicAuth example in python is even bad practice.

@ankur12-1610
Copy link
Contributor Author

Typo fixed.

It might need an explanation: You don't "need" BasicAuth in python because we assume here, that people who are doing python are developing serious integration-solutions which are configurable. Configuration with a token which has been created upfront via ... remember: Ideally we should have done a frontend in order to create the token. We wanted to make it as simple as possible and therefore we skipped the UI and instead created an API endpoint for that as a first fix.

However this (BasicAuth) endpoint should only be used by an administrator of Specter in order to create the token which they then configure in some other (python-) application. But an admin will use curl for that, not python.

So that's the reason why we don't need the BasicAuth endpoint example in python (even though it would be possible).

Or looking it from the other point of view: If you WOULD implement a configuration where you configure Username/Password into the python application and the python application is getting a token and using that one, then the whole exercise would have been completely useless and no security gains would be there. Therefore: Implementing the BasicAuth example in python is even bad practice.

Got it, thanks for the info I didn't have any idea about this ;)

@k9ert
Copy link
Collaborator

k9ert commented Oct 14, 2022

We can soon merge this PR. I don't think there is anything left other than doing a normal bugfix release, probably on monday. Next week tuesday afternoon, we can merge this.

@moneymanolis
Copy link
Collaborator

There were some little changes of the rest api test in this #1920, I'd suggest to merge this PR first and then check whether the tests are still green (I guess so!) and then merge.

@k9ert k9ert merged commit 793a8c5 into cryptoadvance:master Oct 20, 2022
@k9ert
Copy link
Collaborator

k9ert commented Oct 20, 2022

@ankur12-1610 merged, thank you !

@moneymanolis
Copy link
Collaborator

Congrats! @ankur12-1610

@ankur12-1610
Copy link
Contributor Author

Yaaayy! 🎉 it was a great learning experience, thanks @k9ert and @moneymanolis 😄

@ankur12-1610 ankur12-1610 changed the title WIP: Implement JWT authentication in REST API Implement JWT authentication in REST API Feb 7, 2023
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.

3 participants