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: api, legacy wip: Move authentication to django api #2428

Closed

Conversation

togir2
Copy link
Contributor

@togir2 togir2 commented Mar 6, 2023

Description

I added a login and password reset pages to Django.
There are now the Following new Endpoints:

  • /api/v2/authentication/{login, logout}
  • /api/v2/authentication/{password-reset, password-reset-confirm, password-reset-complete}

I also added an email section to the configuration file

Todo:

  • Redirect GET "/login" to " /api/v2/authentication/login"
  • Show a message on login page if the login did succeed
  • Add JWT endpoint so that api only clients can use a token
  • Update docs for email settings
  • Update docs for password reset
  • Add tests for the email config
  • Add e2e test for the login
  • Add e2e tests for the password reset

Links

#1788

@togir2 togir2 force-pushed the move_authentication_to_django_api branch from ba547db to 3d148ab Compare March 6, 2023 17:46
@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #2428 (662fbd1) into main (be3964c) will increase coverage by 3.31%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2428      +/-   ##
==========================================
+ Coverage   58.60%   61.91%   +3.31%     
==========================================
  Files         146      145       -1     
  Lines        4343     4122     -221     
==========================================
+ Hits         2545     2552       +7     
+ Misses       1798     1570     -228     
Flag Coverage Δ
analyzer 47.33% <ø> (ø)
api-client 40.99% <ø> (ø)
playout 30.01% <ø> (+4.21%) ⬆️
shared 88.45% <100.00%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/libretime_api/core/models/user.py 83.95% <ø> (ø)
api/libretime_api/settings/_internal.py 95.83% <ø> (ø)
api/libretime_api/settings/_schema.py 100.00% <ø> (ø)
api/libretime_api/settings/prod.py 100.00% <ø> (ø)
api/libretime_api/urls.py 100.00% <ø> (ø)
shared/libretime_shared/config/__init__.py 100.00% <ø> (ø)
shared/libretime_shared/config/_models.py 96.22% <100.00%> (+0.17%) ⬆️
...out/libretime_playout/liquidsoap/client/_client.py 32.94% <0.00%> (-0.80%) ⬇️
...out/libretime_playout/player/liquidsoap_gateway.py 43.61% <0.00%> (-0.47%) ⬇️
playout/libretime_playout/main.py 0.00% <0.00%> (ø)
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@togir2 togir2 changed the title feat(api, legacy) wip: Move authentication to django api feat: api, legacy wip: Move authentication to django api Mar 6, 2023
@togir2 togir2 force-pushed the move_authentication_to_django_api branch 3 times, most recently from 048871e to 43f55e4 Compare March 6, 2023 19:25
Django must send password-reset emails via smtp
@togir2 togir2 force-pushed the move_authentication_to_django_api branch from 43f55e4 to 662fbd1 Compare March 6, 2023 19:33
@jooola
Copy link
Contributor

jooola commented Mar 6, 2023

Hey @togir2,
I really appreciate the energy, but could we talk about some implementation details before you put too much energy in this ?

Maybe @paddatrapper will be able to guide this week, I'll be afk.

I didnt make a full review, but it seem you added some django views, which we don't want. The Api is meant to be headless. In the future we want to add a Vue based webapp that will talk to the api.

We want to slowly transfom the frontend with parts of the new UI and not do a full rewrite. See the issue tracker, this has been discussed already.

In addition I would like to weight the pros and cons about the sessions vs JWT, I would hate to break things because we didnt thought this trough.

@togir2
Copy link
Contributor Author

togir2 commented Mar 6, 2023

Sorry for the noise with the merge request.

[...] but could we talk about some implementation details before you put too much energy in this ?

Oh yes absolutely!
I did not intended this to be a final solution / to be merged. I intended it (for me) to be a test if:

  • forwarding the auth-data to the legacy would work
  • what needs to be added to the api to support authentication

I though as I did the testing for me anyway I would share it with you all, maybe it would help finding a decision, or provide a concrete example to discuss.

Sorry if that did sends the wrong signal...
And sorry for the noise...

If will close this PR, as I'm sure this will not be the final solution and for discussion there is already the Issue #1788

@togir2 togir2 closed this Mar 6, 2023
@jooola
Copy link
Contributor

jooola commented Mar 6, 2023

It wasn't clear before your comment, but now that you cleared that up, feel free to keep the PR open for experimenting. And no worries, the noise does not bother.

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.

2 participants