Skip to content

Commit

Permalink
Merge pull request #1494 from bcgov/UTOPIA-1389-b
Browse files Browse the repository at this point in the history
Improved logout implementation
  • Loading branch information
BradyMitch authored Aug 25, 2023
2 parents 54e1efc + 6cd5001 commit 8209681
Show file tree
Hide file tree
Showing 13 changed files with 94 additions and 178 deletions.
1 change: 1 addition & 0 deletions src/backend/.config/.env.local
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ KEYCLOAK_CLIENT_SECRET=[GET_FROM_SSO_PORTAL]
KEYCLOAK_USER_INFO_URI=[GET_FROM_SSO_PORTAL]
KEYCLOAK_TOKEN_URI=https://dev.loginproxy.gov.bc.ca/auth/realms/standard/protocol/openid-connect/token
KEYCLOAK_LOGOUT_URI=https://dev.loginproxy.gov.bc.ca/auth/realms/standard/protocol/openid-connect/logout
SITEMINDER_LOGOUT_URI=https://logontest7.gov.bc.ca/clp-cgi/logoff.cgi

################### gcnotify settings ###############
GCNOTIFY_BASE_URL='https://api.notification.canada.ca'
Expand Down
1 change: 1 addition & 0 deletions src/backend/src/config/config.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,6 @@ const configService = new ConfigServiceClass(process.env).ensureValues([
'GCNOTIFY_BASE_URL',
'GCNOTIFY_API_KEY',
'GCNOTIFY_TEMPLATE_ID',
'SITEMINDER_LOGOUT_URI',
]);
export { configService };
22 changes: 8 additions & 14 deletions src/backend/src/modules/auth/auth.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import {
Body,
Controller,
Get,
HttpCode,
HttpException,
HttpStatus,
InternalServerErrorException,
Expand Down Expand Up @@ -67,19 +66,14 @@ export class AuthController {
}
}

