From: Justin Wind Date: Tue, 17 Jun 2025 17:07:30 +0000 (-0700) Subject: auth cleanups X-Git-Url: https://git.squeep.com/?a=commitdiff_plain;ds=sidebyside;p=urlittler auth cleanups --- diff --git a/src/authenticator.js b/src/authenticator.js index a3a0638..eeb6f20 100644 --- a/src/authenticator.js +++ b/src/authenticator.js @@ -119,7 +119,7 @@ class Authenticator { await this.db.context(async (dbCtx) => { authData = await this.db.getAuthById(dbCtx, authenticationId); }); - const secret = authData && authData.secret; + const secret = authData?.secret; if (!secret) { this.logger.debug(_scope, 'failed, invalid authenticationId', { ctx }); @@ -171,20 +171,26 @@ class Authenticator { return false; } - // Update pwhash - // authData.password = await argon2.hash(newPassword, { type: argon2.id }); if (authData.password.startsWith('$argon2')) { if (await argon2.verify(authData.password, authenticationPass)) { + this.logger.debug(_scope, 'passed argon2 verify', { ctx }); + } else { this.logger.debug(_scope, 'failed argon2 verify', { ctx }); return false; - } else { - this.logger.debug(_scope, 'passed argon2 verify', { ctx }); } } else { - if (authData.password !== authenticationPass) { + if (authData.password.length !== authenticationPass.length + || !crypto.timingSafeEqual(Buffer.from(authData.password), Buffer.from(authenticationPass)) + ) { this.logger.debug(_scope, 'failed, password mismatch', { ctx }); return false; } + // Update pwhash + const credential = await argon2.hash(authenticationPass, { type: argon2.argon2id }); + await this.db.context(async (dbCtx) => { + await this.db.upsertAuth(dbCtx, authenticationId, authData.secret, credential); + }); + this.logger.debug(_scope, 'migrated plain password', { ctx, authenticationId }); } ctx.authenticationId = authenticationId; @@ -319,7 +325,7 @@ class Authenticator { return this.requestBasic(res); } - const linkId = ctx.params && ctx.params.id; + const linkId = ctx?.params?.id; // If there is an id parameter, check for a valid token query parameter if (linkId) { authData = (ctx?.queryParams?.token) || (ctx?.parsedBody?.token); @@ -367,9 +373,9 @@ class Authenticator { } // Allow a valid plain token. - const linkId = ctx.params && ctx.params.id; + const linkId = ctx?.params?.id; if (linkId) { - const token = (ctx.queryParams && ctx.queryParams.token) || (ctx.parsedBody && ctx.parsedBody.token); + const token = ctx?.queryParams?.token || ctx?.parsedBody?.token; if (token) { const validToken = await this.isValidToken(linkId, token); if (validToken) { diff --git a/src/db/base.js b/src/db/base.js index b109efd..6bce83b 100644 --- a/src/db/base.js +++ b/src/db/base.js @@ -39,6 +39,10 @@ class BaseDatabase { this._notImplemented('getAuthById', { dbCtx, id }); } + async upsertAuth(dbCtx, id, secert, credential) { + this._notImplemented('upsertAuthCredential', { dbCtx, id, credential }); + } + async insertLink(dbCtx, id, url, authToken) { this._notImplemented('insertLink', { dbCtx, id, url, authToken }); } diff --git a/src/db/postgres/index.js b/src/db/postgres/index.js index e46a401..7549133 100644 --- a/src/db/postgres/index.js +++ b/src/db/postgres/index.js @@ -198,6 +198,16 @@ class PostgresDatabase extends BaseDatabase { } + async upsertAuth(dbCtx, id, secret, credential) { + const _scope = _fileScope('upsertAuth'); + this.logger.debug(_scope, 'called', { id }); + dbCtx = dbCtx || this.db; + + const result = await dbCtx.result(this.statement.authUpsert, { id, secret, credential }); + this.logger.debug(_scope, 'result', PostgresDatabase._resultLog(result) ); + } + + static _epochFix(epoch) { switch (epoch) { case Infinity: diff --git a/src/db/postgres/sql/auth-upsert.sql b/src/db/postgres/sql/auth-upsert.sql new file mode 100644 index 0000000..f90be73 --- /dev/null +++ b/src/db/postgres/sql/auth-upsert.sql @@ -0,0 +1,4 @@ +-- +INSERT INTO auth (id, secret, password) VALUES ($(id), $(secret), $(credential)) +ON CONFLICT (id) DO +UPDATE SET password = $(credential), secret = $(secret) diff --git a/src/db/sqlite/index.js b/src/db/sqlite/index.js index 39b9b6b..e1ce14d 100644 --- a/src/db/sqlite/index.js +++ b/src/db/sqlite/index.js @@ -93,6 +93,8 @@ class SQLiteDatabase extends BaseDatabase { _commit: this.db.prepare('COMMIT'), _rollback: this.db.prepare('ROLLBACK'), getAuthById: this.db.prepare('SELECT * FROM auth WHERE id = :id'), + insertAuth: this.db.prepare('INSERT INTO auth (id, secret, password) VALUES (:id, :secret, :credential)'), + updateAuth: this.db.prepare('UPDATE auth SET password = :credential, secret = :secret WHERE id = :id'), getLinkById: this.db.prepare('SELECT * FROM link WHERE id = :id'), getLinkByUrl: this.db.prepare('SELECT * FROM link WHERE url = :url'), insertLink: this.db.prepare('INSERT INTO link (id, url, auth_token) VALUES (:id, :url, :authToken)'), @@ -168,6 +170,37 @@ class SQLiteDatabase extends BaseDatabase { return auth; } + async upsertAuth(dbCtx, id, secret, credential) { + const _scope = _fileScope('upsertAuthCredential'); + this.logger.debug(_scope, 'called', { id }); + + let info; + try { + info = this.statement.insertAuth.run({ id, secret, credential }); + } catch (e) { + switch (e.code) { + case 'SQLITE_CONSTRAINT_UNIQUE': + case 'SQLITE_CONSTRAINT_PRIMARYKEY': { + this.logger.debug(_scope, 'updating existing auth', { id }); + info = this.statement.updateAuth.run({ id, secret, credential }); + break; + } + + default: { + this.logger.error(_scope, 'failed to upsert auth credential', { error: e, id }); + throw e; + } + } + } + this.logger.debug(_scope, 'run', { info }); + if (info.changes != 1) { + this.logger.error(_scope, 'failed to upsert auth credential', { id, info }); + throw new DBErrors.UnexpectedResult(); + } + + return this._sqliteInfo(info); + } + async insertLink(dbCtx, id, url, authToken) { const _scope = _fileScope('insertLink'); this.logger.debug(_scope, 'called', { id, url }); diff --git a/test/src/authenticator.js b/test/src/authenticator.js index 42629de..2323098 100644 --- a/test/src/authenticator.js +++ b/test/src/authenticator.js @@ -19,6 +19,7 @@ describe('Authenticator', function () { context: async (fn) => fn({}), getAuthById: async () => {}, getLinkById: async () => {}, + upsertAuth: async () => {}, }; authenticator = new Authenticator(logger, db, options); }); @@ -182,29 +183,50 @@ describe('Authenticator', function () { beforeEach(function () { sinon.stub(authenticator, 'requestBasic'); + sinon.stub(authenticator.db, 'getAuthById'); + sinon.stub(authenticator.db, 'upsertAuth'); credentials = 'id:password'; ctx = {}; }); - it('accepts credentials', async function () { - sinon.stub(authenticator.db, 'getAuthById').resolves({ password: 'password' }); + it('accepts plain credential and migrates to hash', async function () { + authenticator.db.getAuthById.resolves({ password: 'password' }); const result = await authenticator.isValidBasic(credentials, ctx); assert.strictEqual(result, true); assert.strictEqual(ctx.authenticationId, 'id'); + assert(authenticator.db.upsertAuth.called); }); - it('rejects wrong password', async function () { - sinon.stub(authenticator.db, 'getAuthById').resolves({ password: 'wrong_password' }); + it('rejects wrong plain credential', async function () { + authenticator.db.getAuthById.resolves({ password: 'wrong_password' }); const result = await authenticator.isValidBasic(credentials, ctx); assert.strictEqual(result, false); assert(!('authenticationId' in ctx)); + assert(authenticator.db.upsertAuth.notCalled); + }); + + it('accepts argon2 credential', async function () { + authenticator.db.getAuthById.resolves({ password: '$argon2id$v=19$m=65536,t=3,p=4$AQKIWU5puGDs3zKIPMo0Ew$Mzl/kzJE6/oRtJLHoGXaoUtlAiXs5HK2qLgHWF6euF8' }); + const result = await authenticator.isValidBasic(credentials, ctx); + assert.strictEqual(result, true); + assert.strictEqual(ctx.authenticationId, 'id'); + assert(authenticator.db.upsertAuth.notCalled); + }); + + it('rejects wrong argon2 credential', async function () { + authenticator.db.getAuthById.resolves({ password: '$argon2id$v=19$m=65536,t=3,p=4$BCPlf0NBgjyXOxdyUDs/FQ$wV4jERm50yByCpSr8lrD8Nu0uVPUsQcVghJQoix5ido' }); + const result = await authenticator.isValidBasic(credentials, ctx); + assert.strictEqual(result, false); + assert(!('authenticationId' in ctx)); + assert(authenticator.db.upsertAuth.notCalled); }); it('rejects missing id', async function () { - sinon.stub(authenticator.db, 'getAuthById').resolves(); + authenticator.db.getAuthById.resolves(); const result = await authenticator.isValidBasic(credentials, ctx); assert.strictEqual(result, false); assert(!('authenticationId' in ctx)); + assert(authenticator.db.upsertAuth.notCalled); }); }); // isValidBasic diff --git a/test/src/db/base.js b/test/src/db/base.js index 72d96ac..9752b6a 100644 --- a/test/src/db/base.js +++ b/test/src/db/base.js @@ -42,6 +42,7 @@ describe('BaseDatabase', function () { 'context', 'transaction', 'getAuthById', + 'upsertAuth', 'insertLink', 'getLinkById', 'getLinkByUrl', diff --git a/test/src/db/postgres/index.js b/test/src/db/postgres/index.js index d52fc90..61825ad 100644 --- a/test/src/db/postgres/index.js +++ b/test/src/db/postgres/index.js @@ -44,6 +44,13 @@ describe('PostgresDatabase', function () { dbCtx = undefined; }); + it('covers constructor options', function () { + options = { + queryLogLevel: 'debug', + }; + db = new PostgresDatabase(logger, options, pgpStub); + }); + describe('context', function () { it('covers', async function () { const fn = sinon.stub(); @@ -86,6 +93,18 @@ describe('PostgresDatabase', function () { }); }); // getAuthById + describe('upsertAuth', function () { + let id, secret, credential; + beforeEach(function () { + id = 'id'; + secret = 'secret'; + credential = 'credential'; + }); + it('stubbed success', async function () { + await db.upsertAuth(dbCtx, id, secret, credential); + }); + }); // upsertAuth + describe('_epochFix', function () { it('clamps infinity', function () { const epoch = Infinity; diff --git a/test/src/db/sqlite/index.js b/test/src/db/sqlite/index.js index b73b34f..f437482 100644 --- a/test/src/db/sqlite/index.js +++ b/test/src/db/sqlite/index.js @@ -35,6 +35,16 @@ describe('SQLiteDatabase', function () { await db.transaction(dbCtx, fn); assert(fn.called); }); + it('covers rollback', async function () { + const fn = sinon.stub(); + fn.throws(new Error('rollback')); + try { + await db.transaction(dbCtx, fn); + assert.fail(noExpectedException); + } catch (e) { + assert.strictEqual(e.message, 'rollback', noExpectedException); + } + }); }); // transaction describe('getAuthById', function () { @@ -67,6 +77,45 @@ describe('SQLiteDatabase', function () { }); }); // getAuthById + describe('upsertAuth', function () { + let id, secret, credential; + beforeEach(function () { + sinon.stub(db.statement.insertAuth, 'run').returns({ changes: 1n, lastInsertRowid: 123n }); + sinon.stub(db.statement.updateAuth, 'run').returns({ changes: 1n, lastInsertRowid: 123n }); + }); + it('stubbed insert success', async function () { + await db.upsertAuth(dbCtx, id, secret, credential); + }); + it('stubbed update success', async function () { + db.statement.insertAuth.run.throws({ code: 'SQLITE_CONSTRAINT_UNIQUE' }); + await db.upsertAuth(dbCtx, id, secret, credential); + }); + it('covers error', async function () { + const expectedException = new Error('blah'); + db.statement.insertAuth.run.throws(expectedException); + try { + await db.upsertAuth(dbCtx, id, secret, credential); + assert.fail(noExpectedException); + } catch (e) { + assert.deepStrictEqual(e, expectedException, noExpectedException); + } + }); + it('covers unexpected error', async function () { + const expectedException = DBErrors.UnexpectedResult; + const returns = { + changes: 0n, + lastInsertRowid: undefined, + }; + db.statement.insertAuth.run.returns(returns); + try { + await db.upsertAuth(dbCtx, id, secret, credential); + assert.fail(noExpectedException); + } catch (e) { + assert(e instanceof expectedException, noExpectedException); + } + }); + }); // upsertAuth + describe('insertLink', function () { let id, url, authToken; beforeEach(function () {