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

feat(GDB-11339): Manage the graphql roles #1809

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

teodossidossev
Copy link
Contributor

@teodossidossev teodossidossev commented Feb 10, 2025

WHAT:

  • Introduced a new SecurityService that delegates all security-related calls to SecurityRestService.
  • Updated controllers and jwt-auth.service to use SecurityService instead of directly invoking SecurityRestService.
  • Implemented bidirectional user model mappers (toUserModelMapper and fromUserModelMapper) using a generic mapObject utility to transform the new backend user model into the UI model.
  • Refactored the UI-to-BE authority mapper (mapAuthoritiesToBackend) to remove legacy READ/WRITE authorities when a combined GraphQL authority exists (e.g., if both READ_REPO_XXX and GRAPHQL_XXX are present, output only "READ_REPO_XXX:GRAPHQL") and correctly handle wildcard (*) cases.
  • Introduced a helper (getRepoFromAuthority) and updated constants, controllers, templates, and locale files to support the new GraphQL-specific authorities and tooltips.

WHY:

  • The backend model has changed—most notably the format of grantedAuthorities (e.g., "READ_REPO_XXX:GRAPHQL")—but we want to preserve existing UI logic without major modifications.
  • The new mapping layer ensures the UI continues to function as before while seamlessly incorporating the new backend features.
  • Modernized promise handling (.then/.catch) replaces deprecated .success/.error methods for improved maintainability.

HOW:

  • Encapsulated all security operations within the new SecurityService.
  • Applied generic object mapping with mapObject to centralize transformation logic.
  • Enhanced the UI-to-BE mapper to consolidate legacy authorities when a corresponding GraphQL authority is present.
  • Updated related modules (constants, authorities-util, controllers, templates, and locale files) to integrate comprehensive GraphQL support.

Testing

Screenshots

image
image

Checklist

  • Branch name
  • Target branch
  • Commit messages
  • Squash commits
  • MR name
  • MR Description
  • Tests

@teodossidossev teodossidossev force-pushed the GDB-11339-manage-graphql-roles branch from 1be22e0 to 65e6cac Compare February 12, 2025 06:30
@@ -88,8 +88,7 @@ angular.module('graphdb.framework.core.services.jwtauth', [
* @param {boolean} justLoggedIn Indicates that the user just logged in.
*/
this.getAuthenticatedUserFromBackend = function(noFreeAccessFallback, justLoggedIn) {
SecurityRestService.getAuthenticatedUser().
success(function(data, status, headers) {
SecurityService.getAuthenticatedUser().then(function(data) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Beware of these changes. There were few places where the success/error handlers were left intentionally due to sertain reasons that I can't recall at the moment but if possible, I'd prefer if we don't switch them here, now

@teodossidossev teodossidossev force-pushed the GDB-11339-manage-graphql-roles branch from 0b068ce to e33ee91 Compare February 13, 2025 10:43
@teodossidossev teodossidossev marked this pull request as ready for review February 13, 2025 10:53
@teodossidossev teodossidossev force-pushed the GDB-11339-manage-graphql-roles branch 2 times, most recently from ad53e4f to 72d129d Compare February 13, 2025 10:58
@teodossidossev teodossidossev marked this pull request as draft February 14, 2025 12:29
@teodossidossev teodossidossev marked this pull request as ready for review February 18, 2025 08:35
@teodossidossev teodossidossev force-pushed the GDB-11339-manage-graphql-roles branch from e23c8fc to 87c30d1 Compare February 18, 2025 08:39
…e usage

- Enhanced the SecurityService that delegates calls to SecurityRestService.
- Updated controllers (security/controllers.js) to use SecurityService instead of directly calling SecurityRestService.
- Modified jwt-auth.service.js to also rely on the new SecurityService.
- Ensured all methods for managing users, roles, and security configs are now available from SecurityService.
… with BE User model changes

The backend model has changed—most notably in the format of grantedAuthorities (e.g. "READ_REPO_XXX:GRAPHQL")—but we want to preserve the existing UI logic without requiring major changes in the UI layer. To achieve this, we introduced a mapping layer that transforms the new backend model into the UI model. This mapping:

- Splits new grantedAuthorities values into their parts (e.g., "READ_REPO_XXX") and adds corresponding UI-specific authorities (e.g., "GRAPHQL_XXX")
- Provides bidirectional conversion via toUserModelMapper (BE → UI) and fromUserModelMapper (UI → BE)
- Utilizes a generic mapping utility (mapObject) to apply transformation rules across object properties

Additionally, we updated our controllers and services to use modern promise handling (.then/.catch) instead of the deprecated .success/.error methods.

These changes ensure that the UI continues to work as before while adding new features and aligning with the changes in the BE model.
…ombined GraphQL authority exists and add GraphQL support

- Updated the UI-to-BE mapping logic in user-mapper.js (mapAuthoritiesToBackend) to:
  - Remove legacy authorities when a combined GraphQL authority is present:
    - If both READ_REPO_XXX and GRAPHQL_XXX exist, output only "READ_REPO_XXX:GRAPHQL" (dropping the standalone READ_REPO_XXX).
    - If both WRITE_REPO_XXX and GRAPHQL_XXX exist, output only "WRITE_REPO_XXX:GRAPHQL" (dropping both standalone READ_REPO_XXX and WRITE_REPO_XXX).
  - Correctly handle wildcard (*) cases.
- Introduced a helper (getRepoFromAuthority) to extract repository IDs and prefixes from authority strings.
- Updated constants and adjusted usage in controllers, templates, and authorities-util to support the new GraphQL-specific authorities.
- Modified locale files to include tooltips for GraphQL rights.
- Fixed jwt-auth.service to properly map principal details from the updated response
- Repositories Service:
  * Updated `getReadableRepositories()` to accept an optional `graphql` parameter
    and delegate to `$jwtAuth.canReadRepo(repo, graphql)`.

- JWT Auth Service:
  * Changed `canReadRepo()` and `canWriteRepo()` to accept `graphql = false`
    and adjust logic within `checkRights()` to skip or include entries ending with `:GRAPHQL`.
  * Ensured that Admin users or wildcard roles properly override new GraphQL checks.

- Authorities Util:
  * Consolidated logic for `READ`, `WRITE`, and `GRAPHQL` prefixes in a single block.
  * Updated the `getRepoFromAuthority()` function to handle `GRAPHQL_PREFIX`.
  * Adjusted parse logic to set `.read`, `.write`, or `.graphql` on a per-repo basis.

- User Mappers & Map-Object:
  * Enhanced `mapObject()` to allow optional `newKey` renaming and `removeOldKey`.
  * Renamed the user’s `grantedAuthorities` field to `grantedAuthoritiesUiModel` when mapping back from BE data.

- Cypress:
  * Added methods for toggling read, write, and GraphQL checkboxes on a per-repo or wildcard basis.
  * Enhanced test coverage in `user-and-access.spec.js` to handle combinations of read/write/GraphQL.
  * Introduced `editUserAuths()` for editing existing permissions, and improved `assertUserAuths()` negative checks.
@teodossidossev teodossidossev force-pushed the GDB-11339-manage-graphql-roles branch from 87c30d1 to 9c7e799 Compare February 18, 2025 12:47
Copy link

Quality Gate Failed Quality Gate failed for 'graphdb-workbench'

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

* The full response is mapped to convert its data property to a UI model.
*
* @param {string} username The username of the user.
* @return {Promise<Object>} A promise that resolves to the full response with a mapped user model.
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the return is a Promise. Could you please check all methods and update the return type and parameters, where it's known? Since you're already in the code, this would be really helpful.

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.

3 participants