-
Notifications
You must be signed in to change notification settings - Fork 991
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
Fixes #37665 - Context-based frontend permission management #10338
base: develop
Are you sure you want to change the base?
Fixes #37665 - Context-based frontend permission management #10338
Conversation
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
}; | ||
|
||
useEffect(() => { | ||
getContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Does this mean that |
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? |
# } | ||
|
||
test "should get full metadata" do | ||
assert_equal true, true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume these are missing a TODO comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to use Rails.root
as a base for a complete path in case something reuses this
sliced = metadata.slice(*only.map { |x| x.to_sym }) | ||
render json: { metadata: sliced } | ||
end | ||
else render json: { metadata: metadata } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else render json: { metadata: metadata } | |
else | |
render json: { metadata: metadata } |
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 |
Tests (at least some) are failing since we mock the context in webpack/test_setup.js, so you can move the mock from 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); |
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. */' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it also put /* eslint-disable */
at the top of the file?
* Supply **requiredPermission** XOR **requiredPermissions** | ||
* @param {string} requiredPermission: A single string representing a required permission | ||
* @param {array<string>} requiredPermissions: An array of permission string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a single permission, wouldnt it be easier to provide just one permission in an array?
/* eslint-enable react/require-default-props */ | ||
Permitted.defaultProps = { | ||
children: null, | ||
unpermittedComponent: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of null, could it be webpack/assets/javascripts/react_app/components/PermissionDenied/index.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great idea! I will implement your suggestions tomorrow.
Also, I tried the wrapper and it works nicely! const ReportsTabWrapper = props => (
<Permitted
requiredPermissions={['view_config_reports']}
unpermittedComponent={
<PermissionDenied missingPermissions={['TESTview_config_reports']} />
}
>
<ReportsTab {...props} />
</Permitted>
);
export default ReportsTabWrapper; |
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:
ForemanContext
Added permissions field to context. Contains the current user's permissions - Implmenented as a set of permission names.
Permitted
A component that abstracts the conditional rendering scheme of "Render if <permission> is granted"
usePermission/s
Two custom hooks that allow checking whether a is granted a single or multiple permissions
useForemanPermissions
Context hook that provides a reference to the actual permissions set
useRefreshedContext
A custom hook that refreshes the ForemanContext by requesting the up-to-date application state from the backend
ContextController
atapi/v2/context
Controller that interfaces with the application context on the backend
view_context
permissionPerformance
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
Permitted
componentPermitted
component in conjunction withuseRefreshedContext()
Discussion
Although it may seem like the difference between
Permitted
andPermitted
+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 usinguseRefreshedContext
. 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:
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
I tried a lot of things, but I didn't manage to mock the
app_metadata
function, so the test is useless at the momentSince 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.