AssertionRoleProvider Issues
TheHokieCoder opened this issue · 11 comments
The first, and major, issue that I have encountered is that the IsUserInRole() method in AssertionRoleProvider.cs does not seem to filter for the specified role name provided to the method. It appears to me that you can specify any role that you want, and as long as the current user is a member of any role, the method will return true. Oddly enough, this hasn't reared itself as a bug in my use cases when dealing with role membership. After doing some debugging and online research, I have come to learn that the .NET identity framework apparently uses GetRolesForUser() to get an initial cache of all roles for the current user. The only time when IsUserInRole() in AssertionRoleProvider would be called is if explicitly called with a different identity from the current user's identity. But that causes this specific implementation to throw a provider exception since that use case is not supported. Thus, the method is basically useless. See this StackOverflow question for an explanation of why the overridden method does not get called for the current user. Regardless, to be complete, I think our implementation should correctly indicate whether or not the user is in the role, even if it is not possible for that kind of invocation to occur.
The second issue is that in the private method GetCurrentUserRoles() does not check for existence of the key (roleAttribute) used for role membership values in the .Attributes dictionary. If that key does not exist in the dictionary, a KeyNotFoundException is thrown. I think it would be better to first test if the key exists in the dictionary (via ContainsKey() or TryGetValue()) before attempting to access the values so that an exception is never thrown.
The third issue is that I am not a fan of how the GetAllRoles() method is being repurposed to perform the same function as GetRolesForUser(). I'd rather have GetAllRoles() return a not implemented exception (since it is not possible for the assertion role provider to know all possible roles for the application) and move its logic to the GetRolesForUser() method. This way, someone familiar with the role membership portion of .NET identity management, but unfamiliar with DotNetCasClient, would not be confused by GetAllRoles() returning different values based upon the current identity context.
Lastly, I'd like to take the opportunity to add better comments to the entire class to make its documentation complete. I just first want to get some feedback/approval regarding the first two issues.
@TheHokieCoder what are your thoughts on a plan of attack on this one?
@phantomtypist I'd like to fix all 3 of the issues I outlined, and in the way that I described. I was just giving an opportunity to either a.) prove that I am wrong or b.) provide an alternate solution. If you are OK with the proposed solutions (in theory at least) would you want me to create a single branch to incorporate all changes, or would rather have separate branches for each sub-issue? To be honest I don't know Git/GitHub well enough to know if you can cherry-pick commits within a pull request (and therefore only need one branch) or if it'd be easier to approve with separate branches and pull requests for each sub-issue. Hopefully that all makes sense!
@TheHokieCoder did you end up making any code for this?
Yes I did, but again, I did it in a private clone of the project. I think it'd be best if I tackle each of the 3 issues as separate PRs, and I'll add commenting where appropriate in each. That way each fix can be addressed, approved, rejected, etc. on their own without holding up one of the others. Is that OK by you?
Sounds good to me!
I'm of the opinion doing things in smaller logical units is always a good idea. Makes you feel like you're getting somewhere quicker too!
I believe that I'll have some time tomorrow afternoon/evening to tackle this and other issues that I've promised to work on. I'll keep you updated.
Whenever, no rush.
@phantomtypist I have submitted PRs for the 3 problems addressed in this issue. I'll let you look them over and approve them if you are OK with the changes. Once they are merged into develop, I'll submit another PR where I will document the rest of the class.
@TheHokieCoder all merged. I'm going to close the issue. When you're adding the missing XML documentation comments just submit a PR.
Thanks!
👍