Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

[BUG] InResponseTo not valid for all requests #682

Closed
innovatechs opened this issue Mar 30, 2022 · 2 comments
Closed

[BUG] InResponseTo not valid for all requests #682

innovatechs opened this issue Mar 30, 2022 · 2 comments
Labels

Comments

@innovatechs
Copy link

innovatechs commented Mar 30, 2022

Hi everyone

We are getting the InResponseTo not valid error for each request since the CacheProvider is creating a new instance for each call (our assumption): getting empty keys when we call the get method.

Expected behavior

The previous versions were using one instance of the CacheProvider (from constructor) and keys are managed correctly (save, get and delete).

Environment

Node.js version: 14
passport-saml version: 3.2.1

@srd90
Copy link

srd90 commented Mar 30, 2022

You didn't care to define what was your previous version.

Lets assume your previous version was https://github.com/sign-in-canada/passport-saml/tree/275f15488823407043208677caf3dea8fee9903a (current HEAD of sign-in-canada/passport-saml repo's master branch which seems to be based on node-saml/passport-saml 2.x).

And lets assume that you are merging passport-saml 3.2.1 (based on content of https://github.com/sign-in-canada/passport-saml/tree/b25305b50e0f69690d788ef48a1145d5bdab1fba which is current HEAD of sign-in-canada/passport-saml repo's 3.x branch).

Since you have modified (based on this commit sign-in-canada@e4fe1fc ... which btw. doesn't make any sense) also multisamlstrategy (which is pretty much the only place which recreates SAML object each and every time) you might have encounter this issue in the context of using multisamlstrategy.

node-saml/passport-saml 2.x's multisamlstrategy created implicitly shared InMemoryCacheProvider if cacheProvider option was not defined:

if(!options.cacheProvider){
options.cacheProvider = new InMemoryCacheProvider(
{keyExpirationPeriodMs: options.requestIdExpirationPeriodMs });
}

node-saml/passport-saml 3.x doesn't do that anymore (related comments: #548 (review) and #334 (comment)).

Wild guess:

  1. You have typo in your cacheProvider configuration option name and
  2. when multisamlstrategy creates SAML object its constructor creates new InMemoryCacheProvider due missing cacheProvider option during e.g. each authenticate invocation.

I.e. code flow would/could be like this (links point to current HEAD of node-saml/passport-saml 3.x):

authenticate(req: RequestWithUser, options: AuthenticateOptions): void {
this._options.getSamlOptions(req, (err, samlOptions) => {

const samlService = new SAML({ ...this._options, ...samlOptions });

new InMemoryCacheProvider({

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 31, 2022

I'm in agreement with @srd90 here. I don't think we have enough information to say what exactly the problem is. Moving to discussion.

@node-saml node-saml locked and limited conversation to collaborators Mar 31, 2022
@cjbarth cjbarth converted this issue into discussion #683 Mar 31, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Projects
None yet
Development

No branches or pull requests

3 participants