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 authorizationId parameter for sharing an existing authorization #6

Conversation

cstanger
Copy link
Contributor

Current Solution
The authorizationKey is generated randomly when loading the module.

Current Problem
Within an application, which scales horizontally, each application instance will generate its own unique authorizationKey. So even though a shared session storage is used, authorization state can not be shared between application instances.

Solution
In order to scale an application instance horizontaly, while using a shared session store, the authorizationKey should be controlled by using a secret 'sessionAuthorizationKeySecret', so that an authorization state is available though out parallel running instances. If 'sessionAuthorizationKeySecret' is not set, a random authorizationKey is generated per application instance.

The secret 'sessionAuthorizationKeySecret' should be added optionally within the settings object.

Copy link
Contributor

@lapinek lapinek left a comment

Choose a reason for hiding this comment

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

Thank you, @cstanger! Just minor comments, let me know what you think.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@lapinek lapinek left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks again for adding this functionality, which seems to be a useful addition to the lib. I'll approve, but see just one minor comment.

Also, please remember to squash all commits into one before I merge, so that there is only one commit for this feature.

As for commit message, please remember to change the key name in it (currently it is still sessionAuthorizationKeySecret). Also, please prefix the message with feat: , according to Semantic Commit Messages, which I follow.

So the current message would become: "feat: add authorizationId to control authorizationKey". Or maybe: "feat: add authorizationId parameter for sharing an existing authorization". Up to you :0).

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@cstanger cstanger changed the title add sessionAuthorizationKeySecret to control authorizationKey add authorizationId parameter for sharing an existing authorization Dec 14, 2020
@cstanger
Copy link
Contributor Author

I'm not sure how to squash all commits into one before you merge...

I can offer to create a complete new Pull Request, or I think you can use the quash and merge' option on GitHub with the merge button.

@lapinek
Copy link
Contributor

lapinek commented Dec 14, 2020

Hi, @cstanger. I am not sure what GitHub Squash and merge will do with your (author) details, due to an open issue: isaacs/github#1368

You could use interactive rebase, to control which commit to squash the others to.

Alternatively, I think you can simply soft reset to the commit you started with and then commit with the new message:

$ git reset --soft 9397b2af463ce1a95b67d12ad9a0d6f01db6f98d
git commit -m "feat: add authorizationId parameter for sharing an existing authorization"

Let me know how it works for you.

@lapinek
Copy link
Contributor

lapinek commented Dec 14, 2020

Oh, and then, because the history was rewritten, you force push the new (single) commit to your branch with the -f option.

@cstanger cstanger force-pushed the feature/NewAuthorizationSessionIdentifierSecret branch from a9889b0 to 42c3951 Compare December 14, 2020 21:00
@cstanger
Copy link
Contributor Author

cstanger commented Dec 14, 2020

Hey @lapinek, with your advice, I was able to squash all commits into one. Thank you :)

So I think, this Feature is ready to merge, if you agree.

@lapinek lapinek merged commit 94d507e into ForgeRock:master Dec 14, 2020
@lapinek
Copy link
Contributor

lapinek commented Dec 14, 2020

Thank you for your contribution, @cstanger!

@lapinek
Copy link
Contributor

lapinek commented Dec 14, 2020

@cstanger, I just realized that not all the comments made it to the jsdoc. The most reliable way to update the docs is to generate them automatically, with jsdoc2md, which is a dev. dependency in this project. This is done automatically with the prepare script when you run npm install, for example.

This is only a side comment, for future interactions; I'll update the docs in a release commit, if that's OK with you.

@lapinek
Copy link
Contributor

lapinek commented Dec 15, 2020

@cstanger, I published a new version yesterday that incorporates your change.

If you don't mind, could you describe briefly your use case? Why do you need horizontal scaling, is it a production system? How do you share session between the instances, do you use persistent storage, JWT?

@cstanger
Copy link
Contributor Author

@lapinek, awesome! thank you for publishing the new version. happy to contribute.

To elaborate more about the use case:
If you use the openid-client-helper module within an AWS Lambda (Authorizer) function, which uses an external persistent session storage (i.e. DynamoDB), each Lambda invocation will need to use the same, deterministic authorizationKey in the shared session object.
This feature makes it possible to share an authorizationKey. So that lambda functions can be used to get the claims (getClaims) for a user based on a session without new auth flow.

Thank you for your help to this PR.
rReally appreciated your comments and feedback.

@lapinek
Copy link
Contributor

lapinek commented Dec 15, 2020

NP.

@lapinek
Copy link
Contributor

lapinek commented Dec 15, 2020

Forgot to say, thanks for the explanation, good to know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants