Skip to content

Conversation

@tianjing-li
Copy link
Collaborator

  • Removes previous SessionMiddleware usage, and get_session endpoint
  • Fixes the GET /auth_strategies endpoint
  • Modifies /login and /auth to return a JWT token on successful authentication
  • Adds config/routers.py file to define default or auth dependencies. These dependencies are set during app creation time
  • Adds unit tests

Copy link
Collaborator

@lusmoura lusmoura left a comment

Choose a reason for hiding this comment

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

LGTM! What should we do about the tests that are failing? Should we add JWT_SECRET_KEY to the repo?

Copy link
Collaborator

@EugeneLightsOn EugeneLightsOn left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks. Only take a look at Luisa's comments.

Copy link
Collaborator

@EugeneLightsOn EugeneLightsOn left a comment

Choose a reason for hiding this comment

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

Great PR, thank you
Please restore Make win-setup and win-first-run commands and good to go.

Copy link
Collaborator

@malexw malexw left a comment

Choose a reason for hiding this comment

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

I didn't take a close look at the tests, but this looks pretty good to me

…into add-jwt

 Please enter a commit message to explain why this merge is necessary,
Copy link
Collaborator

@EugeneLightsOn EugeneLightsOn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@tianjing-li tianjing-li merged commit 1eac5db into main Jun 6, 2024
@tianjing-li tianjing-li deleted the add-jwt branch June 6, 2024 15:31
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