-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Org page permission improvements #881
Comments
quoting #800
so non org members should be able to view the org, and the admins. This has been in the issue since May 6th, do we now disagree with that? |
also I created this issue yesterday, one of them should be closed as duplicate #879 |
Closed #879 in favor of this issue since this one tracks more work. |
I'd already implemented the "non-members cannot see settings tab" feature but hadn't yet pushed it into #800, so here are some screenshots. Logged in as Test User, who's not a member of the Test Org: Logged in as Test Admin, who's also not a member but is an admin so he can see and do everything: |
I've implemented the easy, UI-only changes: hide the settings tab entirely if there's nothing you can do there (you're not a member AND you're not a sire admin), and hide the "Leave Org" button if you're not a member (e.g., a site admin will not see "Leave Org" for orgs he's not part of, but he can still see the Settings tab because he has access to the "Delete Org" button). What remains is the GraphQL permissions question. Here's what I'm thinking:
Let me know if any part of that design is incorrect, please. |
@hahn-kev apologies for overlooking the issue you created and for not reading the description of #779 carefully enough.
These are the only things that I'm pretty sure aren't quite right: Everything is private right now by default. I think that's an important default and we don't want to change it. There has been a request to be able to make a project public. I just documented it in vNext here: #890. So, it's not scheduled yet. I just wanted to be able to point to it. |
I think we want to make projects public by default (unless confidential), most projects have no reason not to be public so that's why we chose the terminology we did, was to lead people to making their projects public. However for any project with unknown confidentiality it is to be considered private as we just don't know, that's very different from default being public. |
OK, nevermind. Yes: what he said ☝️ 😄 |
@hahn-kev @myieye - Code style and correctness question. In #800 (comment), @myieye suggested creating an extension method like I've just pushed commit fee08ab with my first crack at creating an OrgMembershipRequired middleware (also reproduced below so you don't have to click), but I'm unsure if throwing an UnauthorizedAccessException is the right thing to do here. https://chillicream.com/docs/hotchocolate/v13/execution-engine/field-middleware doesn't say anything about whether or not middleware should throw exceptions; it only mentions the case where you might short-circuit reaching the default resolver by refusing to call descriptor.Field(o => o.Members).Use(next => async context =>
{
await next(context);
var result = context.Result;
if (result is List<OrgMember> members)
{
// We want to throw if not logged in, so use User rather than MaybeUser below
var user = context.Service<LexBoxApi.Auth.LoggedInContext>().User;
if (user.IsAdmin || members.Any(om => om.UserId == user.Id))
{
return;
}
}
throw new UnauthorizedAccessException("Must be org member");
}); That was the correctness question, now for the code style question. While I could turn this into a generic class similar to the RefreshJwtProjectMembershipMiddleware class, this code is so specific to P.S. Yes, I'm aware this doesn't actually need to be P.P.S. Also, I know this isn't the correct logic according to #879; this is just an example. |
I suppose we could put a list of orgs into the user's JWT, just like we have the list of projects. Then I'd be able to look up org membership via LoggedInContext.MaybeUser and not have to have access to the |
Yes I would suggest doing that, though to start with we can keep the jwt simple and just have the orgs list be a json list of objects. We did some special stuff for projects to save space but I'm not worried here. Make sure you don't break existing logins where they don't have the orgs property in the jwt. There's already a unit test for this so just run the test referencing a known good jwt. |
Thanks for the tip. My initial implementation, just copying what we did for projects, ended up breaking the LexAuthUserTests with the exception "The input does not contain any JSON tokens. Expected the input to start with a valid JSON token, when isFinalBlock is true." I debugged the tests and found the line throwing the error was I'll dive into this and figure out how to handle that case smoothly; perhaps a simple "if the claim type should be an array but it's an empty string or null, then treat it as an empty array" would be enough to get working. Edit: Or maybe it's just that I didn't assign the "orgs" claim the array value type; the debugger is showing me a ValueType of |
Commit 6bbcfb2 removes the special |
Well, this is interesting. I just tried logging in as the Test User, who doesn't have any org memberships, then going to the org list page. Result: an InvalidOperationException with message "A second operation was started on this context instance before a previous operation completed. This is usually caused by different threads concurrently using the same instance of DbContext. For more information on how to avoid threading issues with DbContext, see https://go.microsoft.com/fwlink/?linkid=2097913." Logs including stack trace and SQL queries logged just before the error[lexbox-api] info: Microsoft.AspNetCore.Routing.EndpointMiddleware[0] [lexbox-api] => TraceId:412015e9dc83c7dd187f97e27d861218 => ConnectionId:0HN4F9K5IPDDD => RequestPath:/api/graphql RequestId:0HN4F9K5IPDDD:00000004 [lexbox-api] Executing endpoint 'Hot Chocolate GraphQL HTTP Pipeline' [lexbox-api] info: Microsoft.EntityFrameworkCore.Database.Command[20101] [lexbox-api] => TraceId:412015e9dc83c7dd187f97e27d861218 => ConnectionId:0HN4F9K5IPDDD => RequestPath:/api/graphql RequestId:0HN4F9K5IPDDD:00000004 [lexbox-api] Executed DbCommand (3ms) [Parameters=[@__orgId_0='?' (DbType = Guid)], CommandType='Text', CommandTimeout='30'] [lexbox-api] SELECT o."Id", o."CreatedDate", o."UpdatedDate", o."Name", t2."Id", t2."Code", t2."Name", t2."Type", t2."UserCount", t2."CreatedDate", t2."Id0", t2."Id1", t3."Id", t3."Role", t3.c, t3."Id0", t3."Name" [lexbox-api] FROM "Orgs" AS o [lexbox-api] LEFT JOIN ( [lexbox-api] SELECT t0."Id", t0."Code", t0."Name", t0."Type", ( [lexbox-api] SELECT count(*)::int [lexbox-api] FROM "ProjectUsers" AS p1 [lexbox-api] INNER JOIN ( [lexbox-api] SELECT p2."Id", p2."Code", p2."CreatedDate", p2."DeletedDate", p2."Description", p2."IsConfidential", p2."LastCommit", p2."MigratedDate", p2."Name", p2."ParentId", p2."ProjectOrigin", p2."ResetStatus", p2."RetentionPolicy", p2."Type", p2."UpdatedDate" [lexbox-api] FROM "Projects" AS p2 [lexbox-api] WHERE p2."DeletedDate" IS NULL [lexbox-api] ) AS t1 ON p1."ProjectId" = t1."Id" [lexbox-api] WHERE t1."DeletedDate" IS NULL AND t0."Id" = p1."ProjectId") AS "UserCount", t0."CreatedDate", o0."Id" AS "Id0", t."Id" AS "Id1", o0."OrgId" [lexbox-api] FROM "OrgProjects" AS o0 [lexbox-api] INNER JOIN ( [lexbox-api] SELECT p."Id", p."DeletedDate" [lexbox-api] FROM "Projects" AS p [lexbox-api] WHERE p."DeletedDate" IS NULL [lexbox-api] ) AS t ON o0."ProjectId" = t."Id" [lexbox-api] INNER JOIN ( [lexbox-api] SELECT p0."Id", p0."Code", p0."CreatedDate", p0."Name", p0."Type" [lexbox-api] FROM "Projects" AS p0 [lexbox-api] WHERE p0."DeletedDate" IS NULL [lexbox-api] ) AS t0 ON o0."ProjectId" = t0."Id" [lexbox-api] WHERE t."DeletedDate" IS NULL [lexbox-api] ) AS t2 ON o."Id" = t2."OrgId" [lexbox-api] LEFT JOIN ( [lexbox-api] SELECT o1."Id", o1."Role", TRUE AS c, u."Id" AS "Id0", u."Name", o1."OrgId" [lexbox-api] FROM "OrgMembers" AS o1 [lexbox-api] INNER JOIN "Users" AS u ON o1."UserId" = u."Id" [lexbox-api] ) AS t3 ON o."Id" = t3."OrgId" [lexbox-api] WHERE o."Id" = @__orgId_0 [lexbox-api] ORDER BY o."Id", t2."Id0", t2."Id1", t2."Id", t3."Id" [lexbox-api] info: Microsoft.EntityFrameworkCore.Database.Command[20101] [lexbox-api] => TraceId:412015e9dc83c7dd187f97e27d861218 => ConnectionId:0HN4F9K5IPDDD => RequestPath:/api/graphql RequestId:0HN4F9K5IPDDD:00000004 [lexbox-api] Executed DbCommand (3ms) [Parameters=[@__userId_0='?' (DbType = Guid)], CommandType='Text', CommandTimeout='30'] [lexbox-api] SELECT t."Id", t."CanCreateProjects", t."CreatedById", t."CreatedDate", t."Email", t."EmailVerified", t."GoogleId", t."IsAdmin", t."LastActive", t."LocalizationCode", t."Locked", t."Name", t."PasswordHash", t."PasswordStrength", t."Salt", t."UpdatedDate", t."Username", t0."Id", t0."CreatedDate", t0."ProjectId", t0."Role", t0."UpdatedDate", t0."UserId", t0."Id0", t0."Code", t0."CreatedDate0", t0."DeletedDate", t0."Description", t0."IsConfidential", t0."LastCommit", t0."MigratedDate", t0."Name", t0."ParentId", t0."ProjectOrigin", t0."ResetStatus", t0."RetentionPolicy", t0."Type", t0."UpdatedDate0", t2."Id", t2."CreatedDate", t2."OrgId", t2."Role", t2."UpdatedDate", t2."UserId", t2."Id0", t2."CreatedDate0", t2."Name", t2."UpdatedDate0" [lexbox-api] FROM ( [lexbox-api] SELECT u."Id", u."CanCreateProjects", u."CreatedById", u."CreatedDate", u."Email", u."EmailVerified", u."GoogleId", u."IsAdmin", u."LastActive", u."LocalizationCode", u."Locked", u."Name", u."PasswordHash", u."PasswordStrength", u."Salt", u."UpdatedDate", u."Username" [lexbox-api] FROM "Users" AS u [lexbox-api] WHERE u."Id" = @__userId_0 [lexbox-api] LIMIT 1 [lexbox-api] ) AS t [lexbox-api] LEFT JOIN ( [lexbox-api] SELECT p."Id", p."CreatedDate", p."ProjectId", p."Role", p."UpdatedDate", p."UserId", t1."Id" AS "Id0", t1."Code", t1."CreatedDate" AS "CreatedDate0", t1."DeletedDate", t1."Description", t1."IsConfidential", t1."LastCommit", t1."MigratedDate", t1."Name", t1."ParentId", t1."ProjectOrigin", t1."ResetStatus", t1."RetentionPolicy", t1."Type", t1."UpdatedDate" AS "UpdatedDate0" [lexbox-api] FROM "ProjectUsers" AS p [lexbox-api] INNER JOIN ( [lexbox-api] SELECT p0."Id", p0."Code", p0."CreatedDate", p0."DeletedDate", p0."Description", p0."IsConfidential", p0."LastCommit", p0."MigratedDate", p0."Name", p0."ParentId", p0."ProjectOrigin", p0."ResetStatus", p0."RetentionPolicy", p0."Type", p0."UpdatedDate" [lexbox-api] FROM "Projects" AS p0 [lexbox-api] WHERE p0."DeletedDate" IS NULL [lexbox-api] ) AS t1 ON p."ProjectId" = t1."Id" [lexbox-api] WHERE t1."DeletedDate" IS NULL [lexbox-api] ) AS t0 ON t."Id" = t0."UserId" [lexbox-api] LEFT JOIN ( [lexbox-api] SELECT o."Id", o."CreatedDate", o."OrgId", o."Role", o."UpdatedDate", o."UserId", o0."Id" AS "Id0", o0."CreatedDate" AS "CreatedDate0", o0."Name", o0."UpdatedDate" AS "UpdatedDate0" [lexbox-api] FROM "OrgMembers" AS o [lexbox-api] INNER JOIN "Orgs" AS o0 ON o."OrgId" = o0."Id" [lexbox-api] ) AS t2 ON t."Id" = t2."UserId" [lexbox-api] ORDER BY t."Id", t0."Id", t0."Id0", t2."Id" [lexbox-api] info: Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler[10] [lexbox-api] => TraceId:412015e9dc83c7dd187f97e27d861218 => ConnectionId:0HN4F9K5IPDDD => RequestPath:/api/graphql RequestId:0HN4F9K5IPDDD:00000004 [lexbox-api] AuthenticationScheme: Cookies signed in. [lexbox-api] info: Microsoft.EntityFrameworkCore.Database.Command[20101] [lexbox-api] => TraceId:412015e9dc83c7dd187f97e27d861218 => ConnectionId:0HN4F9K5IPDDD => RequestPath:/api/graphql RequestId:0HN4F9K5IPDDD:00000004 [lexbox-api] Executed DbCommand (2ms) [Parameters=[@p1='?' (DbType = Guid), @p0='?' (DbType = DateTime)], CommandType='Text', CommandTimeout='30'] [lexbox-api] UPDATE "Users" SET "LastActive" = @p0 [lexbox-api] WHERE "Id" = @p1; [lexbox-api] fail: Microsoft.EntityFrameworkCore.Query[10100] [lexbox-api] => TraceId:412015e9dc83c7dd187f97e27d861218 => ConnectionId:0HN4F9K5IPDDD => RequestPath:/api/graphql RequestId:0HN4F9K5IPDDD:00000004 [lexbox-api] An exception occurred while iterating over the results of a query for context type 'LexData.LexBoxDbContext'. [lexbox-api] System.InvalidOperationException: A second operation was started on this context instance before a previous operation completed. This is usually caused by different threads concurrently using the same instance of DbContext. For more information on how to avoid threading issues with DbContext, see https://go.microsoft.com/fwlink/?linkid=2097913. [lexbox-api] at Microsoft.EntityFrameworkCore.Infrastructure.Internal.ConcurrencyDetector.EnterCriticalSection() [lexbox-api] at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync() [lexbox-api] System.InvalidOperationException: A second operation was started on this context instance before a previous operation completed. This is usually caused by different threads concurrently using the same instance of DbContext. For more information on how to avoid threading issues with DbContext, see https://go.microsoft.com/fwlink/?linkid=2097913. [lexbox-api] at Microsoft.EntityFrameworkCore.Infrastructure.Internal.ConcurrencyDetector.EnterCriticalSection() [lexbox-api] at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync() [lexbox-api] fail: LexBoxApi.GraphQL.ErrorLoggingDiagnosticsEventListener[0] [lexbox-api] => TraceId:412015e9dc83c7dd187f97e27d861218 => ConnectionId:0HN4F9K5IPDDD => RequestPath:/api/graphql RequestId:0HN4F9K5IPDDD:00000004 [lexbox-api] ResolverError: Unexpected Execution Error [lexbox-api] System.InvalidOperationException: A second operation was started on this context instance before a previous operation completed. This is usually caused by different threads concurrently using the same instance of DbContext. For more information on how to avoid threading issues with DbContext, see https://go.microsoft.com/fwlink/?linkid=2097913. [lexbox-api] at Microsoft.EntityFrameworkCore.Infrastructure.Internal.ConcurrencyDetector.EnterCriticalSection() [lexbox-api] at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync() [lexbox-api] at Microsoft.EntityFrameworkCore.Query.ShapedQueryCompilingExpressionVisitor.SingleOrDefaultAsync[TSource](IAsyncEnumerable`1 asyncEnumerable, CancellationToken cancellationToken) [lexbox-api] at Microsoft.EntityFrameworkCore.Query.ShapedQueryCompilingExpressionVisitor.SingleOrDefaultAsync[TSource](IAsyncEnumerable`1 asyncEnumerable, CancellationToken cancellationToken) [lexbox-api] at LexBoxApi.Auth.LexAuthService.RefreshUser(Guid userId, String updatedValue) in /src/backend/LexBoxApi/Auth/LexAuthService.cs:line 90 [lexbox-api] at LexBoxApi.GraphQL.CustomTypes.RefreshJwtProjectMembershipMiddleware.RefreshUser(IMiddlewareContext context, Guid userId) in /src/backend/LexBoxApi/GraphQL/CustomTypes/RefreshJwtProjectMembershipMiddleware.cs:line 70 [lexbox-api] at LexBoxApi.GraphQL.CustomTypes.RefreshJwtProjectMembershipMiddleware.InvokeAsync(IMiddlewareContext context) in /src/backend/LexBoxApi/GraphQL/CustomTypes/RefreshJwtProjectMembershipMiddleware.cs:line 36 [lexbox-api] at HotChocolate.Utilities.MiddlewareCompiler`1.ExpressionHelper.AwaitTaskHelper(Task task) [lexbox-api] at DataAnnotatedModelValidations.Middleware.ValidatorMiddleware.InvokeAsync(IMiddlewareContext context) [lexbox-api] at HotChocolate.Utilities.MiddlewareCompiler`1.ExpressionHelper.AwaitTaskHelper(Task task) [lexbox-api] at HotChocolate.Execution.Processing.Tasks.ResolverTask.ExecuteResolverPipelineAsync(CancellationToken cancellationToken) [lexbox-api] at HotChocolate.Execution.Processing.Tasks.ResolverTask.TryExecuteAsync(CancellationToken cancellationToken) The C# stack trace shows RefreshJwtProjectMembershipMiddleware failing. The OpenTelemetry trace shows two occurrences of RefreshUser. I believe that one of them was to refresh the user's list of orgs, which did not fail, while the failing one was the existing code to refresh the user's list of projects. However, that's simply a race condition and it could easily have gone the other way, and then the list of orgs would have failed. The cause is that when I created RefreshJwtOrgMembershipMiddleware, I made the Looks like what I'll need to do is instead of creating a new RefreshJwtOrgMembershipMiddleware class alongside RefreshJwtProjectMembershipMiddleware, I'll need to have just one class that refreshes both membership lists. Then it will only be called once and there will be no situation where RefreshUser gets double-called. |
Fixed that bug, but now there's a new issue: if Test User, who's not a member of the project and thus can only see public projects, tries to click on a project link, he gets a GraphQL error because PermissionService threw an UnauthorizedAccessException on the |
@rmunn I'm glad you're making some good progress here 😎 I think you've run into our incomplete "public projects" feature. We haven't actually done any work for allowing users to see non-confidential projects. At this point, I see three options:
|
Changing I've looked into option 2, and it would actually require pushing some permission properties deep into the ProjectTable component in order to decide whether the I think what I should probably do is, for now, set the project table links to not preload on hover. And when you click on them, the GraphQL code in the client should expect "permisson denied" errors and return a 404 or a redirect or something appropriate. That way there will be minimal code changes needed, but non-members won't be able to click on the project and view anything at all. |
@rmunn Oh, right. We definitely don't want to just change But, handling permission denied errors is code debt and also weird UX. 🙁 Why not instead of adapting |
Yeah, I pushed commit 349556c because it's a quick-and-dirty solution to the issue, but it's definitely not ideal. (As you can see). I'll go ahead and start implementing |
I think CanAccessProject is a poor name now with the changes we are working on. How about renaming the existing one to CanSyncProject, and create a new permission called CanViewProject which is true if the project is Public? Then we can change some things from using CanSync to CanView where relevant. |
Commit 96d2761 renames CanAccessProject to CanSyncProject; I'll start working on CanViewProject but might not finish implementing that before the end of the working day, so you might or might not see that code today. |
Implemented in commit 04ee8d1. Testing has exposed a bug in the projects listing code from #865, which is considering |
Not actually a bug, I just hadn't asked for |
Okay, #888 has now implemented everything I'm aware of. @hahn-kev @myieye - please feel free to test it to let me know if there are any new issues I need to tackle (e.g., if we change our minds about one of these permissions decidions). I ran out of time to give it a self-review for code quality issues today, so if you want to give it a review, just be aware that there might be some simple mistakes I would have caught had I had time for a self-review. |
I've been able to do a self-review of #888 now. It's ready to merge unless there are more design changes that come up on review. |
Open todos from org page PR
The text was updated successfully, but these errors were encountered: