Skip to content

Commit

Permalink
Merge pull request #1501 from exogee-technology/fix/logging-in-with-e…
Browse files Browse the repository at this point in the history
…xpired-tokens-does-not-work

Fix / Logging in with expired tokens does not work
  • Loading branch information
thekevinbrown authored Jan 8, 2025
2 parents 209f4c3 + da4e470 commit 0bc9d0d
Show file tree
Hide file tree
Showing 18 changed files with 57 additions and 30 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/auth-end-to-end-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/auth-ui-end-to-end-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/federation-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/mysql-end-to-end-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/other-end-to-end-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/postgres-end-to-end-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- name: Setup Node.js
uses: actions/setup-node@v4
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/rest-ui-end-to-end-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/sqlite-end-to-end-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/storage-provider-end-to-end-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/windows-end-to-end-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:
- uses: pnpm/[email protected]
with:
version: 9.11.0
version: 9.15.3

- uses: actions/setup-node@v4
with:
Expand Down
35 changes: 25 additions & 10 deletions src/packages/auth/src/authentication/apollo/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,13 @@ export const authApolloPlugin = <R>(
});

if (!apiKey || !apiKey.secret) {
apiKeyVerificationFailedMessage = 'Bad Request: API Key Authentication Failed. (E0001)';
apiKeyVerificationFailedMessage = 'Bad Request: API Key Authentication Failed.';
logger.error(
`API Key Authentication Failed. No API Key was received, or it had no secret.`
);
} else if (apiKey.revoked) {
apiKeyVerificationFailedMessage = 'Bad Request: API Key Authentication Failed. (E0002)';
apiKeyVerificationFailedMessage = 'Bad Request: API Key Authentication Failed.';
logger.error({ apiKey }, `API Key Authentication Failed. API Key is revoked.`);
} else if (await verifyPassword(secret, apiKey.secret)) {
contextValue.user = new UserProfile({
id: String(apiKey.id),
Expand All @@ -183,7 +187,11 @@ export const authApolloPlugin = <R>(
contextValue.token = {};
upsertAuthorizationContext(contextValue);
} else {
apiKeyVerificationFailedMessage = 'Bad Request: API Key Authentication Failed. (E0003)';
apiKeyVerificationFailedMessage = 'Bad Request: API Key Authentication Failed.';
logger.error(
{ apiKey },
`API Key Authentication Failed. Verify password call did not succeed.`
);
}
} else {
// Ok, we are working in token land at this point. We either have the following scenarios:
Expand Down Expand Up @@ -229,7 +237,16 @@ export const authApolloPlugin = <R>(

upsertAuthorizationContext(contextValue);
} catch (err: unknown) {
logger.error({ err }, 'JWT verification failed.');
logger.error({ err }, 'JWT verification failed, treating as guest.');

// We are a guest and have not logged in yet.
contextValue.user = new UserProfile({
id: undefined,
roles: ['GUEST'],
});
upsertAuthorizationContext(contextValue);

// But we still got an error and need to tell the client to redirect to login.
tokenVerificationFailed = true;
}
}
Expand All @@ -246,9 +263,6 @@ export const authApolloPlugin = <R>(
if (apiKeyVerificationFailedMessage) {
throw new AuthenticationError(apiKeyVerificationFailedMessage);
}
if (tokenVerificationFailed) {
throw new AuthenticationError('Unauthorized: Token verification failed.');
}
},

willSendResponse: async ({ response, contextValue }) => {
Expand All @@ -267,7 +281,7 @@ export const authApolloPlugin = <R>(
}
}

//If we received a challenge error we need to redirect, set the header to tell the client to do so.
// If we received a challenge error we need to redirect, set the header to tell the client to do so.
if (errors?.some(didEncounterChallengeError)) {
logger.trace('Forbidden Error Found: setting X-Auth-Redirect header.');

Expand All @@ -288,8 +302,9 @@ export const authApolloPlugin = <R>(
);
}

// Let's check if verification has failed and redirect to login if it has
if (tokenVerificationFailed) {
// Let's check if verification has failed and redirect to login if it has, but only if this
// happens when we're not actually trying to log in.
if (tokenVerificationFailed && !contextValue.skipLoginRedirect) {
logger.trace('JWT verification failed: setting X-Auth-Redirect header.');
response.http.status = 200;
response.http.headers.set(
Expand Down
11 changes: 9 additions & 2 deletions src/packages/auth/src/authentication/methods/magic-link.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,15 @@ export class MagicLink extends BaseAuthMethod {

async verifyLoginMagicLink({
args: { username, token },
}: ResolverOptions<{ username: string; token: string }>): Promise<Token> {
return this.verifyMagicLink(username, token);
context,
}: ResolverOptions<{ username: string; token: string }, AuthorizationContext>): Promise<Token> {
const result = await this.verifyMagicLink(username, token);

// If they have an expired token while successfully trying to verify a magic link, they don't actually need to get
// redirected to login.
context.skipLoginRedirect = true;

return result;
}

async sendChallengeMagicLink({
Expand Down
6 changes: 6 additions & 0 deletions src/packages/auth/src/authentication/methods/password.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,12 @@ export class Password<D extends CredentialStorage> extends BaseAuthMethod {
if (!userProfile) throw new AuthenticationError('Login unsuccessful: Authentication failed.');

const authToken = await tokenProvider.generateToken(userProfile);

// This allows people with expired tokens to not get redirected to login while trying to log in.
// This happens at the end of this function so that we can be sure all the other checks have passed
// before we say it's ok not to redirect to login.
context.skipLoginRedirect = true;

return authToken;
}

Expand Down
1 change: 1 addition & 0 deletions src/packages/auth/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export interface AuthorizationContext extends BaseContext {
token?: string | JwtPayload;
user?: UserProfile<unknown>;
redirectUri?: URL;
skipLoginRedirect?: boolean;
}

export enum AccessType {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ describe('API Key Authentication', () => {
assert(response.body.kind === 'single');
expect(response.body.singleResult.errors).toBeDefined();
expect(response.body.singleResult.errors?.[0]?.message).toBe(
'Bad Request: API Key Authentication Failed. (E0001)'
'Bad Request: API Key Authentication Failed.'
);
});

Expand All @@ -130,7 +130,7 @@ describe('API Key Authentication', () => {
assert(response.body.kind === 'single');
expect(response.body.singleResult.errors).toBeDefined();
expect(response.body.singleResult.errors?.[0]?.message).toBe(
'Bad Request: API Key Authentication Failed. (E0002)'
'Bad Request: API Key Authentication Failed.'
);
});

Expand All @@ -151,7 +151,7 @@ describe('API Key Authentication', () => {
assert(response.body.kind === 'single');
expect(response.body.singleResult.errors).toBeDefined();
expect(response.body.singleResult.errors?.[0]?.message).toBe(
'Bad Request: API Key Authentication Failed. (E0003)'
'Bad Request: API Key Authentication Failed.'
);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,7 @@ describe('Security', () => {
});

assert(response.body.kind === 'single');
expect(response.body.singleResult.errors?.[0]?.message).toEqual(
'Unauthorized: Token verification failed.'
);
expect(response.body.singleResult.data).toEqual({ tags: null });
expect(response.http.headers.get('X-Auth-Redirect')).toBe(
`${process.env.AUTH_BASE_URI}/auth/login?redirect_uri=${encodeURIComponent(
process.env.AUTH_BASE_URI + '/'
Expand Down

0 comments on commit 0bc9d0d

Please sign in to comment.