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

#1196 Add Criteria property to IAuthorizationContext #1203

Conversation

ajj7060
Copy link
Contributor

@ajj7060 ajj7060 commented Jul 10, 2019

Resolves #1196, which adds a Criteria property to IAuthorizationContext, and updates the data portal to supply it for per-type rules.

@rockfordlhotka rockfordlhotka self-requested a review July 10, 2019 23:58
@rockfordlhotka
Copy link
Member

@ajj7060 this looks good, but I have a fairly major concern. Specifically, a lot of the data portal code is changed by the enhancements in PR #1163

One of the changes is that "criteria" is no longer necessarily a single value. It could be a list of values.

I really hate to ask this, but would you consider applying your changes to the https://github.com/rockfordlhotka/csla/tree/1102-dataportal branch instead of master?

If you look at the updated code (especially DataPortalT) you'll see that the flow is similar, but not identical. I'm sure it is different enough that auto-merge will be impossible, and re-implementation would be required.

However, looking through your changes in this PR I really don't think it'll be too hard to apply your changes (in concept) to my working branch.

The one thing that might be a little tricky is that "criteria" really does flow through DataPortalT as type object, but before it is used it is always converted into a concrete value based on a set of rules such that it can be passed into methods as a parameter array (params keyword).

As an improvement over the old code, those rules are now codified into a couple helper methods (GetCriteriaFromArray and GetCriteriaArray):

https://github.com/rockfordlhotka/csla/blob/1102-dataportal/Source/Csla.Shared/DataPortalT.cs#L73

@ajj7060
Copy link
Contributor Author

ajj7060 commented Jul 11, 2019

@rockfordlhotka Ah, I didn't consider those changes when I did this. I'll give it a shot. I should be able to commit directly to that branch, correct? Or is there some other git magic I need to learn to incorporate things? 😄

Also, since there can be more than one criteria param, do you think the IAuthoriztionContext.Criteria property should be an object[]? I assume that's the only way to go, unless there's something in that branch to use instead?

I probably won't get a chance to dig in until this weekend. Should I leave this PR alone or close it?

@ajj7060 ajj7060 closed this Jul 14, 2019
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide criteria to per-type authorization rules
2 participants