diff --git a/build/msw_plugin.js b/build/msw_plugin.js new file mode 100644 index 0000000000000000000000000000000000000000..f544348fc099340f7a7d3e81596acb2c2f7c7520 --- /dev/null +++ b/build/msw_plugin.js @@ -0,0 +1,28 @@ +import { resolve } from 'node:path' +import { readFile } from 'node:fs/promises' + +const target = 'node_modules/msw/lib/mockServiceWorker.js' + +const mswPlugin = () => { + let projectRoot + return { + name: 'msw-plugin', + apply: 'serve', + configResolved (conf) { + projectRoot = conf.root + }, + configureServer (server) { + server.middlewares.use(async (req, res, next) => { + if (req.path === '/mockServiceWorker.js') { + const file = await readFile(resolve(projectRoot, target)) + res.set('Content-Type', 'text/javascript') + res.send(file) + } else { + next() + } + }) + } + } +} + +export default mswPlugin diff --git a/changelog.d/no-create-app-on-first-visit.fix b/changelog.d/no-create-app-on-first-visit.fix new file mode 100644 index 0000000000000000000000000000000000000000..b538121af831e5eb20bc5d3267ed3bd78a5bfad1 --- /dev/null +++ b/changelog.d/no-create-app-on-first-visit.fix @@ -0,0 +1 @@ +Create an OAuth app only when needed diff --git a/package.json b/package.json index acbea093eb553e6734a0fc94c980de62868ad244..61d459423013e401313fd213df9b1e3cc8716d6c 100644 --- a/package.json +++ b/package.json @@ -88,6 +88,7 @@ "http-proxy-middleware": "3.0.3", "iso-639-1": "3.1.5", "lodash": "4.17.21", + "msw": "2.7.3", "nightwatch": "3.11.1", "opn": "5.5.0", "ora": "0.4.1", diff --git a/src/boot/after_store.js b/src/boot/after_store.js index 8e27d745758e776ddb9fad9cae55aba1aa344d4b..05aaf7d3b0df8e8e44d3005fd980bf111305184e 100644 --- a/src/boot/after_store.js +++ b/src/boot/after_store.js @@ -11,7 +11,6 @@ import routes from './routes' import VBodyScrollLock from 'src/directives/body_scroll_lock' import { windowWidth, windowHeight } from '../services/window_utils/window_utils' -import { getOrCreateApp, getClientToken } from '../services/new_api/oauth.js' import backendInteractorService from '../services/backend_interactor_service/backend_interactor_service.js' import { applyConfig } from '../services/style_setter/style_setter.js' import FaviconService from '../services/favicon_service/favicon_service.js' @@ -228,17 +227,8 @@ const getStickers = async ({ store }) => { } const getAppSecret = async ({ store }) => { - const { state, commit } = store - const { oauth, instance } = state - if (oauth.userToken) { - commit('setBackendInteractor', backendInteractorService(store.getters.getToken())) - } else { - return getOrCreateApp({ ...oauth, instance: instance.server, commit }) - .then((app) => getClientToken({ ...app, instance: instance.server })) - .then((token) => { - commit('setAppToken', token.access_token) - commit('setBackendInteractor', backendInteractorService(store.getters.getToken())) - }) + if (store.state.oauth.userToken) { + store.commit('setBackendInteractor', backendInteractorService(store.getters.getToken())) } } diff --git a/src/components/login_form/login_form.js b/src/components/login_form/login_form.js index b795640e5fafa61b6daafc13476b72552b0d90eb..23ac86bfbad9ff64ef9db080f03e77992f966fa4 100644 --- a/src/components/login_form/login_form.js +++ b/src/components/login_form/login_form.js @@ -34,16 +34,21 @@ const LoginForm = { this.isTokenAuth ? this.submitToken() : this.submitPassword() }, submitToken () { - const { clientId, clientSecret } = this.oauth const data = { - clientId, - clientSecret, instance: this.instance.server, commit: this.$store.commit } - oauthApi.getOrCreateApp(data) - .then((app) => { oauthApi.login({ ...app, ...data }) }) + // NOTE: we do not really need the app token, but obtaining a token and + // calling verify_credentials is the only way to ensure the app still works. + this.$store.dispatch('ensureAppToken') + .then(() => { + const app = { + clientId: this.oauth.clientId, + clientSecret: this.oauth.clientSecret, + } + oauthApi.login({ ...app, ...data }) + }) }, submitPassword () { const { clientId } = this.oauth @@ -55,7 +60,14 @@ const LoginForm = { } this.error = false - oauthApi.getOrCreateApp(data).then((app) => { + // NOTE: we do not really need the app token, but obtaining a token and + // calling verify_credentials is the only way to ensure the app still works. + this.$store.dispatch('ensureAppToken').then(() => { + const app = { + clientId: this.oauth.clientId, + clientSecret: this.oauth.clientSecret, + } + oauthApi.getTokenWithCredentials( { ...app, diff --git a/src/modules/oauth.js b/src/modules/oauth.js index 038bc3f38ee421673ee5ff9d4ab858e4750446d9..f94dbe974aceccbd9738b60ca98d4b671d240c16 100644 --- a/src/modules/oauth.js +++ b/src/modules/oauth.js @@ -1,5 +1,27 @@ +import { createApp, getClientToken, verifyAppToken } from 'src/services/new_api/oauth.js' + +// status codes about verifyAppToken (GET /api/v1/apps/verify_credentials) +const isAppTokenRejected = error => ( + // Pleroma API docs say it returns 422 when unauthorized + error.statusCode === 422 || + // but it actually returns 400 (as of 2.9.0) + // NOTE: don't try to match against the error message, it is translatable + error.statusCode === 400 || + // and Mastodon docs say it returns 401 + error.statusCode === 401 +) + +// status codes about getAppToken (GET /oauth/token) +const isClientDataRejected = error => ( + // Mastodon docs say it returns 401 + error.statusCode === 401 || + // but Pleroma actually returns 400 (as of 2.9.0) + // NOTE: don't try to match against the error message, it is translatable + error.statusCode === 400 +) + const oauth = { - state: { + state: () => ({ clientId: false, clientSecret: false, /* App token is authentication for app without any user, used mostly for @@ -11,7 +33,7 @@ const oauth = { * that need authorized user to be successful (i.e. posting, liking etc) */ userToken: false - }, + }), mutations: { setClientData (state, { clientId, clientSecret }) { state.clientId = clientId @@ -41,6 +63,88 @@ const oauth = { // added here for smoother transition, otherwise user will be logged out return state.userToken || state.token } + }, + actions: { + async createApp ({ rootState, commit }) { + const instance = rootState.instance.server + const app = await createApp(instance) + commit('setClientData', app) + return app + }, + /// Use this if you want to get the client id and secret but are not interested + /// in whether they are valid. + /// @return {{ clientId: string, clientSecret: string }} An object representing the app + ensureApp ({ state, dispatch }) { + if (state.clientId && state.clientSecret) { + return { + clientId: state.clientId, + clientSecret: state.clientSecret + } + } else { + return dispatch('createApp') + } + }, + async getAppToken ({ state, rootState, commit }) { + const res = await getClientToken({ + clientId: state.clientId, + clientSecret: state.clientSecret, + instance: rootState.instance.server + }) + commit('setAppToken', res.access_token) + return res.access_token + }, + /// Use this if you want to ensure the app is still valid to use. + /// @return {string} The access token to the app (not attached to any user) + async ensureAppToken ({ state, rootState, dispatch, commit }) { + if (state.appToken) { + try { + await verifyAppToken({ + instance: rootState.instance.server, + appToken: state.appToken + }) + return state.appToken + } catch (e) { + if (!isAppTokenRejected(e)) { + // The server did not reject our token, but we encountered other problems. Maybe the server is down. + throw e + } else { + // The app token is rejected, so it is no longer useful. + commit('setAppToken', false) + } + } + } + + // appToken is not available, or is rejected: try to get a new one. + // First, make sure the client id and client secret are filled. + try { + await dispatch('ensureApp') + } catch (e) { + console.error('Cannot create app', e) + throw e + } + + // Note that at this step, the client id and secret may be invalid + // (because the backend may have already deleted the app due to no user login) + try { + return await dispatch('getAppToken') + } catch (e) { + if (!isClientDataRejected(e)) { + // Non-credentials problem, fail fast + console.error('Cannot get app token', e) + throw e + } else { + // the client id and secret are invalid, so we should clear them + // and re-create our app + commit('setClientData', { + clientId: false, + clientSecret: false + }) + await dispatch('createApp') + // try once again to get the token + return await dispatch('getAppToken') + } + } + } } } diff --git a/src/modules/users.js b/src/modules/users.js index 0260b7b3c7e782fcfb42caf06b6f78c91daeb1c0..82b1054934f339abf1382174030da12f1962b7f8 100644 --- a/src/modules/users.js +++ b/src/modules/users.js @@ -1,5 +1,6 @@ import backendInteractorService from '../services/backend_interactor_service/backend_interactor_service.js' import { windowWidth, windowHeight } from '../services/window_utils/window_utils' +import apiService from '../services/api/api.service.js' import oauthApi from '../services/new_api/oauth.js' import { compact, map, each, mergeWith, last, concat, uniq, isArray } from 'lodash' import { registerPushNotifications, unregisterPushNotifications } from '../services/sw/sw.js' @@ -527,11 +528,10 @@ const users = { async signUp (store, userInfo) { store.commit('signUpPending') - const rootState = store.rootState - try { - const data = await rootState.api.backendInteractor.register( - { params: { ...userInfo } } + const token = await store.dispatch('ensureAppToken') + const data = await apiService.register( + { credentials: token, params: { ...userInfo } } ) if (data.access_token) { @@ -562,7 +562,9 @@ const users = { instance: instance.server } - return oauthApi.getOrCreateApp(data) + // NOTE: No need to verify the app still exists, because if it doesn't, + // the token will be invalid too + return store.dispatch('ensureApp') .then((app) => { const params = { app, diff --git a/src/services/new_api/oauth.js b/src/services/new_api/oauth.js index a4b7cbf076dd500909818c493d8cfe240a6923a7..d077110c88c37b7a3ff51fe20e0e8cb02b286022 100644 --- a/src/services/new_api/oauth.js +++ b/src/services/new_api/oauth.js @@ -1,12 +1,20 @@ import { reduce } from 'lodash' +import { StatusCodeError } from 'src/services/errors/errors.js' const REDIRECT_URI = `${window.location.origin}/oauth-callback` -export const getOrCreateApp = ({ clientId, clientSecret, instance, commit }) => { - if (clientId && clientSecret) { - return Promise.resolve({ clientId, clientSecret }) +export const getJsonOrError = async (response) => { + if (response.ok) { + return response.json() + .catch((error) => { + throw new StatusCodeError(response.status, error, {}, response) + }) + } else { + throw new StatusCodeError(response.status, await response.text(), {}, response) } +} +export const createApp = (instance) => { const url = `${instance}/api/v1/apps` const form = new window.FormData() @@ -19,9 +27,16 @@ export const getOrCreateApp = ({ clientId, clientSecret, instance, commit }) => method: 'POST', body: form }) - .then((data) => data.json()) + .then(getJsonOrError) .then((app) => ({ clientId: app.client_id, clientSecret: app.client_secret })) - .then((app) => commit('setClientData', app) || app) +} + +export const verifyAppToken = ({ instance, appToken }) => { + return window.fetch(`${instance}/api/v1/apps/verify_credentials`, { + method: 'GET', + headers: { Authorization: `Bearer ${appToken}` } + }) + .then(getJsonOrError) } const login = ({ instance, clientId }) => { @@ -92,7 +107,7 @@ export const getClientToken = ({ clientId, clientSecret, instance }) => { return window.fetch(url, { method: 'POST', body: form - }).then((data) => data.json()) + }).then(getJsonOrError) } const verifyOTPCode = ({ app, instance, mfaToken, code }) => { const url = `${instance}/oauth/mfa/challenge` @@ -144,7 +159,6 @@ const oauth = { login, getToken, getTokenWithCredentials, - getOrCreateApp, verifyOTPCode, verifyRecoveryCode, revokeToken diff --git a/test/fixtures/mock_api.js b/test/fixtures/mock_api.js new file mode 100644 index 0000000000000000000000000000000000000000..3f9e72955b97ba30690ddbf6d8b2898b01e07480 --- /dev/null +++ b/test/fixtures/mock_api.js @@ -0,0 +1,62 @@ +import { test as testBase } from 'vitest' +import { setupWorker } from 'msw/browser' +import { http, HttpResponse } from 'msw' + +// https://mswjs.io/docs/recipes/vitest-browser-mode +export const injectMswToTest = (defaultHandlers) => { + const worker = setupWorker(...defaultHandlers) + + return testBase.extend({ + worker: [ + async ({}, use) => { + await worker.start() + + await use(worker) + + worker.resetHandlers() + worker.stop() + }, + { + auto: true + } + ], + }) +} + +export const testServer = 'https://test.server.example' + +export const authApis = [ + http.post(`${testServer}/api/v1/apps`, () => { + return HttpResponse.json({ + client_id: 'test-id', + client_secret: 'test-secret' + }) + }), + http.get(`${testServer}/api/v1/apps/verify_credentials`, ({ request }) => { + const authHeader = request.headers.get('Authorization') + if (authHeader === 'Bearer test-app-token' || + authHeader === 'Bearer also-good-app-token') { + return HttpResponse.json({}) + } else { + // Pleroma 2.9.0 gives the following respoonse upon error + return HttpResponse.json({ error: { detail: 'Internal server error' } }, { + status: 400 + }) + } + }), + http.post(`${testServer}/oauth/token`, async ({ request }) => { + const data = await request.formData() + + if (data.get('client_id') === 'test-id' && + data.get('client_secret') === 'test-secret' && + data.get('grant_type') === 'client_credentials' && + data.has('redirect_uri')) { + return HttpResponse.json({ access_token: 'test-app-token' }) + } else { + // Pleroma 2.9.0 gives the following respoonse upon error + return HttpResponse.json({ error: 'Invalid credentials' }, { + status: 400 + }) + } + }) +] diff --git a/test/unit/specs/modules/oauth.spec.js b/test/unit/specs/modules/oauth.spec.js new file mode 100644 index 0000000000000000000000000000000000000000..9d726e35e20da3ff50e00e0365eef6b593c94e4b --- /dev/null +++ b/test/unit/specs/modules/oauth.spec.js @@ -0,0 +1,199 @@ +import { createStore } from 'vuex' +import { http, HttpResponse } from 'msw' +import oauth from 'src/modules/oauth.js' +import { injectMswToTest, authApis, testServer } from '/test/fixtures/mock_api.js' + +const test = injectMswToTest(authApis) + +const getStore = (defaultStateInjection) => { + const stateFunction = defaultStateInjection ? () => { + return { + ...oauth.state(), + ...defaultStateInjection + } + } : oauth.state + + return createStore({ + modules: { + instance: { + state: () => ({ server: testServer }) + }, + oauth: { + ...oauth, + state: stateFunction + } + } + }) +} + + +describe('createApp', () => { + test('it should use create an app and record client id and secret', async () => { + const store = getStore() + const app = await store.dispatch('createApp') + expect(store.state.oauth.clientId).to.eql('test-id') + expect(store.state.oauth.clientSecret).to.eql('test-secret') + expect(app.clientId).to.eql('test-id') + expect(app.clientSecret).to.eql('test-secret') + }) + + test('it should throw and not update if failed', async ({ worker }) => { + worker.use( + http.post(`${testServer}/api/v1/apps`, () => { + return HttpResponse.text('Throttled', { status: 429 }) + }) + ) + + const store = getStore() + const res = store.dispatch('createApp') + await expect(res).rejects.toThrowError('Throttled') + expect(store.state.oauth.clientId).to.eql(false) + expect(store.state.oauth.clientSecret).to.eql(false) + }) +}) + +describe('ensureApp', () => { + test('it should create an app if it does not exist', async () => { + const store = getStore() + const app = await store.dispatch('ensureApp') + expect(store.state.oauth.clientId).to.eql('test-id') + expect(store.state.oauth.clientSecret).to.eql('test-secret') + expect(app.clientId).to.eql('test-id') + expect(app.clientSecret).to.eql('test-secret') + }) + + test('it should not create an app if it exists', async ({ worker }) => { + worker.use( + http.post(`${testServer}/api/v1/apps`, () => { + return HttpResponse.text('Should not call this API', { status: 400 }) + }) + ) + + const store = getStore({ + clientId: 'another-id', + clientSecret: 'another-secret' + }) + const app = await store.dispatch('ensureApp') + expect(store.state.oauth.clientId).to.eql('another-id') + expect(store.state.oauth.clientSecret).to.eql('another-secret') + expect(app.clientId).to.eql('another-id') + expect(app.clientSecret).to.eql('another-secret') + }) +}) + +describe('getAppToken', () => { + test('it should get app token and set it in state', async () => { + const store = getStore({ + clientId: 'test-id', + clientSecret: 'test-secret' + }) + const token = await store.dispatch('getAppToken') + expect(token).to.eql('test-app-token') + expect(store.state.oauth.appToken).to.eql('test-app-token') + }) + + test('it should throw and not set state if it cannot get app token', async () => { + const store = getStore({ + clientId: 'bad-id', + clientSecret: 'bad-secret' + }) + await expect(store.dispatch('getAppToken')).rejects.toThrowError('400') + expect(store.state.oauth.appToken).to.eql(false) + }) +}) + +describe('ensureAppToken', () => { + test('it should work if the state is empty', async () => { + const store = getStore() + const token = await store.dispatch('ensureAppToken') + expect(token).to.eql('test-app-token') + expect(store.state.oauth.appToken).to.eql('test-app-token') + }) + + test('it should work if we already have a working token', async () => { + const store = getStore({ + appToken: 'also-good-app-token' + }) + const token = await store.dispatch('ensureAppToken') + expect(token).to.eql('also-good-app-token') + expect(store.state.oauth.appToken).to.eql('also-good-app-token') + }) + + test('it should work if we have a bad token but good app credentials', async ({ worker }) => { + worker.use( + http.post(`${testServer}/api/v1/apps`, () => { + return HttpResponse.text('Should not call this API', { status: 400 }) + }) + ) + const store = getStore({ + appToken: 'bad-app-token', + clientId: 'test-id', + clientSecret: 'test-secret' + }) + const token = await store.dispatch('ensureAppToken') + expect(token).to.eql('test-app-token') + expect(store.state.oauth.appToken).to.eql('test-app-token') + }) + + test('it should work if we have no token but good app credentials', async ({ worker }) => { + worker.use( + http.post(`${testServer}/api/v1/apps`, () => { + return HttpResponse.text('Should not call this API', { status: 400 }) + }) + ) + const store = getStore({ + clientId: 'test-id', + clientSecret: 'test-secret' + }) + const token = await store.dispatch('ensureAppToken') + expect(token).to.eql('test-app-token') + expect(store.state.oauth.appToken).to.eql('test-app-token') + }) + + test('it should work if we have no token and bad app credentials', async () => { + const store = getStore({ + clientId: 'bad-id', + clientSecret: 'bad-secret' + }) + const token = await store.dispatch('ensureAppToken') + expect(token).to.eql('test-app-token') + expect(store.state.oauth.appToken).to.eql('test-app-token') + expect(store.state.oauth.clientId).to.eql('test-id') + expect(store.state.oauth.clientSecret).to.eql('test-secret') + }) + + test('it should work if we have bad token and bad app credentials', async () => { + const store = getStore({ + appToken: 'bad-app-token', + clientId: 'bad-id', + clientSecret: 'bad-secret' + }) + const token = await store.dispatch('ensureAppToken') + expect(token).to.eql('test-app-token') + expect(store.state.oauth.appToken).to.eql('test-app-token') + expect(store.state.oauth.clientId).to.eql('test-id') + expect(store.state.oauth.clientSecret).to.eql('test-secret') + }) + + test('it should throw if we cannot create an app', async ({ worker }) => { + worker.use( + http.post(`${testServer}/api/v1/apps`, () => { + return HttpResponse.text('Throttled', { status: 429 }) + }) + ) + + const store = getStore() + await expect(store.dispatch('ensureAppToken')).rejects.toThrowError('Throttled') + }) + + test('it should throw if we cannot obtain app token', async ({ worker }) => { + worker.use( + http.post(`${testServer}/oauth/token`, () => { + return HttpResponse.text('Throttled', { status: 429 }) + }) + ) + + const store = getStore() + await expect(store.dispatch('ensureAppToken')).rejects.toThrowError('Throttled') + }) +}) diff --git a/vite.config.js b/vite.config.js index 2abf18e5076ce0b34dd6b553b446052bb7b7a600..594046b5d41ef5516ef66a4c0fd655a822a59a06 100644 --- a/vite.config.js +++ b/vite.config.js @@ -8,6 +8,7 @@ import eslint from 'vite-plugin-eslint2' import { devSwPlugin, buildSwPlugin, swMessagesPlugin } from './build/sw_plugin.js' import copyPlugin from './build/copy_plugin.js' import { getCommitHash } from './build/commit_hash.js' +import mswPlugin from './build/msw_plugin.js' const localConfigPath = '<projectRoot>/config/local.json' const getLocalDevSettings = async () => { @@ -117,7 +118,8 @@ export default defineConfig(async ({ mode, command }) => { lintInWorker: true, lintOnStart: true, cacheLocation: resolve(projectRoot, 'node_modules/.cache/stylelintcache') - }) + }), + ...(mode === 'test' ? [mswPlugin()] : []) ], optimizeDeps: { // For unknown reasons, during vitest, vite will re-optimize the following diff --git a/yarn.lock b/yarn.lock index e7c06223336c0873ae2a36e6d373a307c26edd41..b75980da8186a191e0af75b2a0397d4dd049746d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5856,7 +5856,7 @@ ms@2.1.3, ms@^2.1.1, ms@^2.1.3: resolved "https://registry.yarnpkg.com/ms/-/ms-2.1.3.tgz#574c8138ce1d2b5861f0b44579dbadd60c6615b2" integrity sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA== -msw@^2.7.3: +msw@2.7.3, msw@^2.7.3: version "2.7.3" resolved "https://registry.yarnpkg.com/msw/-/msw-2.7.3.tgz#5ad569fae7c7cdb8be2eeba3d041c185600b25a0" integrity sha512-+mycXv8l2fEAjFZ5sjrtjJDmm2ceKGjrNbBr1durRg6VkU9fNUE/gsmQ51hWbHqs+l35W1iM+ZsmOD9Fd6lspw==