@Post('logout')
@Get('keycloakLogout')
@Unprotected()
@HttpCode(HttpStatus.NO_CONTENT)
@ApiOkResponse({ description: 'Logout from keycloak sso server' })
async logout(@Body() token: AppTokensDto) {
try {
await this.authService.logout(token.refresh_token);
} catch (e) {
if (e instanceof HttpException) {
throw new HttpException('Logout failed', e.getStatus());
}
throw new InternalServerErrorException('Logout failed');
}
return;
@ApiOkResponse({
description: 'Redirect to keycloak sso server logout',
})
logout(@Req() req) {
const idToken: string = req.query.id_token;
const redirectUrl: string = req.query.redirect_url;
return this.authService.getUrlLogout(idToken, redirectUrl);
}
}
42 changes: 13 additions & 29 deletions src/backend/src/modules/auth/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ export class AuthService {

private keycloakLogoutUri: string;

private siteMinderLogoutUri: string;

constructor(private readonly httpService: HttpService) {
this.keycloakAuthServerUri = configService.getValue(
'KEYCLOAK_AUTH_SERVER_URI',
Expand All @@ -46,6 +48,7 @@ export class AuthService {
this.keycloakUserInfoUri = configService.getValue('KEYCLOAK_USER_INFO_URI');
this.keycloakTokenUri = configService.getValue('KEYCLOAK_TOKEN_URI');
this.keycloakLogoutUri = configService.getValue('KEYCLOAK_LOGOUT_URI');
this.siteMinderLogoutUri = configService.getValue('SITEMINDER_LOGOUT_URI');
}

getUrlLogin(): any {
Expand Down Expand Up @@ -84,6 +87,7 @@ export class AuthService {
res.data.refresh_token,
res.data.expires_in,
res.data.refresh_expires_in,
res.data.id_token,
),
),
catchError((e) => {
Expand Down Expand Up @@ -136,6 +140,7 @@ export class AuthService {
res.data.refresh_token,
res.data.expires_in,
res.data.refresh_expires_in,
res.data.id_token,
),
),
catchError((e) => {
Expand All @@ -154,36 +159,15 @@ export class AuthService {
return data;
}

async logout(refresh_token: string) {
const params = {
client_id: this.keycloakClientId,
client_secret: this.keycloakClientSecret,
refresh_token: refresh_token,
};

const data = await firstValueFrom(
this.httpService
.post(
this.keycloakLogoutUri,
queryString.stringify(params),
this.getContentType(),
)
.pipe(
map((res: any) => res.data),
catchError((e) => {
console.error(
'auth.service.ts:logout:data',
e?.response?.data || 'Error data unknown, Something Went wrong',
e?.response?.status || 500,
);
throw new HttpException(
e?.response?.data || 'Error data unknown, Something Went wrong',
e?.response?.status || 500,
);
}),
),
getUrlLogout(idToken: string, redirectUrl: string): any {
const keycloakLogoutUri = encodeURIComponent(
`${this.keycloakLogoutUri}?id_token_hint=${idToken}` +
`&post_logout_redirect_uri=${redirectUrl}`,
);
return data;
return {
siteMinderUrl: `${this.siteMinderLogoutUri}?retnow=1&returl=`,
keycloakUrl: keycloakLogoutUri,
};
}

getContentType() {
Expand Down
4 changes: 4 additions & 0 deletions src/backend/src/modules/auth/keycloack-token.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,19 @@ export class KeycloakToken {

refresh_expires_in: string;

id_token: string;

constructor(
access_token: string,
refresh_token: string,
expires_in: string,
refresh_expires_in: string,
id_token: string,
) {
this.access_token = access_token;
this.refresh_token = refresh_token;
this.expires_in = expires_in;
this.refresh_expires_in = refresh_expires_in;
this.id_token = id_token;
}
}
56 changes: 11 additions & 45 deletions src/backend/test/unit/auth/auth.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,63 +237,29 @@ describe('AuthController', () => {
});
});

describe('logout', () => {
describe('keycloakLogout', () => {
/**
* @Description
* This test validates the happy flow if the method `authService.logout` is called with correct mock data
* This test validates the happy flow if the method `authService.getUrlLogout` is called with correct mock data
*
* @Input
* - AppTokensDto
*
* @Output 200
* - no output
*/
it('should call the logout method of the AuthService', async () => {
const appTokensDto: AppTokensDto = { ...appTokensDtoMock };
it('should call the getUrlLogout method of the AuthService', async () => {
const req = {
query: { id_token: '500', redirect_url: 'http://app.example.com' },
};

service.logout = jest.fn().mockResolvedValue(undefined);
service.getUrlLogout = jest.fn().mockResolvedValue(undefined);

await controller.logout(appTokensDto);
expect(service.logout).toHaveBeenCalledWith(appTokensDto.refresh_token);
});
/**
* @Description
* This test validates the happy flow if the method `authService.logout` is failed with http exception
*
* @Input
* - AppTokensDto
*
* @Output 400
* it should return the http exception when service method failed
*/
it('should throw HttpException when authService.logout() throws HttpException', async () => {
const appTokensDto: AppTokensDto = { ...appTokensDtoMock };
const httpException = new HttpException('Logout failed', 400);
service.logout = jest.fn().mockRejectedValue(httpException);
await expect(controller.logout(appTokensDto)).rejects.toThrowError(
httpException,
);
expect(service.logout).toHaveBeenCalledWith(appTokensDto.refresh_token);
});
/**
* @Description
* This test validates the non-happy flow if the method `authService.logout` is throw exception
*
* @Input
* - AppTokensDto
*
* @Output 500
* it should return the InternalServerErrorException
*/
it('should throw InternalServerErrorException when authService.logout() throws other errors', async () => {
service.logout = jest
.fn()
.mockRejectedValue(new Error('Some other error'));
const appTokensDto: AppTokensDto = { ...appTokensDtoMock };
await expect(controller.logout(appTokensDto)).rejects.toThrowError(
InternalServerErrorException,
await controller.logout(req);
expect(service.getUrlLogout).toHaveBeenCalledWith(
'500',
'http://app.example.com',
);
expect(service.logout).toHaveBeenCalledWith(appTokensDto.refresh_token);
});
});
});
72 changes: 19 additions & 53 deletions src/backend/test/unit/auth/auth.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ describe('AuthService', () => {
KEYCLOAK_REALM: 'realm',
KEYCLOAK_USER_INFO_URI: 'https://app.example.com/user',
KEYCLOAK_LOGOUT_URI: 'https://app.example.com/logout',
SITEMINDER_LOGOUT_URI: 'https://siteminder.example.com/logoff',
});
jest
.spyOn(configService, 'getValue')
Expand Down Expand Up @@ -71,7 +72,7 @@ describe('AuthService', () => {
*/
it('should return the correct login URL', () => {
const expectedUrl = `https://app.example.com/oauth/realms/realm/protocol/openid-connect/auth?client_id=myclientid&response_type=idir&scope=scope&redirect_uri=https://app.example.com/callback`;
expect(configService.getValue).toHaveBeenCalledTimes(10);
expect(configService.getValue).toHaveBeenCalledTimes(11);
const result = service.getUrlLogin();
expect(result.url).toEqual(expectedUrl);
});
Expand All @@ -95,6 +96,7 @@ describe('AuthService', () => {
'refresh_token',
'300',
'1800',
'500',
);

const response: any = {
Expand Down Expand Up @@ -214,6 +216,7 @@ describe('AuthService', () => {
'new_refresh_token',
'300',
'1800',
'500',
);

const response: any = {
Expand Down Expand Up @@ -268,68 +271,31 @@ describe('AuthService', () => {
});
});

describe('logout', () => {
describe('getLogoutURL', () => {
/**
* This test validates that the it should successful logout from OAuth server(bc keycloak instance)
* This test validates that the it can return correct OAuth logout URL.
*
* @Input
* - refreshToken
*
*
* @Output
* - empty output
*/
it('should logout user', async () => {
const refreshToken = 'sample_refresh_token';

const response: any = {
data: null,
status: 204,
statusText: 'No Content',
config: {},
headers: {},
};

jest
.spyOn(httpService, 'post')
.mockImplementationOnce(() => of(response));

await expect(service.logout(refreshToken)).resolves.toBeNull();
});
/**
* This test validates that the it should throw exception when got error logout from OAuth server(bc keycloak instance)
*
* @Input
* - refreshToken
* - empty
*
*
* @Output
* - an http exception
* - an object containing url
*/
it('should throw HttpException with 500 status when httpService.post throws other errors', async () => {
const error: any = {
message: JSON.stringify(new Error('Some other error')),
AxiosError: true,
};
const mockRefreshToken = 'sample_refresh_token';
jest
.spyOn(httpService, 'post')
.mockReturnValueOnce(throwError(() => error));
it('should return the correct login URL', () => {
const idToken = '500';
const redirectUrl = 'https://app.example.com';

await expect(service.logout(mockRefreshToken)).rejects.toThrowError(
HttpException,
const expectedKCUrl = encodeURIComponent(
`https://app.example.com/logout?id_token_hint=${idToken}&post_logout_redirect_uri=${redirectUrl}`,
);
const expectedSMUrl =
'https://siteminder.example.com/logoff?retnow=1&returl=';

expect(consoleErrorSpy).toHaveBeenCalledWith(
'auth.service.ts:logout:data',
'Error data unknown, Something Went wrong',
500,
);
expect(httpService.post).toHaveBeenCalledWith(
expect.any(String),
expect.stringContaining(`refresh_token=${mockRefreshToken}`),
expect.any(Object),
);
expect(configService.getValue).toHaveBeenCalledTimes(11);
const result = service.getUrlLogout(idToken, redirectUrl);
expect(result.siteMinderUrl).toEqual(expectedSMUrl);
expect(result.keycloakUrl).toEqual(expectedKCUrl);
});
});
});
2 changes: 1 addition & 1 deletion src/frontend/src/components/common/Header/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function Header({ user }: Props) {
const config = await HttpRequest.get<IConfig>(API_ROUTES.CONFIG_FILE);
AppStorage.setItem(ConfigStorageKeys.CONFIG, config);

navigate(AppStorage.getItem('returnUri') || routes.PIA_LIST);
navigate(routes.PIA_LIST);
} else {
throw new Error('Invalid Token Information found');
}
Expand Down
11 changes: 1 addition & 10 deletions src/frontend/src/components/common/Unauthorized/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import UnAuthorizedImg from '../../../assets/401.svg';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
import { faUser } from '@fortawesome/free-solid-svg-icons';
import { API_ROUTES } from '../../../constant/apiRoutes';
import { AppStorage } from '../../../utils/storage';

const Unauthorized = () => {
const win: Window = window;
Expand All @@ -12,10 +11,6 @@ const Unauthorized = () => {
win.location = API_ROUTES.KEYCLOAK_LOGIN;
};

const clearReturnUri = () => {
AppStorage.removeItem('returnUri');
};

return (
<div className="bcgovPageContainer notfound-container">
<div className="container">
Expand Down Expand Up @@ -43,11 +38,7 @@ const Unauthorized = () => {
Log in with IDIR
<FontAwesomeIcon className="icon" icon={faUser} />
</button>
<Link
className="bcgovbtn bcgovbtn__secondary"
to="/"
onClick={clearReturnUri}
>
<Link className="bcgovbtn bcgovbtn__secondary" to="/">
Go to Home
</Link>
</div>
Expand Down
2 changes: 1 addition & 1 deletion src/frontend/src/constant/apiRoutes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ export const API_ROUTES = {
PIA_INTAKE: '/api/pia-intake',
KEYCLOAK_CALLBACK: 'api/auth/callback',
KEYCLOAK_USER: '/api/auth/user',
KEYCLOAK_LOGOUT: '/api/auth/logout',
KEYCLOAK_LOGOUT: '/api/auth/keycloakLogout',
KEYCLOAK_LOGIN: '/api/auth/keycloakLogin',
KEYCLOAK_REFRESH_TOKEN: '/api/auth/refreshToken',
PPQ_RESULT_DOWNLOAD: '/api/ppq/download/:id',
Expand Down
Loading

0 comments on commit 8209681

Please sign in to comment.