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

Org page permission improvements #881

Closed
2 tasks done
Tracked by #576
myieye opened this issue Jun 13, 2024 · 25 comments · Fixed by #888
Closed
2 tasks done
Tracked by #576

Org page permission improvements #881

myieye opened this issue Jun 13, 2024 · 25 comments · Fixed by #888
Assignees
Milestone

Comments

@myieye
Copy link
Contributor

myieye commented Jun 13, 2024

Open todos from org page PR

@myieye myieye mentioned this issue Jun 13, 2024
14 tasks
@myieye myieye added this to the v2 (orgs) milestone Jun 13, 2024
@myieye myieye mentioned this issue Jun 13, 2024
@hahn-kev
Copy link
Collaborator

hahn-kev commented Jun 13, 2024

quoting #800

Permissions:

Org admins
member CRUD (for now they will be auto added, but we may want that to be an invite process in the future)
Org members
view member list
Non org members
view org
list admins (including email tbd?), not other members

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?

@hahn-kev
Copy link
Collaborator

also I created this issue yesterday, one of them should be closed as duplicate #879

@rmunn
Copy link
Contributor

rmunn commented Jun 14, 2024

Closed #879 in favor of this issue since this one tracks more work.

@rmunn
Copy link
Contributor

rmunn commented Jun 14, 2024

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:

image

Logged in as Test Admin, who's also not a member but is an admin so he can see and do everything:

image

@rmunn
Copy link
Contributor

