fix: UserSessionChanged should be raised regardless of sid claim. Sid…

[bachratyg]

… is not part of the session management spec.

Closes/fixes #1242

Checklist

• This PR makes changes to the public API
• I have included links for closing relevant issue numbers

[pamapa]

CI pipline fails with:

You have changed the public API signature for this project. Please copy the file "temp/oidc-client-ts.api.md" to "docs/oidc-client-ts.api.md", or perform a local build (which does this automatically). See the Git repo documentation for more info.

and with

npm run lint

> [email protected] lint
> eslint --max-warnings=0 --cache .

/home/runner/work/oidc-client-ts/oidc-client-ts/src/SessionMonitor.ts
Error:   57:36  error  Unexpected separator (;)  @typescript-eslint/member-delimiter-style

please resolve

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (9f94cde) 78.17% compared to head (05b26ec) 78.40%.
Report is 4 commits behind head on main.

❗ Current head 05b26ec differs from pull request most recent head 630a680. Consider uploading reports for the commit 630a680 to get more accurate results

Files	Patch %	Lines
src/SessionMonitor.ts	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1247      +/-   ##
==========================================
+ Coverage   78.17%   78.40%   +0.22%     
==========================================
  Files          45       45              
  Lines        1732     1727       -5     
  Branches      346      345       -1     
==========================================
  Hits         1354     1354              
+ Misses        341      336       -5     
  Partials       37       37              

Flag	Coverage Δ
unittests	78.40% <0.00%> (+0.22%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pamapa]

I just found that sid is also part of SessionStatus returned in querySessionStatus. We should either remove it form there as well or as an alternative we make sid optional: If it is defined consider it in the SessionMonitor else ignore it. What do you think about the alternative?

[bachratyg]

I'd rather not put back a partial sid check. That would mean the lib sometimes decides for me if there's a change, and sometimes not. It doesn't feel consistent. This is opinionated behavior, it should be more explicit.

I'd either remove the sid or alternatively extend SessionState to include the full user. querySessionStatus already does a reduced silent sign-in (only the openid scope is requested), might as well do the full thing. The user can also be added to the events then the subscriber can do as they see fit e.g.

userManager.events.addUserSessionChanged(async newUser => {
    if( newUser.sid !== oldUser.sid ) {
        // previous handler logic
    }
});

[pamapa]

I'd either remove the sid or alternatively extend SessionState to include the full user.

if we are going with the User, we could drop SessionState and directly use User:
-> querySessionStatus(args: QuerySessionStatusArgs = {}): Promise<User| null>
The returned object would contain the currently signed-in user, would it be useful to also provide the previous signed-in user?

The main question is how do we then handle the types for monitorAnonymousSession feature. Do you know the use-case for this feature? If it is still useful, we should add some documentation to the settings property else drop it?

Probably something we already have:

const tmpUser = {
  session_state: session.session_state,
  profile: session.sub /*&& session.sid*/ ? {
    sub: session.sub,
    //sid: session.sid,
  } : null,
};

This would also simplify the type for the parameter of SessionMonitor._start.

[bachratyg]

if we are going with the User, we could drop SessionState and directly use User: -> querySessionStatus(args: QuerySessionStatusArgs = {}): Promise<User| null>

The session_state is still needed for monitoring the anonymous session. It should be exposed separately from User.

The returned object would contain the currently signed-in user, would it be useful to also provide the previous signed-in user?

Subscriber can always call getUser but it would certainly be convenient.

The main question is how do we then handle the types for monitorAnonymousSession feature. Do you know the use-case for this feature? If it is still useful, we should add some documentation to the settings property else drop it?

It could be useful for apps that have public features e.g. some sites show a popup when you sign in on another page. The only problem I see here is that a new notification could be triggered while the previous handler is still running but that is already the case with UserSessionChanged.

[pamapa]

I'd either remove the sid or alternatively extend SessionState to include the full user

so lets remove that too...

[bachratyg]

Removed sid for now. I will need a bit more time to improve querySessionStatus with the full profile

[pamapa]

thanks for contributing