refactor: Introduce administrative permission service #3172

seakayone · 2024-04-05T15:10:33Z

refactor: Rename PermissionResponder and add AdministrativePermissionService
Integrate AdministrativePermissionService into PermissionsResponder and PermissionsRestService
disable login for builtIn users
fmt
refactor: make org.knora.webapi.responders.admin.PermissionsResponder#permissionsDataGetADM accept only a KnoraUser
simplify
rename getPermissionData
remove PermissionDataGetADM
move code which calculates User permissions from PermissionsResponder to KnoraUserToUserConverter

Pull Request Checklist

Task Description/Number

Issue Number: DEV-

PR Type

build/chore: maintenance tasks (no production code change)
docs: documentation changes (no production code change)
feat: represents new features
fix: represents bug fixes
perf: performance improvements
refactor: represents production code refactoring
test: adding or refactoring tests (no production code change)
deprecated: Deprecation warning (ideally referencing a migration guide)

Basic requirements for bug fixes and features

Tests for the changes have been added
Docs have been added / updated

Does this PR introduce a breaking change?

Yes

Does this PR change client-test-data?

Yes

codecov · 2024-04-05T15:16:11Z

Codecov Report

Attention: Patch coverage is 93.96552% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 88.80%. Comparing base (a622e4f) to head (055c4e3).
Report is 3 commits behind head on main.

❗ Current head 055c4e3 differs from pull request most recent head 9085f84. Consider uploading reports for the commit 9085f84 to get more accurate results

Files	Patch %	Lines
...webapi/responders/admin/PermissionsResponder.scala	95.12%	4 Missing ⚠️
...r/permissionsmessages/PermissionsMessagesADM.scala	87.50%	1 Missing ⚠️
...ice/admin/api/service/PermissionsRestService.scala	85.71%	1 Missing ⚠️
...bapi/slice/admin/domain/service/GroupService.scala	88.88%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3172       +/-   ##
===========================================
+ Coverage   13.22%   88.80%   +75.57%     
===========================================
  Files         270      271        +1     
  Lines       22244    22206       -38     
===========================================
+ Hits         2941    19719    +16778     
+ Misses      19303     2487    -16816

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…Service

…nd PermissionsRestService

…#permissionsDataGetADM accept only a KnoraUser

BalduinLandolt

LGTM

BalduinLandolt · 2024-04-09T06:51:21Z

integration/src/test/scala/org/knora/webapi/core/LayersTest.scala

    ApiRoutes & AdminApiEndpoints & ApiV2Endpoints & AppRouter & AssetPermissionsResponder & Authenticator &
      AuthorizationRestService & CacheServiceRequestMessageHandler & CardinalityHandler & ConstructResponseUtilV2 &
      DspIngestClient & GravsearchTypeInspectionRunner & GroupsResponderADM & GroupsRestService & GroupService &
-      HttpServer & IIIFRequestMessageHandler & InferenceOptimizationService & IriConverter & ListsResponder &
+      HttpServer & IIIFRequestMessageHandler & InferenceOptimizationService & IriConverter & KnoraUserToUserConverter & ListsResponder &
      ListsResponderV2 & MessageRelay & OntologyCache & OntologyHelpers & OntologyInferencer & OntologyRepo &
-      OntologyResponderV2 & PermissionUtilADM & PermissionsResponderADM & PermissionsRestService & ProjectExportService &
+      OntologyResponderV2 & PermissionUtilADM & PermissionsResponder & PermissionsRestService & ProjectExportService &
      ProjectExportStorageService & ProjectImportService & ProjectService & ProjectRestService & QueryTraverser &
      RepositoryUpdater & ResourceUtilV2 & ResourcesResponderV2 & RestCardinalityService & SearchApiRoutes &
      SearchResponderV2 & StandoffResponderV2 & StandoffTagUtilV2 & State & TestClientService & TriplestoreService &


suggestion: could we put those on one line each? Scalafmt doesn't do it automatically, but it doesn't undo it either, I think. And the diffs would be less nasty.

BalduinLandolt · 2024-04-09T06:55:08Z

...rc/test/scala/org/knora/webapi/slice/admin/domain/service/KnoraUserToUserConverterSpec.scala

