diff --git a/api/config/api_urls.py b/api/config/api_urls.py index 93138e9a5df1df3c8239c5d3740f1b2a5504a6ed..c839025e67e5807ff66b231c7dbfc96f2c6f689a 100644 --- a/api/config/api_urls.py +++ b/api/config/api_urls.py @@ -75,6 +75,9 @@ v1_patterns += [ r"^users/", include(("funkwhale_api.users.api_urls", "users"), namespace="users"), ), + url( + r"^auth/", include(("funkwhale_api.users.auth_urls", "auth"), namespace="auth") + ), url(r"^token/$", jwt_views.obtain_jwt_token, name="token"), url(r"^token/refresh/$", jwt_views.refresh_jwt_token, name="token_refresh"), ] diff --git a/api/config/routing.py b/api/config/routing.py index 13a67cd1e4f2e8691713bc64b0db27248249da46..db8a1d910f6f1ffd7fe72c8bbd2bcf72c4a43247 100644 --- a/api/config/routing.py +++ b/api/config/routing.py @@ -1,4 +1,5 @@ from channels.routing import ProtocolTypeRouter, URLRouter +from channels.sessions import SessionMiddlewareStack from django.conf.urls import url from funkwhale_api.common.auth import TokenAuthMiddleware @@ -7,8 +8,12 @@ from funkwhale_api.instance import consumers application = ProtocolTypeRouter( { # Empty for now (http->django views is added by default) - "websocket": TokenAuthMiddleware( - URLRouter([url("^api/v1/activity$", consumers.InstanceActivityConsumer)]) + "websocket": SessionMiddlewareStack( + TokenAuthMiddleware( + URLRouter( + [url("^api/v1/activity$", consumers.InstanceActivityConsumer)] + ) + ) ) } ) diff --git a/api/config/settings/common.py b/api/config/settings/common.py index 5f69c36d55016c49efa0adffba1bd51afdeff2f1..220ccbc36e096cdd69737beb5a2ec1f54d40e42d 100644 --- a/api/config/settings/common.py +++ b/api/config/settings/common.py @@ -330,7 +330,7 @@ AUTHENTICATION_BACKENDS = ( "funkwhale_api.users.auth_backends.ModelBackend", "allauth.account.auth_backends.AuthenticationBackend", ) -SESSION_COOKIE_HTTPONLY = False +SESSION_COOKIE_HTTPONLY = True # Some really nice defaults ACCOUNT_AUTHENTICATION_METHOD = "username_email" ACCOUNT_EMAIL_REQUIRED = True @@ -537,7 +537,7 @@ MUSICBRAINZ_HOSTNAME = env("MUSICBRAINZ_HOSTNAME", default="musicbrainz.org") # Custom Admin URL, use {% url 'admin:index' %} ADMIN_URL = env("DJANGO_ADMIN_URL", default="^api/admin/") -CSRF_USE_SESSIONS = True +CSRF_USE_SESSIONS = False SESSION_ENGINE = "django.contrib.sessions.backends.cache" # Playlist settings diff --git a/api/funkwhale_api/common/consumers.py b/api/funkwhale_api/common/consumers.py index 48c3186384f278be8e1e3480cd4011f73de2a10c..94930f63d028de038be797e3ab5a19f05df39eec 100644 --- a/api/funkwhale_api/common/consumers.py +++ b/api/funkwhale_api/common/consumers.py @@ -1,16 +1,21 @@ +from asgiref.sync import async_to_sync from channels.generic.websocket import JsonWebsocketConsumer - +from channels import auth from funkwhale_api.common import channels class JsonAuthConsumer(JsonWebsocketConsumer): def connect(self): - try: - assert self.scope["user"].pk is not None - except (AssertionError, AttributeError, KeyError): - return self.close() + if "user" not in self.scope: + try: + self.scope["user"] = async_to_sync(auth.get_user)(self.scope) + except (ValueError, AssertionError, AttributeError, KeyError): + return self.close() - return self.accept() + if self.scope["user"] and self.scope["user"].is_authenticated: + return self.accept() + else: + return self.close() def accept(self): super().accept() diff --git a/api/funkwhale_api/users/auth_serializers.py b/api/funkwhale_api/users/auth_serializers.py new file mode 100644 index 0000000000000000000000000000000000000000..451970904e13efa95119202a2c4ed16feee196db --- /dev/null +++ b/api/funkwhale_api/users/auth_serializers.py @@ -0,0 +1,16 @@ +from django.contrib import auth + +from rest_framework import serializers + + +class LoginSerializer(serializers.Serializer): + username = serializers.CharField() + password = serializers.CharField() + + def validate(self, validated_data): + user = auth.authenticate(request=None, **validated_data) + if user is None: + raise serializers.ValidationError("Invalid username or password") + + validated_data["user"] = user + return validated_data diff --git a/api/funkwhale_api/users/auth_urls.py b/api/funkwhale_api/users/auth_urls.py new file mode 100644 index 0000000000000000000000000000000000000000..385e58da0c76753a2ba253af8a513fd65efc510e --- /dev/null +++ b/api/funkwhale_api/users/auth_urls.py @@ -0,0 +1,8 @@ +from django.conf.urls import url + +from . import auth_views + +urlpatterns = [ + url(r"^login/$", auth_views.LoginView.as_view(), name="login"), + url(r"^logout/$", auth_views.LogoutView.as_view(), name="logout"), +] diff --git a/api/funkwhale_api/users/auth_views.py b/api/funkwhale_api/users/auth_views.py new file mode 100644 index 0000000000000000000000000000000000000000..697b196cf18ff9d98ac5186b9c1df601262e744c --- /dev/null +++ b/api/funkwhale_api/users/auth_views.py @@ -0,0 +1,32 @@ +from django.contrib import auth + +from rest_framework import response +from rest_framework import views + +from . import auth_serializers + + +class LoginView(views.APIView): + authentication_classes = [] + permission_classes = [] + + def post(self, request, *args, **kwargs): + + serializer = auth_serializers.LoginSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + + auth.login(request=request, user=serializer.validated_data["user"]) + + payload = {} + + return response.Response(payload) + + +class LogoutView(views.APIView): + authentication_classes = [] + permission_classes = [] + + def post(self, request, *args, **kwargs): + auth.logout(request) + payload = {} + return response.Response(payload) diff --git a/api/tests/users/test_auth_views.py b/api/tests/users/test_auth_views.py new file mode 100644 index 0000000000000000000000000000000000000000..0b375ed31a5afbf13a85f08f21e58ee9b67b0430 --- /dev/null +++ b/api/tests/users/test_auth_views.py @@ -0,0 +1,86 @@ +from django.contrib import auth +from django.urls import reverse + + +def test_restricted_access(api_client, db): + url = reverse("api:v1:artists-list") + response = api_client.get(url) + + assert response.status_code == 401 + + +def test_login_correct(api_client, factories, mocker): + login = mocker.spy(auth, "login") + password = "hellotest" + user = factories["users.User"]() + user.set_password(password) + user.save() + + url = reverse("api:v1:auth:login") + data = {"username": user.username, "password": password} + expected = {} + response = api_client.post(url, data) + + assert response.status_code == 200 + assert response.data == expected + login.assert_called_once_with(request=mocker.ANY, user=user) + + +def test_login_incorrect(api_client, factories, mocker): + login = mocker.spy(auth, "login") + user = factories["users.User"]() + + url = reverse("api:v1:auth:login") + data = {"username": user.username, "password": "invalid"} + response = api_client.post(url, data) + + assert response.status_code == 400 + + login.assert_not_called() + + +def test_login_inactive(api_client, factories, mocker): + login = mocker.spy(auth, "login") + password = "hellotest" + user = factories["users.User"](is_active=False) + user.set_password(password) + user.save() + + url = reverse("api:v1:auth:login") + data = {"username": user.username, "password": password} + response = api_client.post(url, data) + + assert response.status_code == 400 + assert "Invalid username or password" in response.data["non_field_errors"] + + login.assert_not_called() + + +def test_logout(logged_in_api_client, factories, mocker): + logout = mocker.spy(auth, "logout") + + url = reverse("api:v1:auth:logout") + response = logged_in_api_client.post(url) + + assert response.status_code == 200 + assert response.data == {} + logout.assert_called_once_with(request=mocker.ANY) + + +def test_logout_real(api_client, factories): + password = "hellotest" + user = factories["users.User"]() + user.set_password(password) + user.save() + + url = reverse("api:v1:auth:login") + data = {"username": user.username, "password": password} + response = api_client.post(url, data) + + url = reverse("api:v1:auth:logout") + response = api_client.post(url) + + url = reverse("api:v1:artists-list") + response = api_client.get(url) + + assert response.status_code == 401 diff --git a/changes/changelog.d/629.bugfix b/changes/changelog.d/629.bugfix new file mode 100644 index 0000000000000000000000000000000000000000..d8e58c2b0453bb5dc6ad3c74a6cb5b8067227117 --- /dev/null +++ b/changes/changelog.d/629.bugfix @@ -0,0 +1 @@ +Now use cookie-based auth in browser instead of JWT/LocalStorage in (#629) diff --git a/front/package.json b/front/package.json index 22b5f3bb5ed35aebd5cb562c5f752ec2a65f8533..5f3826fdc63c1924b6f1f86d162c28633c962ead 100644 --- a/front/package.json +++ b/front/package.json @@ -17,7 +17,6 @@ "django-channels": "^1.1.6", "howler": "^2.0.14", "js-logger": "^1.4.1", - "jwt-decode": "^2.2.0", "lodash": "^4.17.10", "masonry-layout": "^4.2.2", "moment": "^2.22.2", diff --git a/front/src/App.vue b/front/src/App.vue index fd94a9f4605dc69d386d864f3663c5d928e8e4e8..46826d6e27d8c6c840806f77834525bcbc71937e 100644 --- a/front/src/App.vue +++ b/front/src/App.vue @@ -175,11 +175,9 @@ export default { } this.disconnect() let self = this - let token = this.$store.state.auth.token - // let token = 'test' const bridge = new WebSocketBridge() this.bridge = bridge - let url = this.$store.getters['instance/absoluteUrl'](`api/v1/activity?token=${token}`) + let url = this.$store.getters['instance/absoluteUrl'](`api/v1/activity`) url = url.replace('http://', 'ws://') url = url.replace('https://', 'wss://') bridge.connect( diff --git a/front/src/components/audio/Track.vue b/front/src/components/audio/Track.vue index 2e0f8c421a0e780247224b93764b55fd476ae541..c82b281673cc621bf55841e5856bc04d113a275f 100644 --- a/front/src/components/audio/Track.vue +++ b/front/src/components/audio/Track.vue @@ -71,15 +71,6 @@ export default { 'mp3' ) }) - if (this.$store.state.auth.authenticated) { - // we need to send the token directly in url - // so authentication can be checked by the backend - // because for audio files we cannot use the regular Authentication - // header - sources.forEach(e => { - e.url = url.updateQueryString(e.url, 'jwt', this.$store.state.auth.token) - }) - } return sources }, updateProgressThrottled () { diff --git a/front/src/components/library/TrackBase.vue b/front/src/components/library/TrackBase.vue index 34ce4a2954598cbcb0b83a8509bf26c55dc31ede..ece34f4dd0cf91ba8ae66b8351b200311728f8d5 100644 --- a/front/src/components/library/TrackBase.vue +++ b/front/src/components/library/TrackBase.vue @@ -154,13 +154,6 @@ export default { let u = this.$store.getters["instance/absoluteUrl"]( this.upload.listen_url ) - if (this.$store.state.auth.authenticated) { - u = url.updateQueryString( - u, - "jwt", - encodeURI(this.$store.state.auth.token) - ) - } return u }, cover() { diff --git a/front/src/main.js b/front/src/main.js index 9f058d8ecbef6491660647a67e815d0b5beb8f62..a722e68958464baeb60d40c6820f09347264b3ed 100644 --- a/front/src/main.js +++ b/front/src/main.js @@ -16,6 +16,7 @@ import GetTextPlugin from 'vue-gettext' import { sync } from 'vuex-router-sync' import locales from '@/locales' +import cookie from '@/utils/cookie' import filters from '@/filters' // eslint-disable-line import globals from '@/components/globals' // eslint-disable-line @@ -70,8 +71,9 @@ Vue.directive('title', function (el, binding) { ) axios.interceptors.request.use(function (config) { // Do something before request is sent - if (store.state.auth.token) { - config.headers['Authorization'] = store.getters['auth/header'] + let csrfToken = cookie.get('csrftoken') + if (csrfToken) { + config.headers['X-CSRFToken'] = csrfToken } return config }, function (error) { @@ -85,9 +87,14 @@ axios.interceptors.response.use(function (response) { }, function (error) { error.backendErrors = [] if (error.response.status === 401) { + if (error.response.config.skipLoginRedirect) { + return Promise.reject(error) + } store.commit('auth/authenticated', false) - logger.default.warn('Received 401 response from API, redirecting to login form', router.currentRoute.fullPath) - router.push({name: 'login', query: {next: router.currentRoute.fullPath}}) + if (router.currentRoute.name !== 'login') { + logger.default.warn('Received 401 response from API, redirecting to login form', router.currentRoute.fullPath) + router.push({name: 'login', query: {next: router.currentRoute.fullPath}}) + } } if (error.response.status === 404) { error.backendErrors.push('Resource not found') diff --git a/front/src/store/auth.js b/front/src/store/auth.js index 29c4c5d719da539922c36f71a3996b2101ed180b..aec40fe05725ad51a2d3faf830016a3069aab686 100644 --- a/front/src/store/auth.js +++ b/front/src/store/auth.js @@ -1,5 +1,4 @@ import axios from 'axios' -import jwtDecode from 'jwt-decode' import logger from '@/logging' import router from '@/router' @@ -15,13 +14,6 @@ export default { moderation: false }, profile: null, - token: '', - tokenData: {} - }, - getters: { - header: state => { - return 'JWT ' + state.token - } }, mutations: { reset (state) { @@ -29,8 +21,6 @@ export default { state.profile = null state.username = '' state.fullUsername = '' - state.token = '' - state.tokenData = {} state.availablePermissions = { federation: false, settings: false, @@ -46,8 +36,6 @@ export default { if (value === false) { state.username = null state.fullUsername = null - state.token = null - state.tokenData = null state.profile = null state.availablePermissions = {} } @@ -63,24 +51,15 @@ export default { state.profile.avatar = value } }, - token: (state, value) => { - state.token = value - if (value) { - state.tokenData = jwtDecode(value) - } else { - state.tokenData = {} - } - }, permission: (state, {key, status}) => { state.availablePermissions[key] = status } }, actions: { - // Send a request to the login URL and save the returned JWT + // Send a request to the login URL login ({commit, dispatch}, {next, credentials, onError}) { - return axios.post('token/', credentials).then(response => { + return axios.post('auth/login/', credentials).then(response => { logger.default.info('Successfully logged in as', credentials.username) - commit('token', response.data.token) dispatch('fetchProfile').then(() => { // Redirect to a specified route router.push(next) @@ -102,29 +81,28 @@ export default { modules.forEach(m => { commit(`${m}/reset`, null, {root: true}) }) - logger.default.info('Log out, goodbye!') - router.push({name: 'index'}) + return axios.post('auth/logout/').then(response => { + logger.default.info('Successfully logged out') + }, response => { + // we cannot contact the backend but we can at least clear our local cookies + logger.default.info('Backend unreachable, cleaning local cookies…') + }).finally(() => { + logger.default.info('Log out, goodbye!') + router.push({name: 'index'}) + }) }, check ({commit, dispatch, state}) { logger.default.info('Checking authentication...') - var jwt = state.token - if (jwt) { - commit('token', jwt) - dispatch('fetchProfile') - dispatch('refreshToken') - } else { + dispatch('fetchProfile').then(() => { + logger.default.info('Welcome back!') + }).catch(() => { logger.default.info('Anonymous user') commit('authenticated', false) - } + }) }, fetchProfile ({commit, dispatch, state}) { - if (document) { - // this is to ensure we do not have any leaking cookie set by django - document.cookie = 'sessionid=; Path=/; Expires=Thu, 01 Jan 1970 00:00:01 GMT;' - } - return new Promise((resolve, reject) => { - axios.get('users/users/me/').then((response) => { + axios.get('users/users/me/', {skipLoginRedirect: true}).then((response) => { logger.default.info('Successfully fetched user profile') dispatch('updateProfile', response.data).then(() => { resolve(response.data) @@ -156,13 +134,5 @@ export default { resolve() }) }, - refreshToken ({commit, dispatch, state}) { - return axios.post('token/refresh/', {token: state.token}).then(response => { - logger.default.info('Refreshed auth token') - commit('token', response.data.token) - }, response => { - logger.default.error('Error while refreshing token', response.data) - }) - } } } diff --git a/front/src/store/instance.js b/front/src/store/instance.js index 6af36e8090e3ba276c7fabaf870ebd251f086a15..1215af5e43f29dd9d1b5d6f339dee7c2382e93d4 100644 --- a/front/src/store/instance.js +++ b/front/src/store/instance.js @@ -114,7 +114,6 @@ export default { commit(`${m}/reset`, null, {root: true}) }) }, - // Send a request to the login URL and save the returned JWT fetchSettings ({commit}, payload) { return axios.get('instance/settings/').then(response => { logger.default.info('Successfully fetched instance settings') diff --git a/front/src/utils/cookie.js b/front/src/utils/cookie.js new file mode 100644 index 0000000000000000000000000000000000000000..2b8fd8c79994e474d6eec63de7ec6b760d1ee5a1 --- /dev/null +++ b/front/src/utils/cookie.js @@ -0,0 +1,7 @@ +export default { + get (name) { + var value = "; " + document.cookie; + var parts = value.split("; " + name + "="); + if (parts.length == 2) return parts.pop().split(";").shift(); + } +} diff --git a/front/tests/unit/specs/store/auth.spec.js b/front/tests/unit/specs/store/auth.spec.js index 05581979fb122e9f4d789d653ef1baa56d827226..0bb98bb93e4af121c2b2f6c46605b0272db2d4f0 100644 --- a/front/tests/unit/specs/store/auth.spec.js +++ b/front/tests/unit/specs/store/auth.spec.js @@ -37,54 +37,21 @@ describe('store/auth', () => { it('authenticated false', () => { const state = { username: 'dummy', - token: 'dummy', - tokenData: 'dummy', profile: 'dummy', availablePermissions: 'dummy' } store.mutations.authenticated(state, false) expect(state.authenticated).to.equal(false) expect(state.username).to.equal(null) - expect(state.token).to.equal(null) - expect(state.tokenData).to.equal(null) expect(state.profile).to.equal(null) expect(state.availablePermissions).to.deep.equal({}) }) - it('token null', () => { - const state = {} - store.mutations.token(state, null) - expect(state.token).to.equal(null) - expect(state.tokenData).to.deep.equal({}) - }) - it('token real', () => { - // generated on http://kjur.github.io/jsjws/tool_jwt.html - const state = {} - let token = 'eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJpc3MiOiJodHRwczovL2p3dC1pZHAuZXhhbXBsZS5jb20iLCJzdWIiOiJtYWlsdG86bWlrZUBleGFtcGxlLmNvbSIsIm5iZiI6MTUxNTUzMzQyOSwiZXhwIjoxNTE1NTM3MDI5LCJpYXQiOjE1MTU1MzM0MjksImp0aSI6ImlkMTIzNDU2IiwidHlwIjoiaHR0cHM6Ly9leGFtcGxlLmNvbS9yZWdpc3RlciJ9.' - let tokenData = { - iss: 'https://jwt-idp.example.com', - sub: 'mailto:mike@example.com', - nbf: 1515533429, - exp: 1515537029, - iat: 1515533429, - jti: 'id123456', - typ: 'https://example.com/register' - } - store.mutations.token(state, token) - expect(state.token).to.equal(token) - expect(state.tokenData).to.deep.equal(tokenData) - }) it('permissions', () => { const state = { availablePermissions: {} } store.mutations.permission(state, {key: 'admin', status: true}) expect(state.availablePermissions).to.deep.equal({admin: true}) }) }) - describe('getters', () => { - it('header', () => { - const state = { token: 'helloworld' } - expect(store.getters['header'](state)).to.equal('JWT helloworld') - }) - }) describe('actions', () => { it('logout', () => { testAction({ @@ -100,30 +67,8 @@ describe('store/auth', () => { ] }) }) - it('check jwt null', () => { - testAction({ - action: store.actions.check, - params: {state: {}}, - expectedMutations: [ - { type: 'authenticated', payload: false } - ] - }) - }) - it('check jwt set', () => { - testAction({ - action: store.actions.check, - params: {state: {token: 'test', username: 'user'}}, - expectedMutations: [ - { type: 'token', payload: 'test' } - ], - expectedActions: [ - { type: 'fetchProfile' }, - { type: 'refreshToken' } - ] - }) - }) it('login success', () => { - moxios.stubRequest('token/', { + moxios.stubRequest('auth/login/', { status: 200, response: { token: 'test' @@ -135,9 +80,7 @@ describe('store/auth', () => { testAction({ action: store.actions.login, payload: {credentials: credentials}, - expectedMutations: [ - { type: 'token', payload: 'test' } - ], + expectedMutations: [], expectedActions: [ { type: 'fetchProfile' } ] @@ -187,18 +130,5 @@ describe('store/auth', () => { ] }) }) - it('refreshToken', () => { - moxios.stubRequest('token/refresh/', { - status: 200, - response: {token: 'newtoken'} - }) - testAction({ - action: store.actions.refreshToken, - params: {state: {token: 'oldtoken'}}, - expectedMutations: [ - { type: 'token', payload: 'newtoken' } - ] - }) - }) }) })