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

feat: Core authentication plugins #1180

Merged
merged 27 commits into from
Apr 20, 2023
Merged

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Mar 28, 2023

  • Add authentication plugins framework
  • Add core plugins (Pre-shared key, Parent, and anonymous authentication)
  • Fixes Support plugins for authentication #1058
  • Refactored app initialization so embed-grid and embed-chart share the same logic
    • Allows embed-grid/embed-chart to display a login screen if necessary
    • embed-grid/embed-chart now also authorize

@mofojed mofojed changed the title Core authentication plugins feat: Core authentication plugins Mar 28, 2023
@mofojed mofojed requested a review from niloc132 March 28, 2023 19:50
@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #1180 (6882a2f) into main (2f9c020) will increase coverage by 0.52%.
The diff coverage is 53.11%.

@@            Coverage Diff             @@
##             main    #1180      +/-   ##
==========================================
+ Coverage   45.14%   45.67%   +0.52%     
==========================================
  Files         460      473      +13     
  Lines       33637    33703      +66     
  Branches     8462     8455       -7     
==========================================
+ Hits        15186    15394     +208     
+ Misses      18401    18258     -143     
- Partials       50       51       +1     
Flag Coverage Δ
unit 45.67% <53.11%> (+0.52%) ⬆️

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

Impacted Files Coverage Δ
packages/app-utils/src/components/useConnection.ts 0.00% <0.00%> (ø)
packages/app-utils/src/components/usePlugins.ts 0.00% <0.00%> (ø)
packages/app-utils/src/plugins/loadRemoteModule.ts 100.00% <ø> (ø)
...s/app-utils/src/plugins/remote-component.config.ts 100.00% <ø> (ø)
packages/auth-plugins/src/AuthPlugin.ts 0.00% <0.00%> (ø)
packages/code-studio/src/AppRoot.tsx 0.00% <ø> (ø)
packages/code-studio/src/index.tsx 0.00% <0.00%> (ø)
packages/code-studio/src/main/AppInit.tsx 0.00% <0.00%> (ø)
packages/code-studio/src/main/AppMainContainer.tsx 34.84% <0.00%> (-0.13%) ⬇️
...ages/code-studio/src/styleguide/StyleGuideRoot.tsx 0.00% <ø> (ø)
... and 30 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

packages/code-studio/src/main/AppInit.tsx Outdated Show resolved Hide resolved
packages/code-studio/src/main/AppInit.tsx Outdated Show resolved Hide resolved
@mofojed mofojed requested review from niloc132 and vbabich March 29, 2023 16:03
@mofojed mofojed self-assigned this Mar 29, 2023
@mofojed mofojed marked this pull request as ready for review March 29, 2023 16:03
mofojed added 6 commits March 29, 2023 14:53
- Load AuthPlugins from the server before logging in
- Find the appropriate auth plugin and login with it
- Add core plugins for Pre-shared Key, Parent, and Anonymous types of authentication
- Will create an example for authclock login in the deephaven-js-plugins repo
- Include an example in the auth-plugin/README
- Add tests for the core plugin types
- Cleanup some of the AppInit stuff
niloc132
niloc132 previously approved these changes Mar 31, 2023
Copy link
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

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

Manual testing of PSK and anonymous login work great, also tested by creating my own auth wiring for keycloak and loading it through the plugins mechanism.

mofojed added 2 commits April 11, 2023 12:04
- Resolved conflicts in AppInit.tsx
- Moved requestParentLoginOptions call to AuthPluginParent
mofojed added 10 commits April 17, 2023 15:26
- Will be used by embed-grid and embed-chart as well
- Added types to @deephaven/plugin-utils
- Reference those types in @deephaven/redux
- Move SessionUtils into jsapi-utils package
- Move PluginUtils and some of the app loading stuff into app-utils package
- Will be needed for all the apps
- Now it should be easier for other apps to initialize (like the embedded apps)
- All unit tests pass
- Need to fix embed-grid and embed-chart apps
mofojed added 2 commits April 19, 2023 14:21
- Update readme
- Delete AppRootBootstrap (unused)
- Wasn't proxying the plugins URL
- Rename 'utils.ts' to 'ConnectUtils.ts'
- Wrap styleguide in a FontBootstrap
@mofojed mofojed requested a review from mattrunyon April 19, 2023 19:16
packages/app-utils/package.json Show resolved Hide resolved
packages/app-utils/src/components/usePlugins.ts Outdated Show resolved Hide resolved
packages/auth-core-plugins/src/AuthPluginPsk.tsx Outdated Show resolved Hide resolved
packages/auth-core-plugins/src/AuthPluginParent.tsx Outdated Show resolved Hide resolved
packages/auth-core-plugins/src/AuthPluginParent.test.tsx Outdated Show resolved Hide resolved
packages/code-studio/src/main/AppInit.tsx Show resolved Hide resolved
packages/jsapi-utils/src/SessionUtils.ts Outdated Show resolved Hide resolved
packages/auth-plugins/src/AuthPlugin.ts Outdated Show resolved Hide resolved
packages/auth-plugins/src/AuthPlugin.ts Outdated Show resolved Hide resolved
- Tests the full AppBootstrapping procedure
@mofojed mofojed requested a review from mattrunyon April 20, 2023 19:55
Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

Looking pretty good. There's a few unresolved conversations I commented on in this last round, so address those (just adding a comment or creating follow up tickets) and then this should be good to merge

@mofojed mofojed requested a review from mattrunyon April 20, 2023 20:54
Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

Tested w/ psk and anonymous locally

We should maybe do a follow up to improve psk by adding a prompt if it's not in the URL. Just a little better user experience

@mofojed mofojed merged commit 1624309 into deephaven:main Apr 20, 2023
@mofojed mofojed deleted the auth-plugins branch April 20, 2023 21:19
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2023
@mofojed
Copy link
Member Author

mofojed commented Apr 20, 2023

Yea I've had some discussions with @dsmmcken about that. I don't think we settled on exactly how we want to prompt for it. #1240

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

Successfully merging this pull request may close these issues.

Support plugins for authentication
3 participants