+import org.knora.webapi.slice.admin.domain.model.UserStatus
+import org.knora.webapi.slice.admin.domain.model.Username
+
+object KnoraUserToUserConverterSpec extends E2EZSpec {


question: in its nature, this test is not an end-to-end test. Should it really be derived from E2EZSpec?

BalduinLandolt · 2024-04-09T06:55:46Z

webapi/src/main/scala/org/knora/webapi/core/LayersLive.scala

      CacheServiceRequestMessageHandler & CardinalityHandler & ConstructResponseUtilV2 &
      GravsearchTypeInspectionRunner & GroupsResponderADM & GroupsRestService & GroupService & HttpServer &
      IIIFRequestMessageHandler & InferenceOptimizationService & InstrumentationServerConfig & IriConverter &
-      JwtService & ListsResponder & ListsResponderV2 & MessageRelay & OntologyCache & OntologyHelpers &
-      OntologyInferencer & OntologyResponderV2 & PermissionsResponderADM & PermissionsRestService &
+      JwtService & KnoraUserToUserConverter & ListsResponder & ListsResponderV2 & MessageRelay & OntologyCache & OntologyHelpers &
+      OntologyInferencer & OntologyResponderV2 & PermissionsResponder & PermissionsRestService &
      PermissionUtilADM & ProjectService & ProjectExportService & ProjectExportStorageService &
      ProjectImportService & ProjectRestService & QueryTraverser & RepositoryUpdater & ResourcesResponderV2 &
      ResourceUtilV2 & ResourceUtilV2 & RestCardinalityService & SearchApiRoutes &


BalduinLandolt · 2024-04-09T07:00:55Z

webapi/src/main/scala/org/knora/webapi/slice/admin/api/AdminApiModule.scala

-    AppConfig & AssetPermissionsResponder & AuthorizationRestService & BaseEndpoints & CacheService &
+    AppConfig & AdministrativePermissionService & AssetPermissionsResponder & AuthorizationRestService & BaseEndpoints & CacheService &
      GroupsResponderADM & GroupService & HandlerMapper & KnoraProjectService & KnoraResponseRenderer &
      KnoraUserService & KnoraUserToUserConverter & ListsResponder & MaintenanceService & OntologyCache &
-      PasswordService & PermissionsResponderADM & ProjectExportService & ProjectImportService & ProjectService &
+      PasswordService & PermissionsResponder & ProjectExportService & ProjectImportService & ProjectService &


mpro7 · 2024-04-09T06:56:52Z

...pi/src/main/scala/org/knora/webapi/slice/admin/domain/service/KnoraUserToUserConverter.scala

+      /* Follow the precedence rule:
+         1. ProjectAdmin > 2. CustomGroups > 3. ProjectMember > 4. KnownUser
+         Permissions are added following the precedence level from the highest to the lowest. As soon as one set
+         of permissions is written into the buffer, any additionally permissions do not need to be added. */


I would suggest to move it to docsstring above, which will make it visible on function hover.

TBH, I was even considering to remove it. However it explains why the implementation below is how it is and I think that is the right place.

mpro7 · 2024-04-09T08:27:18Z

webapi/src/main/scala/org/knora/webapi/slice/admin/domain/service/GroupService.scala

-  def toKnoraGroup(group: Group): KnoraGroup =
-    KnoraGroup(
-      id = GroupIri.unsafeFrom(group.id),
-      groupName = GroupName.unsafeFrom(group.name),
-      groupDescriptions = GroupDescriptions.unsafeFrom(group.descriptions),
-      status = GroupStatus.from(group.status),
-      belongsToProject = group.project.map(it => ProjectIri.unsafeFrom(it.id)),
-      hasSelfJoinEnabled = GroupSelfJoin.from(group.selfjoin),
-    )


Could you bring it back please? It is unused not, but will be soon - group slice WIP.

Please readd it yourself when in use and ideally tested.

seakayone force-pushed the refactor/introduce-administrative-permission-service branch from f623943 to 817e78a Compare April 5, 2024 15:11

seakayone force-pushed the refactor/introduce-administrative-permission-service branch 2 times, most recently from 0e31326 to 06bd4ee Compare April 8, 2024 11:50

seakayone added 26 commits April 8, 2024 15:59

refactor: Rename PermissionResponder and add AdministrativePermission…

1dfa7f4

…Service

Integrate AdministrativePermissionService into PermissionsResponder a…

7f1d222

…nd PermissionsRestService

disable login for builtIn users

c97b28d

fmt

81962ae

refactor: make org.knora.webapi.responders.admin.PermissionsResponder…

665e1fb

…#permissionsDataGetADM accept only a KnoraUser

simplify

fa414e7

rename getPermissionData

853d63c

cleanup spec

023fd87

cleanup GroupService

01a92a8

header

0c75589

Use ZIO.foreach (simplify)

bbe4779

Use ZIO.foreach (simplify)

f993963

cleanup

c60b915

remove unused tuple part

f086ef2

simplify

2a12b16

simplify

2ff0400

simplify

58966b5

simplify

53a26ac

fmt

7619126

cleanup

73ff423

add types

e3714d9

fmt

c717468

move code to KnoraUserToUserConverter

3c79e8c

improve type safety internally

e90b109

migrate KnoraUserToUserConverterSpec to zio-test E2EZSpec

c1a2d25

header

8f17f68

seakayone force-pushed the refactor/introduce-administrative-permission-service branch from e806b4a to 8f17f68 Compare April 8, 2024 14:00

seakayone added 3 commits April 8, 2024 16:21

cleanup

c23672f

cleanup

aeb6126

align naming findByIds

9085f84

seakayone marked this pull request as ready for review April 8, 2024 14:27

seakayone requested review from BalduinLandolt, mpro7 and siers as code owners April 8, 2024 14:27

BalduinLandolt approved these changes Apr 9, 2024

View reviewed changes

mpro7 approved these changes Apr 9, 2024

View reviewed changes

seakayone merged commit 80ca581 into main Apr 9, 2024
11 checks passed

seakayone deleted the refactor/introduce-administrative-permission-service branch April 9, 2024 12:18

daschbot mentioned this pull request Apr 8, 2024

chore(main): Release 30.12.0 #3171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Introduce administrative permission service #3172

refactor: Introduce administrative permission service #3172

seakayone commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading

BalduinLandolt left a comment

BalduinLandolt Apr 9, 2024

BalduinLandolt Apr 9, 2024

BalduinLandolt Apr 9, 2024

BalduinLandolt Apr 9, 2024

mpro7 Apr 9, 2024

seakayone Apr 9, 2024

mpro7 Apr 9, 2024

seakayone Apr 9, 2024

refactor: Introduce administrative permission service #3172

refactor: Introduce administrative permission service #3172

Conversation

seakayone commented Apr 5, 2024 • edited Loading

Pull Request Checklist

Task Description/Number

PR Type

Basic requirements for bug fixes and features

Does this PR introduce a breaking change?

Does this PR change client-test-data?

codecov bot commented Apr 5, 2024 • edited Loading

Codecov Report

BalduinLandolt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seakayone commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading