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

Changes to <HawtioLogin> to allow for plugin to provide customisations #607

Merged
merged 5 commits into from
Oct 16, 2023

Conversation

phantomjinx
Copy link
Member

@phantomjinx phantomjinx commented Oct 12, 2023

While most changes concern updating the UI of the <HawtioLogin/> to support a customised child login form, a necessary change is also included for the JolokiaService. Specifically, the use-case concerns what happens when logging-out occurs.

When the logging-out button is clicked, the app routes back to /login but was stuck on displaying the waiting symbol.
Analysis showed that because <HawtioLogin> is now using the usePlugins hook, the component waits on the pluginsLoaded flag. This was never returning true indicating that the plugins were never completing resolution. Digging in further indicated that despite not being logged-in the JMX plugin was, as a result of its isActive() plugin function, still trying to list nodes from the jolokiaService yet this process was hanging.
This has been resolved by, since the service was already waiting for the userService.isLogin() to return, taking the result of isLogin() and if false then returning a null jolokia url. This causes a dummy tree to be created and the JMX plugin to resolve satisfactorily.

@phantomjinx phantomjinx changed the title Changes to HawtioLoginForm to allow for plugin to provide customisations Changes to <HawtioLogin> to allow for plugin to provide customisations Oct 12, 2023
@phantomjinx
Copy link
Member Author

Is a dependency of hawtio/hawtio-online#155

@github-actions
Copy link

github-actions bot commented Oct 12, 2023

Test Results

    8 files  ±0      8 suites  ±0   1h 0m 26s ⏱️ + 2m 3s
  59 tests ±0    57 ✔️  - 1  1 💤 ±0  1 +1 
468 runs  ±0  459 ✔️  - 1  8 💤 ±0  1 +1 

For more details on these failures, see this check.

Results for commit 9abd083. ± Comparison against base commit 6bcaec8.

♻️ This comment has been updated with latest results.

@hawtio-ci
Copy link

hawtio-ci bot commented Oct 12, 2023

Test results

Run attempt: 609
Detailed summary

NAME TESTS PASSED ✅ SKIPPED 💤 FAILED ❌ ERRORS 🚫 TIME 🕖
results-quarkus-node(16)-java(11)-firefox 59 58 1 0 0 436.304
results-quarkus-node(16)-java(17)-firefox 59 58 1 0 0 497.778
results-quarkus-node(18)-java(11)-firefox 59 58 1 0 0 395.997
results-quarkus-node(18)-java(17)-firefox 59 58 1 0 0 440.237
results-springboot-node(16)-java(11)-firefox 58 57 1 0 0 433.646
results-springboot-node(16)-java(17)-firefox 58 56 1 1 0 526.1
results-springboot-node(18)-java(11)-firefox 58 57 1 0 0 406.498
results-springboot-node(18)-java(17)-firefox 58 57 1 0 0 489.749

Copy link
Member

@tadayosi tadayosi left a comment

Choose a reason for hiding this comment

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

thanks! there is only one comment I'd like to be cleared.

packages/hawtio/src/plugins/shared/jolokia-service.ts Outdated Show resolved Hide resolved
@tadayosi
Copy link
Member

Please fix lint error too.

* HawtioLogin.tsx
 * Looks at HawtConfig for title
 * Checks if there is an active login plugin available and substitutes its
   component for the login form
 * Should no plugin be available then the default HawtioLoginForm is loaded
 * Awaits on all plugins being loaded since otherwise the plugins array
   from usePlugins() will be empty

* HawtioLoginForm.tsx
 * Default delegate form that a user populate with user/password

* core.ts
 * Adds isLogin property to the Plugin interface to flag the plugin as a
   login plugin

* HawtioPage.tsx
 * Ensures that the login plugins are filtered out of the page plugins
* During jmx plugin resolution, its isActive() function tries to load the
  tree but listing nodes from the jolokiaService. Seems that even though
  it waits on the userService.isLogin(), it then gets stuck after if not
  logged-in. This now results in the plugin never reporting itself as
  loaded and 'plugins' in usePlugins() hook remains empty.

* By not just awaiting but observing the isLogin() return value, this can
  be avoided by returning a null url and so causing the jolokiaService to
  create a DummyJolokia tree instead. This makes sense since no list of
  nodes can possibly be made if not logged-in.
* [rbac|jolokia]-service / workspace
 * Services constructed on load cannot have jolokia-related functions
   executed in their constructors. Doing so prior to userService.fetchUser()
   being called will now result in an error being thrown that the user is
   not logged in.

 * Each public function from the services is therefore provided with an
   init() function which does the job of the constructor, ensuring the
   jolokia-related members are initialised correctly, once it has been
   established that the user is logged-in.

* core.ts
 * Plugins should only be activated if the user has been logged-in with
   the exception of those plugins, specifically advertising themselves as
   'isLogin' type plugins, ie. plugins providing a custom login form.
 * Only if the user is logged-in will other plugins determine if they can
   be activated by calling their isActive() functions.
* Locally, executing the unit tests causes numerous errors with
  auth/globals.ts/Logger being 'undefined'.
 * Due to inclusion of userService in core.ts the auth and core packages
   are circular associated via the userService and Logger
…wtio#594

* Since tests make use of services, such as workspace, it is now essential
  that the userService is initialised by them to ensure the environment
  is correctly setup for the test to execute.
@tadayosi
Copy link
Member

There are a few issues that need to be addressed in the latest fix. But instead of running another cycle of commit & review, I can take care of them after merging it. Feel free to turn it to "ready for merge" and merge it.

@phantomjinx phantomjinx marked this pull request as ready for review October 16, 2023 10:38
@phantomjinx phantomjinx merged commit a9cef9a into hawtio:main Oct 16, 2023
11 of 13 checks passed
@phantomjinx phantomjinx deleted the auth-form branch October 18, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants