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

Restore a subset of the args removed from the sign in methods #95

Closed
kherock opened this issue Sep 26, 2021 · 6 comments · Fixed by #96
Closed

Restore a subset of the args removed from the sign in methods #95

kherock opened this issue Sep 26, 2021 · 6 comments · Fixed by #96

Comments

@kherock
Copy link
Collaborator

kherock commented Sep 26, 2021

With oidc-client 1.x, I used a few parameters in the signinRedirect, signinPopup, and signinSilent methods which were removed in #20:

  • extraQueryParams - I need to set this dynamically to tweak the auth server's login page. For example, Keycloak has kc_idp_hint that allows me to create multiple buttons for launching different identity provider login flows. I also might need to be able to suggest a username that's only obtained after the UserManager is created
  • popupWindowFeatures
    • I want to be able to center the popup window in front of the open window rather than the hardcoded offset that's set when constructing the UserManager. These offsets can only be accurately determined the moment the popup is opened.
    • I need to be able to customize the size of the window based on various login pages that can show based on the parameter I mentioned above
@pamapa
Copy link
Member

pamapa commented Sep 26, 2021

My idea was to move all that stuff which is coming into the settings. If that is not possible we can add stuff to the arguments again, but only typed :-) I am happy to accept changes here.

@pamapa
Copy link
Member

pamapa commented Sep 27, 2021

extraQueryParams, we can re-add args to signinRedirect, signinPopup, and signinSilent:
with something like this as the type interface SigninArgs { extraQueryParams?: Record<string, any>; }. And pass that a long the stack back into OidcClient.createSigninRequest.

popupWindowFeatures: no idea what the best solution is, need to first see the code. This sounds something, that probably everybody wants? In that case it should be default. if not we need a setting for it?

@kherock
Copy link
Collaborator Author

kherock commented Sep 27, 2021

I just opened a PR adding the params that customize the PopupWindow, IFrameWindow, and RedirectNavigator behavior. I haven't decided the best function signature for adding back extraQueryParams yet, trying to avoid overcomplicating the types of the arguments there, I might save it for a separate PR.

I'm also thinking it might be handy to expose popupWindowFeatures as a typed object that we serialize as a string. It should be pretty easy from there to give it default values based on window dimensions.

@pamapa
Copy link
Member

pamapa commented Sep 27, 2021

wow i just started to hack on adding back the args and luckily i saw your effort! Which looks great. So i stopped my attempt.

I am planning to make the two projects less depended on myself. Therefore i will move them into an free/opensource github "organization", such that they are together and might add additional non security TypeScript libraries later. Also i would like to empower you to review+merge stuff from other developers. The main idea is that nobody merges stuff from himself. The sole exception is releasing i guess. What do you think?

@kherock
Copy link
Collaborator Author

kherock commented Sep 27, 2021

I think a new org makes sense, though I don't see any rush. It would also be nice if we could eventually reuse the original oidc-client package name on npm 🙂, but that's up to @brockallen's discretion. I always find it a bit odd when NPM packages include js or ts in their names, though I don't think there are any good alternatives available at the moment.

Anyway, I'd be happy to more formally help with code review in the future. I already have a few projects that depend on this library that I'll be continuing to support for the foreseeable future.

@brockallen
Copy link
Contributor

It would also be nice if we could eventually reuse the original oidc-client package name on npm 🙂, but that's up to @brockallen's discretion.

Generally due to supply chain trust issues, this is discouraged.

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

Successfully merging a pull request may close this issue.

3 participants