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

Add support for role/user based access #606

Merged
merged 61 commits into from
Nov 21, 2023
Merged

Add support for role/user based access #606

merged 61 commits into from
Nov 21, 2023

Conversation

iamdharmesh
Copy link
Member

@iamdharmesh iamdharmesh commented Oct 24, 2023

Description of the Change

This PR adds support for role- and user-based access control to the ClassifiAI feature. It also includes functionality for user-based opt-out, allowing users to choose not to use the specific feature if they prefer not to or if they dislike it.

Feature settings:
image

User Profile:
image

Edit User:
image

In-Context disable feature link to user profile

Title generation Resize content
Screenshot 2023-10-30 at 5 17 15 PM Screenshot 2023-10-30 at 5 18 06 PM

Note: E2E tests are not added for this new feature.

Closes #588

How to test the Change

  1. Enable role-based access to the feature.
  2. Select roles in the allowed roles to provide access to that feature.
  3. Verify that only the selected roles have access to that feature.
  4. Enable user-based access to the feature.
  5. Select the user in the allowed users to provide access to that feature.
  6. Verify that the selected user has access to that feature.
  7. Enable user-based opt-out for the feature.
  8. Go to the user profile and verify that you see the checkbox for that feature to opt out of using it.
  9. Check the opt-out checkbox and save.
  10. Verify that access to that feature has been disabled.
  11. Uncheck the opt-out checkbox to opt back in to the feature.
  12. Verify that the user now has access to that feature.
  13. Follow these steps for each ClassifAI feature.

Changelog Entry

Added - Role and user-based access control for features

Credits

Props @jeffpaul @dkotter @iamdharmesh @Sidsector9

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@iamdharmesh iamdharmesh self-assigned this Oct 24, 2023
@iamdharmesh iamdharmesh added this to the 2.5.0 milestone Oct 24, 2023
@dkotter dkotter modified the milestones: 2.5.0, 2.4.0 Oct 25, 2023
@iamdharmesh iamdharmesh changed the title [WIP] Add support for role/user based access Add support for role/user based access Oct 27, 2023
@dkotter
Copy link
Collaborator

dkotter commented Oct 27, 2023

Overall this looks really good, nice work here @iamdharmesh!

I've left a handful of minor code review comments. In addition to that, I have some thoughts on the UI and in particular some text we're showing.

In the settings for each feature, we have the following new text strings:

  • Enables ability to select which role can access FEATURE
  • Choose which roles are allowed to FEATURE
  • Enables ability to select which users can access FEATURE
  • Enables ability for users to opt-out from their user profile page

And then on the user profile page, we show:

  • Opt out of using the FEATURE feature

For some features and settings, this text makes sense (like Choose which roles are allowed to Classify Content). But in most, the feature name doesn't pair well with this text (like Enables ability to select which users can access Generate images).

Open to ideas on any of these strings (especially from @jeffpaul) but I'd recommend the following:

  • Since each of these strings is already within the settings section for a particular feature, I don't think we need to use the feature name itself. I'd suggest we standardize on:
    • Enables ability to select which roles can access this feature
    • Choose which roles are allowed to access this feature
    • Enables ability to select which users can access this feature

If we want to keep the feature name in there, I think we should change it to something like:

  • Enables ability to select which role can access the FEATURE feature
  • Choose which roles are allowed to access the FEATURE feature
  • Enables ability to select which users can access the FEATURE feature

Again, I'd recommend the former over the latter but open to thoughts.

I think what we show on the profile page is fine, since it already has the the FEATURE feature part which makes more sense here in the context of a profile and not a settings page for the feature.

I would recommend though that we standardize on capitalization. Right now most features have the first word capitalized (like Generate captions) but some have both (like Classify Content). I suggest when using the feature name as a description, we should lower case all words. In addition, some of these descriptions have a trailing period and some don't. I think standardizing on no trailing period makes sense.

