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

Move authentification to the django API #1788

Open
jooola opened this issue Apr 22, 2022 · 13 comments · May be fixed by #2572
Open

Move authentification to the django API #1788

jooola opened this issue Apr 22, 2022 · 13 comments · May be fixed by #2572

Comments

@jooola
Copy link
Contributor

jooola commented Apr 22, 2022

In the transition to Django, I think we should setup a shared session store in the db (or in a key/value store). I am unsure how feasible this is on the PHP side, but this should simplify the authentication management.

The future Webapp will need to call the Django API, and we will probably reuse the same shared session.

Ideally we would have a single place with Write access to the store, and the rest will only have a read only permissions to the store to authenticate a request and restore session data.

@togir2
Copy link
Contributor

togir2 commented Mar 4, 2023

I took a look at the php-code and I am afraid of changing the session store...
I do have a proposal with which we can login via Django and then create two sessions, one for Django and one for php.

  1. Login to Django with Username/ Password
  2. Django creates its session and an extra token for the user stored in a cookie
    The cookie should look like this:
  message = {"db_userid": <id>, "valid_until":"<timestring>"}
  cookie = {"message" : message, "hmac": hmac(mesage, "libretime_secret_key")}
  valid_until should be short timespan
  1. Django redirects to the legacy login
  2. Custom authAdapter reads cookie-token, check if hmac is valid.
  3. If valid: create php session for the user and delete token from cookie

The user is now logged in with his Django "session_cookie" and the PHP_SESSID

Advantages:

  • "Easy" to implement
  • We do not need an extra database
  • The session handling of the legacy php is not changed
  • We do need only an addition to the Django authentication flow
  • The Django login and session management is not changed
  • It is possible to update the password in the DB to use the Django hashfunctions (no more md5!)
  • In the future our additions can be removed and the "stock" Django authentication is back.

Disadvantages:

  • On logout Django must delete it's own session cookie + the PHP_SESSID
  • Session in legacy does not get deleted on logout, only when expired (allows for longer bruteforce time)
  • Deletion of only one session_cookie (e.g. session timeout) will lead to the user logged in at either Django or legacy
  • When the cookie gets stolen we are "duration from now until valid_until" vulnerable to replay attacks, but if it is possible that cookies are stolen the session cookies are also not save.

@jooola
Copy link
Contributor Author

jooola commented Mar 5, 2023

Yes, maybe not shared the session store is cleaner.

Have you considered adding some internal endpoints to legacy, and rather than redirecting the user, we make a few calls from the api to legacy to maintain the php sessions ?

After valid login in the api, we forward the request to legacy, create a session and we respond to the user with both api and legacy data ?

And what about not using sessions at all in the API and rather use for example a JWT ?

@jooola jooola changed the title Shared session between Django and Zend Move authentification to the django API Mar 5, 2023
@togir2
Copy link
Contributor

togir2 commented Mar 5, 2023

After valid login in the api, we forward the request to legacy, create a session and we respond to the user with both api and legacy data ?

This does sound fine and also not hard to implement.
I did some tests and crafted this curl command to authenticate against the legacy:

curl 'http://localhost:8080/login' -X POST -H 'Referer: http://localhost:8080/login' --data-raw 'username=<username&password=<password>&locale=en_US&submit=Login' -v

And what about not using sessions at all in the API and rather use for example a JWT ?

I think JWT is also fine.
I do also think django can support both cookie-sessions and JWT parallel without problems.
A cool feature of JWT would be that they do not need a database and so (in theory) also the legacy or other future other parts, if there ever will be any, could use them for user authentication.

@togir2
Copy link
Contributor

togir2 commented Mar 6, 2023

I had some time, so I though I'll give it a try and see how possible this is.
I decided to use the Cookie Sesssion for a start as there is currently no code that can use a JWT and I did not want to invest
extra time adding the token endpoints for a POC.

I plan to add the JWT endpoints in such a way that a either:

  • an authenticated client can make a GET/POST request to the token endpoint and receives a response the tokens
  • an unauthenticated client can make a POST request with "username" and "password" to the token endpoint and receives the token

What do you think about this?

@jooola
Copy link
Contributor Author

jooola commented Mar 14, 2023

Let's keep JWT away for now, we can always add it later on for external clients. I think the cookie session is our best option for browser clients. The discussion about JWT should probably also involve oauth2 (auth+scopes) and our permission model. So I prefer to keep in a dedicated ticket.

I could see the following auth systems:

  • Cookie based sessions for browsers clients.
  • global and/or users tokens (e.g. for the main/show stream inputs). Whether we use JWT or something else is still open for discussion.
  • the legacy api_key for internal communication (key stored in the config file), but I would like to deprecate this in favor of something more dynamic and secure. This is also another discussion.

I don't think there is any other email that we send except the user management emails, which will be moved to Django. This should close #724 and we should probably only use Django as our mail client. (I am very happy that this will be closed as well!)