rmunn commented Jun 14, 2024

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:

  • All public (non-confidential) projects belonging to an org are visible to everyone, and show up on its Projects list.
  • Confidential projects are only exposed to org members or site admins
  • The Members tab shows org admins to everyone (I believe that's the design Kevin has in mind)
  • Members tab shows member names to other members
    • Org admins can also see usernames and email addresses
    • Regular org members can only see names, not username/email (so that column gets hidden since it will be empty)
  • All of this is enforced by GQL permissions rules, so that GraphQL queries only return data you're allowed to see based on your org memberships and admin status

Let me know if any part of that design is incorrect, please.

@rmunn rmunn self-assigned this Jun 14, 2024
@myieye
Copy link
Contributor Author

myieye commented Jun 14, 2024

@hahn-kev apologies for overlooking the issue you created and for not reading the description of #779 carefully enough.

@rmunn

  • All public (non-confidential) projects belonging to an org are visible to everyone, and show up on its Projects list.
  • Confidential projects are only exposed to org members or site admins

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.
Orgs let us make org projects non-private by default for org members.
Confidential means even other org members shouldn't see 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.

@hahn-kev
Copy link
Collaborator

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.

@myieye
Copy link
Contributor Author

myieye commented Jun 14, 2024

OK, nevermind. Yes: what he said ☝️ 😄

@rmunn
Copy link
Contributor

rmunn commented Jun 17, 2024

@hahn-kev @myieye - Code style and correctness question. In #800 (comment), @myieye suggested creating an extension method like descriptor.Field(org => org.Members).OrgMembershipRequired(), similar to the IObjectFieldDescriptor AdminRequired extension in AdminRequiredAttribute.cs. However, since OrgMembershipRequired would need to know the ID of the org in question, the only way I've found so far to implement this idea is using middleware.

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 next(context). But in code that needs context.Result, you can't do that. So is it correct to throw an exception inside HotChocolate middleware, or is there a different approach I'm missing somehow?

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 org.Members that it seems right to me to just leave it as an inline-defined function. Would you be okay with this, or would you want me to move this into an extension method/class even though it can only be used on the org.Members field?

P.S. Yes, I'm aware this doesn't actually need to be async; I'll trim off the async once I've settled that I won't actually need to access any async APIs in resolving this.

P.P.S. Also, I know this isn't the correct logic according to #879; this is just an example.

@rmunn
Copy link
Contributor

rmunn commented Jun 17, 2024

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 org.Members list. Will bring up this idea in the team meeting today.

@hahn-kev
Copy link
Collaborator

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.

@rmunn
Copy link
Contributor

rmunn commented Jun 18, 2024

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 JsonSerializer.Deserialize<JsonObject>(claim.Value) when claim.Type was "orgs" and claim.Value was the empty string.

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 "http://www.w3.org/2001/XMLSchema#string". So it's probably even simpler than that; I just forgot to set the ValueType of the "orgs" claim, it seems.

@rmunn
Copy link
Contributor

rmunn commented Jun 18, 2024

Commit 6bbcfb2 removes the special OrgsJson code I wrote to mimic ProjectsJson, and the unit tests from LexAuthUserTests now pass again. So good call on having the orgs list be just a list of objects; as you said, we're not going to get users belonging to hundreds of orgs so there's no need to squeeze bytes there.

@rmunn
Copy link
Contributor

rmunn commented Jun 18, 2024

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 REFRESHED_USER_KEY constant to be a different string than the one from RefreshJwtProjectMembershipMiddleware. So the UserAlreadyRefreshed logic allowed both refreshes to run, which resulted in the same DbContext object being called from two different async operations.

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.

@rmunn
Copy link
Contributor

rmunn commented Jun 18, 2024

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 projectByCode query. We'll need to tweak the project page's load function to handle UnauthorizedAccessException and not just throw up an error message when it happens, and we'll need to decide how to handle it. Maybe just deny access to the page and navigate back where the user came from?

@myieye
Copy link
Contributor Author

myieye commented Jun 18, 2024

@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:

  1. Don't actually show "public" projects for now 😢
  2. Only list public projects, but don't let users navigate to them unless they're allowed to based on our current code 🙁
  3. Make the exciting permission change of adapting PermissionService.CanAccessProject to return true if the project is explicitly non-confidential (i.e. == false). I'm guessing we'd want to implement that much like/next to ProjectService.LookupProjectId. I.e. something like a ProjectService.IsPublic method that uses a memory cache, so we don't hit the database every time.

@rmunn
Copy link
Contributor

rmunn commented Jun 20, 2024

Changing CanAccessProject to be true if non-confidential would be too dangerous at the moment, because it's also used to control access permissions to the Mercurial repo. Which do not distinguish between pushing and pulling from a permissions standpoint. Until we implement more fine-grained permissions such as CanViewProject (seeing metadata but not repo), CanReadProjectRepo (can pull but not push, and can also see metadata) and CanWriteToProjectRepo (can also push as well as the rest), it's not safe to change the definition of CanAccessProject.

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 <a> should be wrapped around the project name or not. I think I'd rather avoid that.

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.

@myieye
Copy link
Contributor Author

myieye commented Jun 20, 2024

@rmunn Oh, right. We definitely don't want to just change CanAccessProject. Good catch 👍

But, handling permission denied errors is code debt and also weird UX. 🙁

Why not instead of adapting CanAccessProject you add CanViewProject and use that for GQL queries and such?

@rmunn
Copy link
Contributor

rmunn commented Jun 20, 2024

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 CanViewProject.

@hahn-kev
Copy link
Collaborator

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.

@rmunn
Copy link
Contributor

rmunn commented Jun 20, 2024

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.

@rmunn
Copy link
Contributor

rmunn commented Jun 20, 2024

Implemented in commit 04ee8d1. Testing has exposed a bug in the projects listing code from #865, which is considering isConfidential == null as public. (In the test data, sena-3 is listed as isConfidential = null while elawa is listed as isConfidential = false). I'll work on fixing that bug tomorrow, but the first step is one I can do right now: make the test org own both sena-3 and elawa in SeedingData.cs, so that I can have both projects in the org list and verify that Test User (who's not a member of the org) can only see elawa but not sena-3. (Right now he can see sena-3, which will still throw an Unexpected Execution Error if he tries to click on it because he really shouldn't be accessing a non-public project.)

@rmunn
Copy link
Contributor

rmunn commented Jun 20, 2024

Not actually a bug, I just hadn't asked for isConfidential in the GQL query for the org's projects, so the filter was seeing isConfidential = null everywhere and hiding the projects. Commit ee1eb71 fixes that: it's a single-line commit, because all I actually had to do was request isConfidential and the filter started working properly. Nice to see that my OrgProjectsVisibilityMiddleware code was correct: it hid all projects if the confidentiality was unknown, then showed them if they were known to be public.

@rmunn
Copy link
Contributor

rmunn commented Jun 20, 2024

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.

@rmunn
Copy link
Contributor

rmunn commented Jun 26, 2024

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.

@rmunn rmunn closed this as completed in #888 Jul 4, 2024
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 a pull request may close this issue.

3 participants