Another thing I'm seeing is that some of these settings descriptions don't have proper spacing (they're too close to the checkbox) while others are fine. See Watson NLU as an example:

Screenshot 2023-10-27 at 12 25 50 PM

Looks like there's some CSS scoped to .classifai-content .classifai-wrap .classifai-input-description that doesn't get applied everywhere. We should standardize on that to ensure we have proper spacing.

One area we already talked about but just mentioning again is this does make some of these settings pages pretty long (like Azure Image Processing). I think this is fine for now and will be helped once we complete the refactor to have stand-alone features in our settings (and even further once we do a refactor on these settings screens themselves) but just mentioning that there may be complaints on how unwieldy these pages are getting.

And I think final thing is looks like we are standardizing on Image captions being the feature name for the alt text generation. While I know we added that as a feature (where the alt text can be saved as a caption, description or alt text) I'm not sure this makes sense as the main feature name (also something for @jeffpaul to chime in on).

I think something like Image descriptive text instead of Image captions may make more sense, as well as Generate descriptive text instead of Generate captions

includes/Classifai/Providers/Watson/NLU.php Outdated Show resolved Hide resolved
includes/Classifai/Providers/Watson/NLU.php Outdated Show resolved Hide resolved
includes/Classifai/Providers/Provider.php Outdated Show resolved Hide resolved
includes/Classifai/Helpers.php Show resolved Hide resolved
includes/Classifai/Admin/UserProfile.php Outdated Show resolved Hide resolved
includes/Classifai/Providers/AccessControl.php Outdated Show resolved Hide resolved
includes/Classifai/Providers/AccessControl.php Outdated Show resolved Hide resolved
includes/Classifai/Helpers.php Outdated Show resolved Hide resolved
includes/Classifai/Providers/Provider.php Outdated Show resolved Hide resolved
includes/Classifai/Providers/Provider.php Outdated Show resolved Hide resolved
src/js/admin.js Outdated Show resolved Hide resolved
@jeffpaul
Copy link
Member

@dkotter @iamdharmesh I pretty much agree with Darin's recommendations in his #606 (comment)

@iamdharmesh
Copy link
Member Author

@dkotter I have added E2E tests, could you please help with re-reviewing this? Thanks

@iamdharmesh iamdharmesh requested a review from dkotter November 9, 2023 06:40
dkotter
dkotter previously approved these changes Nov 9, 2023
@jeffpaul
Copy link
Member

@iamdharmesh one item that came up in UAT today was the user opt-out links from within the editor/etc. is missing on the Generate Images page linked off the Media Library admin menu. Please ensure we add the opt-out link to the user profile there (if the opt-out setting is allowed) and double-check that we're adding that link across all the ClassifAI features... thanks!

@iamdharmesh
Copy link
Member Author

@jeffpaul I have added a disable feature link to generate images popup and Generate excerpt metabox. Also, please find the status of each feature for the disable link. I have skipped most of the automated features and added a potential UI option to add a link if needed. Could you please help to check and please let me know if you think we should add disable links for the given features? We may handle it in follow-up PR to unblock this PR to be merged.

Language Processing:

  • IBM Watson (Natural Language Understanding):
    Skipped as we have confirmation popup development in progress. (disable link should be added there)

  • OpenAI Chatgpt

    • Excerpt Generation:
    • Title Generation:
    • Resize content:
  • OpenAI Embeddings:
    Skipped due to automated behavior

  • OpenAI Whisper:
    Skipped due to automated behavior.
    But we can add a link to the media model if needed (though it will make the UI a bit messy), what do you think?
    Screenshot 2023-11-17 at 12 15 51 PM

  • Microsoft Azure (Text to Speech):
    Skipped due to automated behavior.
    We can add disable link in metabox and ClassifAI slotfill. what do you think?
    image

Image Processing:

  • AI Vision( Microsoft Azure):
    It skipped due to automated behavior, but we can add a link to the media model if needed.
    image

  • OpenAI DALL·E (Image generation): ✅

cc: @dkotter

@jeffpaul
Copy link
Member

I think adding the disable link to all the places you mention are worthwhile, but defer if @dkotter thinks any aren't necessary. Also, if you could please push a commit to #609 for the post classification disabling that would be helpful... thanks!

@jeffpaul jeffpaul requested a review from dkotter November 17, 2023 17:35
@dkotter
Copy link
Collaborator

dkotter commented Nov 17, 2023

Yeah, if we have places we can easily add this messaging, I think it's worth doing

@dkotter dkotter merged commit 65cdddf into develop Nov 21, 2023
13 checks passed
@dkotter dkotter deleted the enhancement/588 branch November 21, 2023 17:43
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.

Enabling features by Role/User
4 participants