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.

Sign up for GitHub

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

Jump to bottom

feat(authz): introduce an owner relationship when creating an entity #1151

Merged
merged 7 commits into from
May 16, 2024
Merged

feat(authz): introduce an owner relationship when creating an entity #1151

merged 7 commits into from
May 16, 2024

Conversation

bobeal
Copy link
Member

@bobeal bobeal commented May 3, 2024

No description provided.

@github-actions github-actions bot added the feature New feature or request label May 3, 2024
@bobeal bobeal linked an issue May 3, 2024 that may be closed by this pull request
Add a special relationship to keep track of the creator of an entity #553
Closed
@bobeal bobeal self-assigned this May 3, 2024
@bobeal bobeal added the authz Relates to auth(z) topics label May 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 3, 2024 •
edited
Loading

Test Results

   57 files  + 33     57 suites  +33   1m 21s ⏱️ +45s
  968 tests +637    968 ✅ +637  0 💤 ±0  0 ❌ ±0 
1 007 runs  +676  1 007 ✅ +676  0 💤 ±0  0 ❌ ±0 

Results for commit 70e5d5e. ± Comparison against base commit 0023f9f.

This pull request removes 12 and adds 641 tests. Note that renamed tests count towards both. 
    "lang": "@none"
    "lang": "fr"
    "lang": "nl"
    "type": "Property",
    "value": "Big Place",
    "value": "Grand Place",
    "value": "Grote Markt",
com.egm.stellio.shared.model.LanguageFilterTests ‑ [1] nl, "languageProperty": {
com.egm.stellio.shared.model.LanguageFilterTests ‑ [2] en, "languageProperty": {
com.egm.stellio.shared.model.LanguageFilterTests ‑ [3] *, "languageProperty": {
…

com.egm.stellio.search.authorization.AuthorizationServiceTests ‑ get authorized entities should return a count of -1 if authentication is not enabled()
com.egm.stellio.search.authorization.AuthorizationServiceTests ‑ get groups memberships should return a count of -1 if authentication is not enabled()
com.egm.stellio.search.authorization.AuthorizationServiceTests ‑ get users should return a count of -1 if authentication is not enabled()
com.egm.stellio.search.authorization.AuthorizationServiceTests ‑ it should authorize access to read()
com.egm.stellio.search.authorization.EnabledAuthorizationServiceTests ‑ it should allow an user that has the right to admin an entity()
com.egm.stellio.search.authorization.EnabledAuthorizationServiceTests ‑ it should allow an user that has the right to read an entity()
com.egm.stellio.search.authorization.EnabledAuthorizationServiceTests ‑ it should allow an user that has the right to update an entity()
com.egm.stellio.search.authorization.EnabledAuthorizationServiceTests ‑ it should create owner link for a set of entities()
com.egm.stellio.search.authorization.EnabledAuthorizationServiceTests ‑ it should return a null filter is user has the stellio-admin role()
com.egm.stellio.search.authorization.EnabledAuthorizationServiceTests ‑ it should return a valid entity filter if user does not have the stellio-admin role()
…

♻️ This comment has been updated with latest results.

@bobeal bobeal force-pushed the feature/553-add-creator-relationship branch from 644ff7a to 531d0a5 Compare May 4, 2024 15:08
@bobeal bobeal requested a review from ranim-n May 4, 2024 15:08
ranim-n
ranim-n reviewed May 6, 2024
View reviewed changes
search-service/src/main/resources/db/migration/V0_41__migrate_to_creator_right.sql Outdated
FROM temporal_entity_attribute
WHERE entity_id IN (select entity_id from entities_more_than_one_admin)
GROUP BY entity_id
), entities_oldest_with_sub AS (
Copy link
Contributor

Choose a reason for hiding this comment

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

entities_with_oldest_sub not entities_oldest_with_sub

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, I am really looking for the oldest sub of each entity

Copy link
Member Author

Choose a reason for hiding this comment

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

ah indeed, misread the comment

search-service/src/main/resources/db/migration/V0_41__migrate_to_creator_right.sql
) l on true
where tea.entity_id = entities_with_oldest_date.entity_id
and tea.created_at = entities_with_oldest_date.created_at
)
Copy link
Contributor

Choose a reason for hiding this comment

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

here why did you use on true in the inner join ?

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, it's not needed here since sub cannot be null

...ice/src/test/kotlin/com/egm/stellio/search/authorization/EnabledAuthorizationServiceTests.kt Outdated
@@ -177,21 +179,21 @@ class EnabledAuthorizationServiceTests {
eq(Some(subjectUuid)),
eq(entityId01),
emptyList(),
listOf(AccessRight.R_CAN_ADMIN)
listOf(R_IS_OWNER, R_CAN_ADMIN)
)
}
}

@Test
fun `it should create admin link for a set of entities`() = runTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

the name of the method should be renamed too : it should create creator link for a set of entities

Copy link
Member Author

Choose a reason for hiding this comment

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

and even it should create owner link ...

ranim-n
ranim-n reviewed May 6, 2024
View reviewed changes
...ice/src/test/kotlin/com/egm/stellio/search/authorization/EnabledAuthorizationServiceTests.kt Outdated
@@ -421,4 +423,92 @@ class EnabledAuthorizationServiceTests {
)
}
}

@Test
fun `it should returned serialized access control entities with other rigths if user is owner`() = runTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

the name of the method : it should return and not it should returned

@bobeal bobeal force-pushed the feature/553-add-creator-relationship branch from 6235e03 to 70e5d5e Compare May 16, 2024 11:22
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
83.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@bobeal bobeal merged commit 59aa1b6 into develop May 16, 2024
10 checks passed
@bobeal bobeal deleted the feature/553-add-creator-relationship branch May 16, 2024 11:36
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ranim-n ranim-n ranim-n approved these changes

Assignees

@bobeal bobeal

Labels
authz Relates to auth(z) topics feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a special relationship to keep track of the creator of an entity
2 participants
@bobeal @ranim-n