I still need to iterate on this a bit.

Hey @sven-bitwarden , this is a new pattern I’m trying out. Happy to get any feedback.

We’re obviously doing the same check in a lot of places, so I was thinking we could abstract it into a single validator.

Benefits:

1. Keeps the logic DRY
2. Handles both single-user and bulk-user org cases without requiring multiple async calls.

The validator should follow our new pattern of not throwing errors for business logic failure. Have a single public method that returns a value; then callers can decide how to handle that value. (e.g. v1 command can throw, v2 command can return it out as a result)

There are two methods in this validator: one for the old approach (v1 method: EnforceAsync), which throws an exception, and another for the new approach (v2 method: IsBlockedAsync), which just returns a bool.

I can change v2 to return the error message class if you'd like. One concern is that different actions have slightly different error messages. We could pass in an enum similar to how I'm handling it in v1.

I haven't put too much thought into v2 yet, so I'm open to ideas. I'd like to avoid doing multiple checks where the calling code has to inspect the result. Ideally, we would only need a single check.

In v1, we're effectively doing one check since the calling code handles the exception. With v2, we might end up needing a double check.

Although this is async, currentContext caches its results locally so you don't need a second layer of caching here. (I also generally think that this is an antipattern so would be pretty hesitant to extend this kind of behavior)

Can you clarify which pattern you’re referring to here: currentContext or local caching?

If you mean the local caching approach, could you elaborate a bit more on why you see it as an anti-pattern? I don’t think I fully understand the tradeoffs yet. The main concern I can think of is stale data if the cache is long-lived, but in this case I made sure it only lives for the duration of the request by registering the validator as AddScoped in DI.

I think having ICurrentContext as a request level object that does a bunch of things off the bat every request is the anti-pattern he's referring to. I think it would be more preferred if there were smaller more focused contexts depending on the request that is occurring. ICurrentContext does too much at this point. But using it is better than taking something out of CurrentContext to store in another spot.

ICurrentContext is populated every request via middleware. By creating another scoped entity, you're caching the cache, which is also an antipattern.

It sounds like we want to remove my cache layer and just use ICurrentContext's cache. I'm okay with this. My main goal with adding the cache was to move away from this pattern for bulk operations, so single and bulk validation can be more aligned.

Since it's already cached, should we just call await _currentContext.OrganizationCustom(organizationId); for each user moving forward?

You already would be at a certain point. It doesn't do a db lookup every time. Just once at the start and then is awaited everywhere else due to how the async/await works (because of lazy loading of provider data). See below

Honestly you could probably get away with using the CurrentContextOrg which is sync. That has access to Permissions and org user type.

Side Note:

    public Task<bool> OrganizationCustom(Guid orgId)
    {
        return Task.FromResult(Organizations?.Any(o => o.Id == orgId && o.Type == OrganizationUserType.Custom) ?? false);
    }

This shouldn't even be async.

Moving the discussion to this comment here so it’ll be easier to comment on.

I think this should be handled by the authorization layer

I’m not against this idea since it’s role-related. One concern is that the *AuthorizationHandler works great for single use cases, but how do we handle bulk actions? We’re running into the same problem as the ticket that checks whether orgUser belongs to the org during bulk actions.

Another concern is that AuthorizationHandler normally just returns pass or fail, while in our case, we return meaningful errors that are displayed to the user depending on the scenario. Changing this would be a breaking change.

edit: cc @eliykat