Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for OIDC session management error status #613

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,6 @@ node_modules/
.vscode/
temp/
.history/

.idea
*.iml
11 changes: 10 additions & 1 deletion docs/oidc-client-ts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export class AccessTokenEvents {

// @internal (undocumented)
export class CheckSessionIFrame {
constructor(_callback: () => Promise<void>, _client_id: string, url: string, _intervalInSeconds: number, _stopOnError: boolean);
constructor(_callback: () => Promise<void>, _client_id: string, url: string, _intervalInSeconds: number, _stopOnError: boolean, propagateUserSessionError: boolean, userSessionErrorCallback: () => void);
// (undocumented)
load(): Promise<void>;
// (undocumented)
Expand Down Expand Up @@ -941,6 +941,8 @@ export class UserManagerEvents extends AccessTokenEvents {
addSilentRenewError(cb: SilentRenewErrorCallback): () => void;
addUserLoaded(cb: UserLoadedCallback): () => void;
addUserSessionChanged(cb: UserSessionChangedCallback): () => void;
// Warning: (ae-forgotten-export) The symbol "UserSessionErrorCallback" needs to be exported by the entry point index.d.ts
addUserSessionError(cb: UserSessionErrorCallback): () => void;
addUserSignedIn(cb: UserSignedInCallback): () => void;
addUserSignedOut(cb: UserSignedOutCallback): () => void;
addUserUnloaded(cb: UserUnloadedCallback): () => void;
Expand All @@ -953,12 +955,15 @@ export class UserManagerEvents extends AccessTokenEvents {
// @internal (undocumented)
_raiseUserSessionChanged(): void;
// @internal (undocumented)
_raiseUserSessionError(): void;
// @internal (undocumented)
_raiseUserSignedIn(): void;
// @internal (undocumented)
_raiseUserSignedOut(): void;
removeSilentRenewError(cb: SilentRenewErrorCallback): void;
removeUserLoaded(cb: UserLoadedCallback): void;
removeUserSessionChanged(cb: UserSessionChangedCallback): void;
removeUserSessionError(cb: UserSessionErrorCallback): void;
removeUserSignedIn(cb: UserSignedInCallback): void;
removeUserSignedOut(cb: UserSignedOutCallback): void;
removeUserUnloaded(cb: UserUnloadedCallback): void;
Expand All @@ -983,6 +988,8 @@ export interface UserManagerSettings extends OidcClientSettings {
popupWindowFeatures?: PopupWindowFeatures;
popupWindowTarget?: string;
// (undocumented)
propagateUserSessionError?: boolean;
// (undocumented)
query_status_response_type?: string;
redirectMethod?: "replace" | "assign";
redirectTarget?: "top" | "self";
Expand Down Expand Up @@ -1024,6 +1031,8 @@ export class UserManagerSettingsStore extends OidcClientSettingsStore {
// (undocumented)
readonly popupWindowTarget: string;
// (undocumented)
readonly propagateUserSessionError: boolean;
// (undocumented)
readonly query_status_response_type: string;
// (undocumented)
readonly redirectMethod: "replace" | "assign";
Expand Down
6 changes: 6 additions & 0 deletions src/CheckSessionIFrame.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ export class CheckSessionIFrame {
url: string,
private _intervalInSeconds: number,
private _stopOnError: boolean,
private propagateUserSessionError: boolean,
private userSessionErrorCallback: () => void,
) {
const parsedUrl = new URL(url);
this._frame_origin = parsedUrl.origin;
Expand Down Expand Up @@ -55,6 +57,10 @@ export class CheckSessionIFrame {
if (this._stopOnError) {
this.stop();
}

if (this.propagateUserSessionError) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could combine propagateUserSessionError and userSessionErrorCallback: Make userSessionErrorCallback optional, if defined call it if not not. Then propagateUserSessionError is not needed inside this class...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the way I implemented it is the same pattern as the _callback is called after the changed status is triggered. By using the propagateUserSessionError flag it is made optional. If you'd rather like that the existence of the userSessionErrorCallback is checked to trigger that callback I would have to allow for a function to be defined in the settings object which, of course, would be possible but would break with the way that a client can attach to e.g. the changed status.

this.userSessionErrorCallback();
}
}
else if (e.data === "changed") {
this._logger.debug("changed message from check session op iframe");
Expand Down
40 changes: 19 additions & 21 deletions src/SessionMonitor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ export class SessionMonitor {
// doesn't trigger load event.
if (user) {
void this._start(user);
}
else if (this._userManager.settings.monitorAnonymousSession) {
} else if (this._userManager.settings.monitorAnonymousSession) {
const session = await this._userManager.querySessionStatus();
if (session) {
const tmpUser = {
Expand Down Expand Up @@ -69,8 +68,7 @@ export class SessionMonitor {
this._sub = user.profile.sub;
this._sid = user.profile.sid;
logger.debug("session_state", session_state, ", sub", this._sub);
}
else {
} else {
this._sub = undefined;
this._sid = undefined;
logger.debug("session_state", session_state, ", anonymous user");
Expand All @@ -90,21 +88,27 @@ export class SessionMonitor {
const intervalInSeconds = this._userManager.settings.checkSessionIntervalInSeconds;
const stopOnError = this._userManager.settings.stopCheckSessionOnError;

const checkSessionIFrame = new CheckSessionIFrame(this._callback, client_id, url, intervalInSeconds, stopOnError);
const checkSessionIFrame = new CheckSessionIFrame(this._callback, client_id, url, intervalInSeconds, stopOnError, this.doUserSessionErrorPropagation(), this.raiseUserSessionError.bind(this));
await checkSessionIFrame.load();
this._checkSessionIFrame = checkSessionIFrame;
checkSessionIFrame.start(session_state);
}
else {
} else {
logger.warn("no check session iframe found in the metadata");
}
}
catch (err) {
} catch (err) {
// catch to suppress errors since we're in non-promise callback
logger.error("Error from getCheckSessionIframe:", err instanceof Error ? err.message : err);
}
};

private doUserSessionErrorPropagation(): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doUserSessionErrorPropagation is only called once -> not really needed, instead add there e.g. a variable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a matter of preference, but yes, I will introduce a variable.

return this._userManager.settings.propagateUserSessionError;
}

private raiseUserSessionError(): void {
this._userManager.events._raiseUserSessionError.bind(this);
}

protected _stop = (): void => {
const logger = this._logger.create("_stop");
this._sub = undefined;
Expand Down Expand Up @@ -133,8 +137,7 @@ export class SessionMonitor {
};
void this._start(tmpUser);
}
}
catch (err) {
} catch (err) {
// catch to suppress errors since we're in a callback
logger.error("error from querySessionStatus", err instanceof Error ? err.message : err);
}
Expand All @@ -155,32 +158,27 @@ export class SessionMonitor {

if (session.sid === this._sid) {
logger.debug("same sub still logged in at OP, restarting check session iframe; session_state", session.session_state);
}
else {
} else {
logger.debug("same sub still logged in at OP, session state has changed, restarting check session iframe; session_state", session.session_state);
this._userManager.events._raiseUserSessionChanged();
}
}
else {
} else {
logger.debug("different subject signed into OP", session.sub);
}
}
else {
} else {
logger.debug("subject no longer signed into OP");
}

if (raiseEvent) {
if (this._sub) {
this._userManager.events._raiseUserSignedOut();
}
else {
} else {
this._userManager.events._raiseUserSignedIn();
}
} else {
logger.debug("no change in session detected, no event to raise");
}
}
catch (err) {
} catch (err) {
if (this._sub) {
logger.debug("Error calling queryCurrentSigninSession; raising signed out event", err);
this._userManager.events._raiseUserSignedOut();
Expand Down
50 changes: 50 additions & 0 deletions src/UserManagerEvents.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,56 @@ describe("UserManagerEvents", () => {
expect(cb).toBeCalledTimes(0);
});

it("should allow callback", () => {
// arrange
const cb = jest.fn();

// act
subject.addUserSessionError(cb);
subject._raiseUserSessionError();

// assert
expect(cb).toBeCalled();
});

it("should allow unregistering callback", () => {
// arrange
const cb = jest.fn();

// act
subject.addUserSessionError(cb);
subject.removeUserSessionError(cb);
subject._raiseUserSessionError();

// assert
expect(cb).toBeCalledTimes(0);
});

it("should allow callback", () => {
// arrange
const cb = jest.fn();

// act
subject.addUserSessionError(cb);
subject._raiseUserSessionError();

// assert
expect(cb).toBeCalled();
});

it("should allow unregistering callback", () => {
// arrange
const cb = jest.fn();

// act
subject.addUserSessionError(cb);
subject.removeUserSessionError(cb);
subject._raiseUserSessionError();

// assert
expect(cb).toBeCalledTimes(0);
});

it("should pass error to callback", () => {
// arrange
let e: Error | null = null;
Expand Down
25 changes: 24 additions & 1 deletion src/UserManagerEvents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ export type UserSignedOutCallback = () => Promise<void> | void;
* @public
*/
export type UserSessionChangedCallback = () => Promise<void> | void;

/**
* @public
*/
export type UserSessionErrorCallback = () => Promise<void> | void;
/**
* @public
*/
Expand All @@ -43,6 +46,7 @@ export class UserManagerEvents extends AccessTokenEvents {
private readonly _userSignedIn = new Event<[]>("User signed in");
private readonly _userSignedOut = new Event<[]>("User signed out");
private readonly _userSessionChanged = new Event<[]>("User session changed");
private readonly _userSessionError = new Event<[]>("User session error");

public constructor(settings: UserManagerSettingsStore) {
super({ expiringNotificationTimeInSeconds: settings.accessTokenExpiringNotificationTimeInSeconds });
Expand Down Expand Up @@ -163,4 +167,23 @@ export class UserManagerEvents extends AccessTokenEvents {
public _raiseUserSessionChanged(): void {
this._userSessionChanged.raise();
}
/**
* Add callback: Raised when the user session changed (when `monitorSession` is set).
* @see {@link UserManagerSettings.monitorSession}
*/
public addUserSessionError(cb: UserSessionErrorCallback): () => void {
return this._userSessionError.addHandler(cb);
}
/**
* Remove callback: Raised when the user session changed (when `monitorSession` is set).
*/
public removeUserSessionError(cb: UserSessionErrorCallback): void {
this._userSessionError.removeHandler(cb);
}
/**
* @internal
*/
public _raiseUserSessionError(): void {
this._userSessionError.raise();
}
}
4 changes: 4 additions & 0 deletions src/UserManagerSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export interface UserManagerSettings extends OidcClientSettings {
checkSessionIntervalInSeconds?: number;
query_status_response_type?: string;
stopCheckSessionOnError?: boolean;
propagateUserSessionError?: boolean;

/**
* The `token_type_hint`s to pass to the authority server by default (default: ["access_token", "refresh_token"])
Expand Down Expand Up @@ -109,6 +110,7 @@ export class UserManagerSettingsStore extends OidcClientSettingsStore {
public readonly checkSessionIntervalInSeconds: number;
public readonly query_status_response_type: string;
public readonly stopCheckSessionOnError: boolean;
public readonly propagateUserSessionError: boolean;

public readonly revokeTokenTypes: ("access_token" | "refresh_token")[];
public readonly revokeTokensOnSignout: boolean;
Expand Down Expand Up @@ -139,6 +141,7 @@ export class UserManagerSettingsStore extends OidcClientSettingsStore {
checkSessionIntervalInSeconds = DefaultCheckSessionIntervalInSeconds,
query_status_response_type = "code",
stopCheckSessionOnError = true,
propagateUserSessionError = false,

revokeTokenTypes = ["access_token", "refresh_token"],
revokeTokensOnSignout = false,
Expand Down Expand Up @@ -170,6 +173,7 @@ export class UserManagerSettingsStore extends OidcClientSettingsStore {
this.checkSessionIntervalInSeconds = checkSessionIntervalInSeconds;
this.stopCheckSessionOnError = stopCheckSessionOnError;
this.query_status_response_type = query_status_response_type;
this.propagateUserSessionError = propagateUserSessionError;

this.revokeTokenTypes = revokeTokenTypes;
this.revokeTokensOnSignout = revokeTokensOnSignout;
Expand Down