Skip to content

Commit

Permalink
Removed OpenID token fetching from OAuth flow (directus#11079)
Browse files Browse the repository at this point in the history 
* Removed OpenID token fetching from OAuth and unified logging

* Removed full-stops from logging
  • Loading branch information
@aidenfoxx
aidenfoxx authored Jan 17, 2022
1 parent 333d93b commit 35564ff
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 24 deletions.
32 changes: 10 additions & 22 deletions api/src/auth/drivers/oauth2.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class OAuth2AuthDriver extends LocalAuthDriver {

const { authorizeUrl, accessUrl, profileUrl, clientId, clientSecret, ...additionalConfig } = config;

if (!authorizeUrl || !accessUrl || !clientId || !clientSecret || !additionalConfig.provider) {
if (!authorizeUrl || !accessUrl || !profileUrl || !clientId || !clientSecret || !additionalConfig.provider) {
throw new InvalidConfigException('Invalid provider config', { provider: additionalConfig.provider });
}

Expand All @@ -44,8 +44,7 @@ export class OAuth2AuthDriver extends LocalAuthDriver {
authorization_endpoint: authorizeUrl,
token_endpoint: accessUrl,
userinfo_endpoint: profileUrl,
// Required for openid providers (openid flow should be preferred!)
issuer: additionalConfig.issuerUrl,
issuer: additionalConfig.provider,
});

this.client = new issuer.Client({
Expand Down Expand Up @@ -105,15 +104,7 @@ export class OAuth2AuthDriver extends LocalAuthDriver {
{ code: payload.code, state: payload.state },
{ code_verifier: payload.codeVerifier, state: generators.codeChallenge(payload.codeVerifier) }
);

const issuer = this.client.issuer;
if (issuer.metadata.userinfo_endpoint) {
userInfo = await this.client.userinfo(tokenSet.access_token!);
} else if (tokenSet.id_token) {
userInfo = tokenSet.claims();
} else {
throw new InvalidConfigException('OAuth profile URL not defined', { provider: this.config.provider });
}
userInfo = await this.client.userinfo(tokenSet.access_token!);
} catch (e) {
throw handleError(e);
}
Expand All @@ -125,7 +116,7 @@ export class OAuth2AuthDriver extends LocalAuthDriver {
const identifier = (userInfo[identifierKey] as string | null | undefined) ?? email;

if (!identifier) {
logger.warn(`Failed to find user identifier for provider "${this.config.provider}"`);
logger.warn(`[OAuth2] Failed to find user identifier for provider "${this.config.provider}"`);
throw new InvalidCredentialsException();
}

Expand Down Expand Up @@ -171,7 +162,7 @@ export class OAuth2AuthDriver extends LocalAuthDriver {
try {
authData = JSON.parse(authData);
} catch {
logger.warn(`Session data isn't valid JSON: ${authData}`);
logger.warn(`[OAuth2] Session data isn't valid JSON: ${authData}`);
}
}

Expand All @@ -194,26 +185,24 @@ export class OAuth2AuthDriver extends LocalAuthDriver {
const handleError = (e: any) => {
if (e instanceof errors.OPError) {
if (e.error === 'invalid_grant') {
logger.trace(e, `[OAuth2] Invalid grant.`);
// Invalid token
logger.trace(e, `[OAuth2] Invalid grant`);
return new InvalidTokenException();
}

logger.trace(e, `[OAuth2] Unknown OP error.`);

// Server response error
logger.trace(e, `[OAuth2] Unknown OP error`);
return new ServiceUnavailableException('Service returned unexpected response', {
service: 'oauth2',
message: e.error_description,
});
} else if (e instanceof errors.RPError) {
// Internal client error
logger.trace(e, `[OAuth2] Unknown RP error.`);
logger.trace(e, `[OAuth2] Unknown RP error`);
return new InvalidCredentialsException();
}

logger.trace(e, `[OAuth2] Unknown error.`);

logger.trace(e, `[OAuth2] Unknown error`);
return e;
};

Expand Down Expand Up @@ -274,7 +263,7 @@ export function createOAuth2AuthRouter(providerName: string): Router {
res.clearCookie(`oauth2.${providerName}`);

if (!req.query.code || !req.query.state) {
logger.warn(`[OAuth2]Couldn't extract OAuth2 code or state from query: ${JSON.stringify(req.query)}`);
logger.warn(`[OAuth2] Couldn't extract OAuth2 code or state from query: ${JSON.stringify(req.query)}`);
}

authResponse = await authenticationService.login(providerName, {
Expand Down Expand Up @@ -305,7 +294,6 @@ export function createOAuth2AuthRouter(providerName: string): Router {
}

logger.warn(error, `[OAuth2] Unexpected error during OAuth2 login`);

throw error;
}

Expand Down
17 changes: 15 additions & 2 deletions api/src/auth/drivers/openid.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ export class OpenIDAuthDriver extends LocalAuthDriver {

async getUserID(payload: Record<string, any>): Promise<string> {
if (!payload.code || !payload.codeVerifier) {
logger.trace('[OpenID] No code or codeVerifier in payload');
throw new InvalidCredentialsException();
}

Expand Down Expand Up @@ -132,7 +133,7 @@ export class OpenIDAuthDriver extends LocalAuthDriver {
const identifier = (userInfo[identifierKey ?? 'sub'] as string | null | undefined) ?? email;

if (!identifier) {
logger.warn(`Failed to find user identifier for provider "${this.config.provider}"`);
logger.warn(`[OpenID] Failed to find user identifier for provider "${this.config.provider}"`);
throw new InvalidCredentialsException();
}

Expand All @@ -152,6 +153,9 @@ export class OpenIDAuthDriver extends LocalAuthDriver {

// Is public registration allowed?
if (!allowPublicRegistration || !isEmailVerified) {
logger.trace(
`[OpenID] User doesn't exist, and public registration not allowed for provider "${this.config.provider}"`
);
throw new InvalidCredentialsException();
}

Expand Down Expand Up @@ -179,7 +183,7 @@ export class OpenIDAuthDriver extends LocalAuthDriver {
try {
authData = JSON.parse(authData);
} catch {
logger.warn(`Session data isn't valid JSON: ${authData}`);
logger.warn(`[OpenID] Session data isn't valid JSON: ${authData}`);
}
}

Expand All @@ -204,17 +208,23 @@ const handleError = (e: any) => {
if (e instanceof errors.OPError) {
if (e.error === 'invalid_grant') {
// Invalid token
logger.trace(e, `[OpenID] Invalid grant`);
return new InvalidTokenException();
}

// Server response error
logger.trace(e, `[OpenID] Unknown OP error`);
return new ServiceUnavailableException('Service returned unexpected response', {
service: 'openid',
message: e.error_description,
});
} else if (e instanceof errors.RPError) {
// Internal client error
logger.trace(e, `[OpenID] Unknown RP error`);
return new InvalidCredentialsException();
}

logger.trace(e, `[OpenID] Unknown error`);
return e;
};

Expand Down Expand Up @@ -300,11 +310,14 @@ export function createOpenIDAuthRouter(providerName: string): Router {
reason = 'INVALID_USER';
} else if (error instanceof InvalidTokenException) {
reason = 'INVALID_TOKEN';
} else {
logger.warn(error, `[OpenID] Unexpected error during OpenID login`);
}

return res.redirect(`${redirect.split('?')[0]}?reason=${reason}`);
}

logger.warn(error, `[OpenID] Unexpected error during OpenID login`);
throw error;
}

Expand Down

0 comments on commit 35564ff

Please sign in to comment.