Skip to content

Conversation

@GalMunGral
Copy link
Collaborator

  • Related: ncsa/idot-pma#173
  • POST /login and POST /auth/login have the exact same logic. @longshuicy suggested deprecating the POST /login endpoint.
  • Applied the same logic for refresh tokens as used in the authorization code flow (GET /auth) so that access tokens obtained through the password flow (POST /auth/login) can also be refreshed by GET /auth/refresh_token.

@GalMunGral GalMunGral requested a review from max-zilla as a code owner October 24, 2024 20:47
@CLAassistant
Copy link

CLAassistant commented Oct 24, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and tested.
One thing I noticed is the "deprecated" not seems to work? I can still POST to the /api/v2/login endpoint with no issue. And i didn't see any notification either.
Could you look into that? if it's not straightforward, maybe we can just delete this endpoint. But we might be careful, a quick search I found there are some scripts (in the scripts folder) still uses this endpoint.

@longshuicy
Copy link
Member

longshuicy commented Oct 25, 2024

After we discussed, could you try below: @GalMunGral

  1. Make a utility function of that tokenDB logic. You can put it in backend/app/routers/utils.py.
  2. Then revert the deprecate. We might want to keep both endpoints. (so you don't need to worry about making deprecating work)
  3. Put the logic in /auth/login and /login

@max-zilla if I've missed anything, please chime in.

@GalMunGral
Copy link
Collaborator Author

After we discussed, could you try below: @GalMunGral

  1. Make a utility function of that tokenDB logic. You can put it in backend/app/routers/utils.py.
  2. Then revert the deprecate. We might want to keep both endpoints. (so you don't need to worry about making deprecating work)
  3. Put the logic in /auth/login and /login

@max-zilla if I've missed anything, please chime in.

Will do!

@max-zilla max-zilla requested a review from longshuicy October 30, 2024 14:04
@max-zilla
Copy link
Contributor

@longshuicy i will take a look at this also but it's ready for another look, thanks.

Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and functionality works. Approve.

@lmarini lmarini merged commit 15c47d2 into release/v2.0-beta-3 Nov 5, 2024
@lmarini lmarini deleted the 1204-save-refresh-token-when-using-password-grant-flow branch November 5, 2024 15:23
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.

6 participants