From c395076fce7237f9e3123a783002e7327274d3c3 Mon Sep 17 00:00:00 2001
From: Eliot Berriot <contact@eliotberriot.com>
Date: Wed, 13 Mar 2019 16:50:49 +0100
Subject: [PATCH] Resolve "Use cookies instead of local storage for auth in Web
UI"
---
api/config/api_urls.py | 3 +
api/config/routing.py | 9 ++-
api/config/settings/common.py | 4 +-
api/funkwhale_api/common/consumers.py | 17 ++--
api/funkwhale_api/users/auth_serializers.py | 16 ++++
api/funkwhale_api/users/auth_urls.py | 8 ++
api/funkwhale_api/users/auth_views.py | 32 ++++++++
api/tests/users/test_auth_views.py | 86 +++++++++++++++++++++
changes/changelog.d/629.bugfix | 1 +
front/package.json | 1 -
front/src/App.vue | 4 +-
front/src/components/audio/Track.vue | 9 ---
front/src/components/library/TrackBase.vue | 7 --
front/src/main.js | 15 +++-
front/src/store/auth.js | 62 ++++-----------
front/src/store/instance.js | 1 -
front/src/utils/cookie.js | 7 ++
front/tests/unit/specs/store/auth.spec.js | 74 +-----------------
18 files changed, 203 insertions(+), 153 deletions(-)
create mode 100644 api/funkwhale_api/users/auth_serializers.py
create mode 100644 api/funkwhale_api/users/auth_urls.py
create mode 100644 api/funkwhale_api/users/auth_views.py
create mode 100644 api/tests/users/test_auth_views.py
create mode 100644 changes/changelog.d/629.bugfix
create mode 100644 front/src/utils/cookie.js
diff --git a/api/config/api_urls.py b/api/config/api_urls.py
index 93138e9a5..c839025e6 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 13a67cd1e..db8a1d910 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 5f69c36d5..220ccbc36 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 48c318638..94930f63d 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 000000000..451970904
--- /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 000000000..385e58da0
--- /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 000000000..697b196cf
--- /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 000000000..0b375ed31
--- /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 000000000..d8e58c2b0
--- /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 22b5f3bb5..5f3826fdc 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 fd94a9f46..46826d6e2 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 2e0f8c421..c82b28167 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 34ce4a295..ece34f4dd 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 9f058d8ec..a722e6895 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 29c4c5d71..aec40fe05 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 6af36e809..1215af5e4 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 000000000..2b8fd8c79
--- /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 05581979f..0bb98bb93 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' }
- ]
- })
- })
})
})
--
GitLab