-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
SF-3076 Simplify guest role sharing settings #2858
Conversation
src/RealtimeServer/scriptureforge/services/sf-project-migrations.spec.ts
Fixed
Show fixed
Hide fixed
src/RealtimeServer/scriptureforge/services/sf-project-migrations.spec.ts
Fixed
Show fixed
Hide fixed
src/RealtimeServer/scriptureforge/services/sf-project-migrations.spec.ts
Fixed
Show fixed
Hide fixed
src/RealtimeServer/scriptureforge/services/sf-project-migrations.spec.ts
Fixed
Show fixed
Hide fixed
src/RealtimeServer/scriptureforge/services/sf-project-migrations.spec.ts
Fixed
Show fixed
Hide fixed
db21685
to
9ba6a3c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2858 +/- ##
==========================================
+ Coverage 79.81% 80.15% +0.34%
==========================================
Files 528 530 +2
Lines 31015 31116 +101
Branches 5039 5072 +33
==========================================
+ Hits 24755 24942 +187
+ Misses 5481 5386 -95
- Partials 779 788 +9 ☔ View full report in Codecov by Sentry. |
9ba6a3c
to
e6fae7a
Compare
36f65f3
to
da5c791
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work on this Peter. This will be a nice addition once it's finished. I had a couple small comments for you to look at.
Reviewed 34 of 52 files at r1, 32 of 32 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @pmachapman)
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-base.component.ts
line 9 at r2 (raw file):
import { SF_PROJECT_ROLES } from '../../core/models/sf-project-role-info'; export abstract class ShareBaseComponent extends SubscriptionDisposable {
Do we have a Jira issue or a plan for tracking/refactoring where we've use SubscriptionDisposable
to DestroyRef
?
Code quote:
extends SubscriptionDisposable
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-dialog.component.ts
line 137 at r2 (raw file):
canShare && ((this.shareRole === SFProjectRole.CommunityChecker && this.projectDoc?.data?.checkingConfig.checkingEnabled) || this.shareRole !== SFProjectRole.CommunityChecker)
This check seems unnecessary as canShare
is already handling share permissions.
Code quote:
((this.shareRole === SFProjectRole.CommunityChecker && this.projectDoc?.data?.checkingConfig.checkingEnabled) ||
this.shareRole !== SFProjectRole.CommunityChecker)
src/SIL.XForge.Scripture/Services/SFProjectService.cs
line 1498 at r2 (raw file):
{ await projectDoc.SubmitJson0OpAsync(op => op.Set(p => p.RolePermissions[role], permissions)); }
We should add a check if Community Checking is disabled and Community Checkers were previously permitted to share that we unset that settings permission. NOTE: This may need to occur where we are updating the settings instead of here in the code.
Code quote:
else
{
await projectDoc.SubmitJson0OpAsync(op => op.Set(p => p.RolePermissions[role], permissions));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @Github-advanced-security[bot] from 5 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kylebuss and @Nateowami)
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-base.component.ts
line 9 at r2 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
Do we have a Jira issue or a plan for tracking/refactoring where we've use
SubscriptionDisposable
toDestroyRef
?
No, not that I am aware of. @Nateowami might have an idea?
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-dialog.component.ts
line 137 at r2 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
This check seems unnecessary as
canShare
is already handling share permissions.
This part of the if condition is checking whether the role that the user wants to be shared can be shared. i.e. we only want to invite users as Community checkers if Community Checking is enabled in the project settings.
src/SIL.XForge.Scripture/Services/SFProjectService.cs
line 1498 at r2 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
We should add a check if Community Checking is disabled and Community Checkers were previously permitted to share that we unset that settings permission. NOTE: This may need to occur where we are updating the settings instead of here in the code.
This property is unrelated to community checking. SFProjectService.CheckShareKeyValidity()
contains the check you are referring to, via the roles returned by SFProjectService.GetAvailableRoles()
The permission that will currently be set via this endpoint is SF_PROJECT_RIGHTS.joinRight(SFProjectDomain.UserInvites, Operation.Create)
(see SettingsComponent.updateSharingSetting()
). Although, an admin could add any permission they have and grant it to another user.
871c558
to
e91c756
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Nateowami and @pmachapman)
src/SIL.XForge.Scripture/Services/SFProjectService.cs
line 1498 at r2 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
This property is unrelated to community checking.
SFProjectService.CheckShareKeyValidity()
contains the check you are referring to, via the roles returned bySFProjectService.GetAvailableRoles()
The permission that will currently be set via this endpoint is
SF_PROJECT_RIGHTS.joinRight(SFProjectDomain.UserInvites, Operation.Create)
(seeSettingsComponent.updateSharingSetting()
). Although, an admin could add any permission they have and grant it to another user.
This was probably the wrong place in the code to address the scenario that I was thinking of.
Here are the steps:
- Navigate to the settings page.
- Enable Community Checking.
- Enable Allow Community Checkers to invite others.
- Disable Community Checking.
- Enable Community Checking.
- Observe that Allow Community Checkers to invite others is still enabled.
I would expect us to remove the "Allow Community Checkers to invite others" permission if we disable Community Checking all together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kylebuss and @pmachapman)
src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-base.component.ts
line 9 at r2 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
No, not that I am aware of. @Nateowami might have an idea?
No, there isn't. I kind of look at deprecations as self-documenting and generally not being urgent (though it's always nice when they get addressed).
The reason I marked it as deprecated was because I wanted it to no longer get used for new components. I'm guessing it's used here because this was ported from something already using it.
Previously, Nateowami wrote…
Yes, it was ported over. Thanks for explaining why and how to proceed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kylebuss)
src/SIL.XForge.Scripture/Services/SFProjectService.cs
line 1498 at r2 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
This was probably the wrong place in the code to address the scenario that I was thinking of.
Here are the steps:
- Navigate to the settings page.
- Enable Community Checking.
- Enable Allow Community Checkers to invite others.
- Disable Community Checking.
- Enable Community Checking.
- Observe that Allow Community Checkers to invite others is still enabled.
I would expect us to remove the "Allow Community Checkers to invite others" permission if we disable Community Checking all together.
This is by design. I wanted the user's settings to remain in the background when community checking is disabled, so any accidental disabling then re-enabling does not affect the stored sharing settings.
e91c756
to
4e75af0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @pmachapman)
b4da753
to
987df32
Compare
987df32
to
71862d2
Compare
This PR updates the sharing functionality to be configured via a simplified interface in Settings.
To achieve this, the following modifications were made to the Scripture Forge codebase:
rolePermissions
property insf_project
(similar touserPermissions
but per role).rightsByRole.json
which is loaded into TypeScript (in the RealtimeServer and Frontend) and C# (dotnet backend).SFProjectRole
andOperation
in C# to allow permissions to be strongly typed.ShareDialogComponent
andShareControlComponent
viaShareBaseComponent
.mongodb/Migrations/
These changes have made this PR considerably larger than originally envisaged, but the goal is a simplification and unification of permission calculations which should make future PRs with permissions simpler.
Notes
mongodb/Migrations/20241127-AddCreatedByRoleToShareKeys.mongodb
Design Mock Up
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)