Make profile REST API provide same information then XML API #4575
Conversation
Current profile REST API misses the information: visible, enable and enableBy. These are needed by IPA to improve the UI. Additionally, a filter for visible and enable has been added. Resolve RHCS-4375
|
@ladycfu I have not changed the logic of which profiles admin and user can access but I am wondering if non admin users should access not enabled profiles. Is this correct or we have to limit them? |
| if (!visibleOnly) { | ||
| ret.setProfileVisible(Boolean.valueOf(profile.isVisible())); | ||
| ret.setProfileEnable(Boolean.valueOf(profile.isEnable())); | ||
| ret.setProfileEnableBy(profile.getApprovedBy()); | ||
| } |
There was a problem hiding this comment.
IIUC this code will hide the profileVisible, profileEnable, and profileEnableBy attributes from all users except CA admins/agents. Is there a requirement for that behavior? I don't think these attributes are sensitive, so if we can include them in the results it will be easier for the callers/clients to process the results (i.e. no need to handle cases where those attributes are missing).
There was a problem hiding this comment.
Ideally I think createProfileDataInfo() should only construct a ProfileDataInfo object from a given Profile, so it should not contain any code to filter out entries/attributes, but this is an existing issue, no need to fix it in this PR.
There was a problem hiding this comment.
One more thing, I think the Boolean.valueOf() might be redundant because of auto-boxing, but it's fine to keep it.
There was a problem hiding this comment.
Initially, I was thinking to add the new information only to the admin but I have modified according to all your comments.
base/ca/src/main/java/org/dogtagpki/server/ca/rest/ProfileService.java
Outdated
Show resolved
Hide resolved
| @ACLMapping("profiles.list") | ||
| public Response listProfiles( | ||
| @QueryParam("start") Integer start, | ||
| @QueryParam("size") Integer size); | ||
| @QueryParam("size") Integer size, | ||
| @QueryParam("visible") Boolean visible, | ||
| @QueryParam("enable") Boolean enable); |
There was a problem hiding this comment.
It would be nice if we can add options into ProfileFindCLI so we can test these params from CLI, but it's not required.
There was a problem hiding this comment.
Added. I have added 3 option but they work slightly differently.
The new options for the command ca-profile-find are:
--visibleshows only visible profiles (this is the default behaviour for non admin users);--enableshows only the enabled profiles;--enableBy <username>shows the profiles enabled by the provided user.
| Boolean visible = cmd.hasOption("visible") ? Boolean.TRUE : null; | ||
| Boolean enable = cmd.hasOption("enable") ? Boolean.TRUE : null; |
There was a problem hiding this comment.
This code doesn't allow us to find hidden / disabled profiles. How about something like this?
Boolean visible = cmd.hasOption("visible") ? Boolean.valueOf(cmd.getOptionValue("visible")) : null;
Boolean enable = cmd.hasOption("enable") ? Boolean.valueOf(cmd.getOptionValue("enable")) : null;
There was a problem hiding this comment.
In this way the option will require the explicit value true or false and for a boolean I do not like. However, this is a particular case because it allows to manage all 3 possible conditions: all, only true and only false. Therefore I have update accordingly.
There was a problem hiding this comment.
Thanks for the update! The alternative is to provide options like: --visible-only, --hidden-only, --enabled-only, --disabled-only. I'll let you decide. Regardless, there's no objection from me.
If these changes are sufficient for IPA feel free to merge. Thanks!
The command ca-profile-find has 3 additional filters: - "--visible" shows only visible profiles (this is the default behaviour for non admin users); - "--enable" shows only the enabled profiles; - "--enableBy <username>" shows the profiles enabled by the provided user.
fmarco76
force-pushed
the
RestProfileFilters
branch
from
082d81c
to
bb13d41
Compare
|
Kudos, SonarCloud Quality Gate passed! |
|
@edewata Thanks! |
Current profile REST API misses the information: visible, enable and enableBy. These are needed by IPA to improve the UI.
Additionally, a filter for visible and enable has been added.
Resolve RHCS-4375