The current login page (legacy) must be kept, but we will have to change the login POST endpoint to use the django api. The changes are minimal, maybe not pretty, but I prefer to transition to the new frontend in a future PR.

Legacy is currently supposed to support a LDAP auth backend, I think this should also be taken into consideration when moving the auth system to the API. But I have to admit, I have no idea if this is still working or even used. If a ldap backend is added to the api auth, we need to make it optional, as I prefer not to install too much dependencies if only a few need the feature.

I should probably think a bit more about the pros and cons between JWT and sessions (cookies), I have been away for some time, so I need some more time to dive into this.

@jooola
Copy link
Contributor Author

jooola commented Mar 23, 2023

With this change I think we can introduce the new Vue based UI, and replace the entire login page. So we don't loose too much time working with the legacy code.

@jooola jooola added this to the 3.2.0 milestone Apr 6, 2023
@jooola
Copy link
Contributor Author

jooola commented Apr 24, 2023

After looking for a cleaner/alternative solution, I concluded that we should rather use a database based session store for the legacy app, and manage the store from the Django side.

This prevents us from doing HTTP requests between services, which would introduce a lot of headache. For example, just to find the internal URL of the legacy service without adding an extra configuration entry is not so straight forward.

Here are few steps to solve:

  • Move the legacy session storage from the file base sessions to the database based sessions (Zend has an adapter for this, so we won't reimplement the wheel here)
  • Add an auth management app in the django api with a few views (login/logout/password reset/...) (I'd use the dj-rest-auth library)
  • Create a PoC python script to create a session from python into the database: as we have to serialize the user data in a PHP specific format, we will have to rely on some library to serialize the user data and store it in the database.
  • Implement an extra session store in django that is capable of creating and deleting sessions in the database for the Legacy app. The django session app already provide some classes that could be enhanced with our custom logic. The session store should also handle expiration and other session related features.
  • Attach the legacy session store to the auth management app
  • Embed a Vue view in the legacy app and implement a new login page with the new API endpoints.
  • Remove old login/logout legacy views.

This is a decent amount of work, but it is a good step to properly handle the migration from the legacy code to django.

@paddatrapper
Copy link
Contributor

This prevents us from doing HTTP requests between services, which would introduce a lot of headache

Do you mean the new approach would prevent us from doing HTTP requests between services? We currently use HTTP requests between services a fair amount - e.g. downloading files from legacy for playout and getting the schedule some of the time.

Add an auth management app in the django api with a few views (login/logout/password reset/...) (I'd use the dj-rest-auth library)

There is some of this already implemented - login and logout at least. It handles both API tokens and user accounts. We could either extend that or replace it with dj-rest-auth.

@jooola
Copy link
Contributor Author

jooola commented Apr 25, 2023

Do you mean the new approach would prevent us from doing HTTP requests between services? We currently use HTTP requests between services a fair amount - e.g. downloading files from legacy for playout and getting the schedule some of the time.

Indeed, we do use HTTP calls between Playout and legacy/API, this is the architecture we agreed on (Schedule and Play blocks). But in our case, we have to make HTTP calls between 2 services that are supposed to be blended together. We cannot use the LIBRETIME_PUBLIC_URL env variable trick to override the public URL inside the API service (as we do for all the satellite service around legacy/API), since the value is required by both legacy and API. We could introduce an extra configuration, but this feels dirty. This is why using the database as a communication layer seem like a better idea.

However, if there is a better alternative/trick, I'm happy to work towards it.

There is some of this already implemented - login and logout at least. It handles both API tokens and user accounts. We could either extend that or replace it with dj-rest-auth.

Hmm, then I am confused. I did find the api token auth system, but I couldn't find the login/logout views with or without sessions, maybe you have a link ?

@paddatrapper
Copy link
Contributor

Hmm, then I am confused. I did find the api token auth system, but I couldn't find the login/logout views with or without sessions, maybe you have a link ?

The default DRF login/logout views work with standard Django session management AFAIR. I'll have to test it out and see if I can remember what I did to get it working

@jooola
Copy link
Contributor Author

jooola commented Apr 27, 2023

The default DRF login/logout views work with standard Django session management AFAIR. I'll have to test it out and see if I can remember what I did to get it working

Oh, you are referring to the existing DRF API browser views, do you think those are usable in production ? I think we should prefer using a library battery included instead of tweaking the API browser views.

@jooola

This comment was marked as outdated.

@jooola
Copy link
Contributor Author

jooola commented May 11, 2023

I've been playing a bit with this new idea, and I managed to create a legacy session in the db, from a django command.

Once the session is inserted, the browser with that session id can navigate the UI without going through the legacy login process.

I've started to work in this in #2523, and have more work in other branches, as I wanted to split the work in multiple PRs.

Now comes the question, are we moving towards this, or should we investigate other solutions ?

@jooola jooola linked a pull request May 30, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants