-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feanil/api docs #32971
feanil/api docs #32971
Conversation
feanil
commented
Aug 9, 2023
- docs: Add some basic docs on the Rest API.
- docs: Add some docs for using the REST API
6705522
to
895693a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding these docs @feanil !
Couple of nits and one request, if you have time.
docs/concepts/rest_apis.rst
Outdated
OAuth2 Application (the application is associated with your user). | ||
|
||
JWTs by default expire every hour so when they expire you'll have to get a new | ||
one before you can call the API again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind to add a code sample to the how-to that shows how you detect that a JWT is expired, and how to refresh it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pomegranited: Refreshing tokens is a different idea which I am guessing Feanil was ignoring for the sake of this document. He is simply asking them to get a new one using the same original credentials, which I think is fine. I'm not sure if it will add confusion or not to mention that there is also a way to refresh using the refresh token, but that adds complexity, and would probably go in the other how to anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up adding a reference section of useful code that includes using refresh tokens if you are working with a public client (you don't get refresh tokens with your JWT for Confidential
clients). Take a look at the new auth_code_samples.rst
.
docs/concepts/rest_apis.rst
Outdated
OAuth2 Application (the application is associated with your user). | ||
|
||
JWTs by default expire every hour so when they expire you'll have to get a new | ||
one before you can call the API again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pomegranited: Refreshing tokens is a different idea which I am guessing Feanil was ignoring for the sake of this document. He is simply asking them to get a new one using the same original credentials, which I think is fine. I'm not sure if it will add confusion or not to mention that there is also a way to refresh using the refresh token, but that adds complexity, and would probably go in the other how to anyway.
There is probably a lot more to say but add a quick How-To and concept docs that we can build on.
895693a
to
88096cc
Compare
[inform] This somewhat related doc exists: https://github.com/openedx/edx-platform/blob/master/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/testing_manually.rst. I also created an issue in edx-platform, because this how-to is not in the published docs: #33047 |
This should be ready for re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @feanil.
Typos and other small fixes. Co-authored-by: Robert Raposa <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @feanil
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |