Fixes #37665 - Context-based frontend permission management #10338

Thorben-D · 2024-10-01T05:17:38Z

Redmine issue
Community-forum post this implementation is based on

This PR implements a faster way to check user permissions in Foreman core/plugin frontend code. This speed advantage is gained by leveraging the ForemanContext to store the current user's permissions.

Changes made:

Extended: ForemanContext
Added permissions field to context. Contains the current user's permissions - Implmenented as a set of permission names.
Added: Permitted
A component that abstracts the conditional rendering scheme of "Render if <permission> is granted"
- Developers may require a single or multiple permissions
- Supports rendering a different component if the permissions are not granted
Added: usePermission/s
Two custom hooks that allow checking whether a is granted a single or multiple permissions
Added: useForemanPermissions
Context hook that provides a reference to the actual permissions set
Added: useRefreshedContext
A custom hook that refreshes the ForemanContext by requesting the up-to-date application state from the backend
- automatically sets the ForemanContext when called
- Supports partial context updates
Added: ContextController at api/v2/context
Controller that interfaces with the application context on the backend
- Only supports reading of the context at the moment
- Requires view_context permission
Added: Foreman-core permissions as JS constants
Added: Rake task to generate JS permission constants
Added: DeveloperDocs page on permission handling

Performance

Five samples taken on a decently fast system using FireFox developer edition with cache disabled and no background tasks running.
Different approaches tested using this component.

API-based approach

Sample	Total page load time (s)	API request duration (ms)
1	2.14	197
2	2.07	156
3	2.05	176
4	2.09	197
5	2.16	184
Mean	2.10	182

`Permitted` component

Sample	Total page load time (s)
1	1.98
2	1.96
3	1.91
4	1.99
5	1.99
Mean	1.97

`Permitted` component in conjunction with `useRefreshedContext()`

Sample	Total page load time (s)	API request duration (ms)
1	1.91	158
2	2.00	146
3	1.92	167
4	1.92	149
5	2.00	154
Mean	1.95	155

Discussion

Although it may seem like the difference between Permitted and Permitted + useRefreshedContext is negligible, this is not the case, as the actual time to mount the component differs by at least the API request duration when using useRefreshedContext. Unfortunately, I don't have any values there because the React profiler is ...not great.
Even with a full page-reload, a speed difference of ~7% may be observed. This will be amplified on weaker / busier systems than mine. Furthermore, navigation between frontend-rendered pages will benefit a lot more, as comparatively little data needs to be requested from the backend.
Unfortunately, I nuked my dev-environment shortly after collecting the data above, so I will have to amend the results for client-rendered -> client-rendered later :)

The case for permission constants in JS

Although not strictly in the scope of this issue, I believe that this presents a good opportunity to introduce permission constants for JS, similar to API status constants. These constants mainly offer the following benefits:

Should it ever become necessary to rename a permission, having them constantized makes this very easy
Modern IDEs can use these constants to perform analyses such as usage finding.

To aid developers in creating these constants, I created a rake task, export_permissions, that automatically generates these JS constants from the permission seed file.

TODO

ContextController test:
I tried a lot of things, but I didn't manage to mock the app_metadata function, so the test is useless at the moment
useRefreshedContext test:
Since this hook interfaces pretty deeply with the context and the API, testing it is rather difficult. I got pretty far with renderHook and mocks but am running into issues with the setForemanContext function.

Introduce a faster alternative to API based permission management in the frontend based on ForemanContext - Add Permitted component - Add permission hooks - Add ContextController - Add JS permission constants - Add rake task to export permissions - Add permission management page to developer docs

MariaAga · 2024-10-11T15:13:49Z

webpack/assets/javascripts/react_app/Root/Context/Hooks/useRefreshedContext.js

+  };
+
+  useEffect(() => {
+    getContext();


You can set the getContext inside the useEffect to avoid the error.
But I think instead of getContext and useEffect you can do something like:

const { response: responseData, status } = useAPI('get', '/api/v2/context', { only, }); const isLoading = status === STATUS.PENDING; const isError = status === STATUS.ERROR; const error = isError ? responseData : null; const setForemanContext = useForemanSetContext(responseData); setForemanContext(...

(also will review the rest as well)

MariaAga · 2024-10-11T16:32:01Z

Navigating between frontend-rendered pages does not refresh the context. Currently (2024-09-24), this does not pose a problem for permission management, as every page that may grant permissions to users is rendered serverside. To address the issue of stale context, developers may use the useRefreshedContext hook.

Does this mean that useRefreshedContext is for when a user is on reactPage1, their permissions change from a different page, and then they navigate to reactPage2? so the context permission list is incorrect since its not been refreshed?

ekohl · 2024-10-11T16:39:05Z

To aid developers in creating these constants, I created a rake task, export_permissions, that automatically generates these JS constants from the permission seed file.

My programming professor from university approves the use of constants. I just wonder about the plugin use case. Can we make it easy for plugins to do the same?

ekohl · 2024-10-11T16:40:12Z

test/controllers/api/v2/context_controller_test.rb

+  # }
+
+  test "should get full metadata" do
+    assert_equal true, true


I assume these are missing a TODO comment :)

Yes, those tests are still missing because I didn't manage to stub/mock the helper method...
Do you have an idea how I might do that?

ekohl · 2024-10-11T16:41:00Z

lib/tasks/export_permissions.rake

+desc 'Export Foreman permissions to JavaScript'
+task export_permissions: :environment do
+  formatted = PermissionsList.permissions.map { |permission| "export const #{permission[1].upcase} = '#{permission[1]}';\n" }
+  File.open('webpack/assets/javascripts/react_app/permissions.js', 'w') do |f|


I'd like to use Rails.root as a base for a complete path in case something reuses this

ekohl · 2024-10-11T16:41:57Z

app/controllers/api/v2/context_controller.rb

+            sliced = metadata.slice(*only.map { |x| x.to_sym })
+            render json: { metadata: sliced }
+          end
+        else render json: { metadata: metadata }


Suggested change

else render json: { metadata: metadata }

else

render json: { metadata: metadata }

Thorben-D · 2024-10-14T10:55:43Z

Does this mean that useRefreshedContext is for when a user is on reactPage1, their permissions change from a different page, and then they navigate to reactPage2? so the context permission list is incorrect since its not been refreshed?

Yes, exactly! As I wrote in the docs, this is not really needed (yet) in regards to permissions, but it may be of use for UISettings.
On a different note: Do you know what causes the tests to fail? Obviously I ran the tests manually before pushing, but I used my IDE to do so, which calls node directly. I didn't think it would make a difference but when using tfm-test, I get the same errors...

MariaAga · 2024-10-14T11:34:32Z

Tests (at least some) are failing since we mock the context in webpack/test_setup.js, so you can move the mock from jest.spyOn(foremanContextHooks, 'useForemanPermissions').mockImplementation(() => allPermissions) to that file.
Also an easy way to test only your tests is to run npm test -- webpack/assets/javascripts/react_app/common/hooks/Permissions/permissionHooks.test.js
its a bit weird, but the code will be: (because of jest scopes)

import { useForemanPermissions } from './assets/javascripts/react_app/Root/Context/ForemanContext';
import { allPermissions } from './assets/javascripts/react_app/common/hooks/Permissions/permissionHooks.fixtures';
...
  getHostsPageUrl: displayNewHostsPage =>
    displayNewHostsPage ? '/new/hosts' : '/hosts',
  useForemanPermissions: jest.fn(),
...
useForemanPermissions.mockReturnValue(allPermissions);

MariaAga · 2024-10-14T14:44:08Z

lib/tasks/export_permissions.rake

+task export_permissions: :environment do
+  formatted = PermissionsList.permissions.map { |permission| "export const #{permission[1].upcase} = '#{permission[1]}';\n" }
+  File.open('webpack/assets/javascripts/react_app/permissions.js', 'w') do |f|
+    f.puts '/* This file is automatically generated. Run "bundle exec rake export_permissions" to regenerate it. */'


Can it also put /* eslint-disable */ at the top of the file?

MariaAga · 2024-10-14T14:45:19Z

webpack/assets/javascripts/react_app/components/Permitted/Permitted.js

+ * Supply **requiredPermission** XOR **requiredPermissions**
+ * @param {string} requiredPermission: A single string representing a required permission
+ * @param {array<string>} requiredPermissions: An array of permission string.


For a single permission, wouldnt it be easier to provide just one permission in an array?

Yes, at second thought, having both options seems redundant and confusing. I will remove the single permission options.
Same goes for the hooks.

MariaAga · 2024-10-15T11:34:01Z

webpack/assets/javascripts/react_app/components/Permitted/Permitted.js

+/* eslint-enable react/require-default-props */
+Permitted.defaultProps = {
+  children: null,
+  unpermittedComponent: null,


Instead of null, could it be webpack/assets/javascripts/react_app/components/PermissionDenied/index.js?

That's a great idea! I will implement your suggestions tomorrow.

MariaAga · 2024-10-15T12:05:26Z

Also, I tried the wrapper and it works nicely!
in webpack/assets/javascripts/react_app/components/HostDetails/Tabs/ReportsTab/index.js

const ReportsTabWrapper = props => (
  <Permitted
    requiredPermissions={['view_config_reports']}
    unpermittedComponent={
      <PermissionDenied missingPermissions={['TESTview_config_reports']} />
    }
  >
    <ReportsTab {...props} />
  </Permitted>
);

export default ReportsTabWrapper;

pr-processor bot added the Not yet reviewed label Oct 1, 2024

github-actions bot added UI Docs labels Oct 1, 2024

MariaAga self-assigned this Oct 7, 2024

MariaAga reviewed Oct 11, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Oct 11, 2024

ekohl reviewed Oct 11, 2024

View reviewed changes

MariaAga reviewed Oct 14, 2024

View reviewed changes

MariaAga reviewed Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37665 - Context-based frontend permission management #10338

Fixes #37665 - Context-based frontend permission management #10338

Thorben-D commented Oct 1, 2024

MariaAga Oct 11, 2024 •

edited

Loading

MariaAga commented Oct 11, 2024

ekohl commented Oct 11, 2024

ekohl Oct 11, 2024

Thorben-D Oct 14, 2024

ekohl Oct 11, 2024

ekohl Oct 11, 2024

Thorben-D commented Oct 14, 2024

MariaAga commented Oct 14, 2024

MariaAga Oct 14, 2024

MariaAga Oct 14, 2024

Thorben-D Nov 11, 2024 •

edited

Loading

MariaAga Oct 15, 2024

Thorben-D Oct 16, 2024

MariaAga commented Oct 15, 2024

	else render json: { metadata: metadata }
	else
	render json: { metadata: metadata }

Fixes #37665 - Context-based frontend permission management #10338

Are you sure you want to change the base?

Fixes #37665 - Context-based frontend permission management #10338

Conversation

Thorben-D commented Oct 1, 2024

Changes made:

Performance

API-based approach

Permitted component

Permitted component in conjunction with useRefreshedContext()

Discussion

The case for permission constants in JS

TODO

MariaAga Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

MariaAga commented Oct 11, 2024

ekohl commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thorben-D commented Oct 14, 2024

MariaAga commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thorben-D Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariaAga commented Oct 15, 2024

`Permitted` component

`Permitted` component in conjunction with `useRefreshedContext()`

MariaAga Oct 11, 2024 •

edited

Loading

Thorben-D Nov 11, 2024 •

edited

Loading