We’re rewarding the question askers & reputations are being recalculated! Read more.
10

There are a couple of things that I would change in your code to make it clearer or mix well with standards. internal static bool IsRunAsAdmin() { var Principle = new WindowsPrincipal(WindowsIdentity.GetCurrent()); return Principle.IsInRole(WindowsBuiltInRole.Administrator); } I would change the name of this Method to ...


10

So, thing is, it's GREAT that you made the individual tests. Imagine this method with all those tests inlined. That'd be... really bad. So that part is good. But like you say, something is off. Now, one way you could look at this is that you don't really need to test everything. That depends on what an implementation means, but someone has read rights if ...


8

public delegate IEnumerable<string> GetUserPermissions(IPrincipal user, object resource); public delegate IEnumerable<string> GetUserRoles(IPrincipal user, object resource); public delegate bool IsUserInRole(IPrincipal user, object resource); I seldom create new delegate types, I tend to use Action and Func delegates instead, but I like how it ...


8

I'd replace the conditions with guard clauses. (Flattening Arrow Code) function has_access() { if ( !is_user_logged_in() ) { return false; } $role = get_current_user_role(); $admins = array( 'Administrator', 'Agent', 'Contributor', ); if (in_array($role, $admins)) { return true; } ...


8

It's not an error to ask for help When -h is passed, the help() function shouldn't exit with non-zero status, because it's done what was asked for. Conversely, when we pass no arguments, we should exit with non-zero, and we should write the message to the standard error stream. I'd write that as help() { local tab=$'\t' cat <<END $0 - run ...


6

I'll take a stab at answering your questions and provide some suggestions: If you have FormsAuthentication configured in web.config, it will automatically pull the cookie for you, so you shouldn't have to do any manual population of the FormsIdentity. This is pretty easy to test in any case. You probably want to override both AuthorizeCore and ...


6

I am understanding that your goal is to return a set of data that a user is authorized to see. I further understand that ChannelUsers and WorkGroupUsers are only used to validate the authorization. (I see you do select ChannelUsers.user_id but I don't think you really need it - you can take it from Users.id instead, I believe). Given that, I believe you ...


5

There is something wrong with this Method that makes it a bit confusing. protected override bool AuthorizeCore(HttpContextBase httpContext) { if(httpContext.User.Identity.IsAuthenticated) { if(string.IsNullOrEmpty(Roles)) { return true; } else { var rolesOfUser = System.Web.Security.Roles....


5

No, this design is not object-oriented, nor would I recommend you follow your friend's advice. I also would not recommend your original design either. Neither one adheres very well to object-oriented design principles, namely the SOLID principles. In your friend's rendition, he is proposing a structure that is very active record-like, where a domain object ...


5

It would be slightly more readable to use early returns, with so-called guard statements, for example: protected void createUser_Click(object sender, EventArgs e) { var createStatus = new MembershipCreateStatus(); if (!IsBarNumberAndEmailValid(email.Text, barNumber.Text)) { userCreationResults.Text = "Your Email and/or Bar Number doesn'...


4

Normally I would say that method names should be Pascal Case, meaning that createUser_Click should be CreateUser_Click and the Comment for adding the user to the attorney role in the MembershipCreateStatus.Success case should be deleted, or it should be a valid comment like //TODO: Add User to Attorney Role


4

I'm no C# expert, but because each case is so similar, wouldn't it make more sense instead to store MembershipCreateStatus codes and resulting text? Then one could use a nice, fast data structure such as a tree to generate the appropriate text from the given status.


4

As far as I can see, createStatus is only used as an out parameter in MemberShip.createUser(). You don't need to explicitly create a new instance for this so I wouldn't do it either. I prefer to leave this to the method itself since it has to take care of that anyway. This would result in the following code: MembershipCreateStatus createStatus; var newUser =...


4

I think it's too nested. A switch inside an if block should leave you with a funny smell and an urge to pull things out of there. Also the method seems to me like it's doing too many things - I'd extract that switch block to some SetCreationResultText method, and then look into ways to get rid of the switch altogether.


4

Decoupling If we extract the composition of the body to a separate method private String ComposeEmailBody(String userName, String userEmail) { String bodyTemplate = ReadBodyTemplate(Request.PhysicalApplicationPath, "ActivationTemplate.txt"); bodyTemplate = bodyTemplate.Replace("<%UserName%>", userName); return bodyTemplate.Replace("&...


4

I think there is a problem with encapsulation and the single responsibility principle here. The method depends on too many external factors, it makes too many external references, and it includes logic of multiple independent responsibilities that would make sense to extract elsewhere. These are good and belong to this method: Creating and filling a ...


4

You shouldn't place this inside the button click eventhandler, but to a separate method. Assume you want to create a user at some other place, what will you do? Refactoring so the code in question is placed in a separate method or misusing the eventhandler ? First I would add a method which returns a String based on a MembershipCreateStatus enum value. ...


4

There are two primary concerns I have. First, rather than accessing [[UserManager sharedManager]isUserLoggedIn] twice, let's access it just once: BOOL loggedIn = [[UserManager sharedManager] isUserLoggedIn]; Now just use the loggedIn variable in place of calling these methods. In this case, it's not going to make a huge difference, but it's a good ...


4

From: http://api.jquery.com/jquery.ajax/ Deprecation Notice: The jqXHR.success(), jqXHR.error(), and jqXHR.complete() callbacks are deprecated as of jQuery 1.8. To prepare your code for their eventual removal, use jqXHR.done(), jqXHR.fail(), and jqXHR.always() instead. So, it looks like you should do: $.ajax({ url: "/submit/checkuser" }) .done(function(...


4

There's no reason (as far as I can tell) to do string comparison to check a class. #is_a? would presumably work fine too. And it'd be stricter, which is desirable. There's no reason to check students.size. If there are no students, the loop just won't do anything, so why bother checking first? Conversely, if there are any students you're actually doing 2 ...


4

Security You are very likely open to SQL injection. You should never directly put variables into queries, and you really need to use prepared statements. You are also likely open to persistent XSS, which may or may not matter, depending on your application. Formatting Your lines are up to 273 characters long, which is way too much. You should aim for 80 ...


4

Default class access modifier Unless otherwise specified, classes in C# are internal by default. This means that you can remove the internal access modifier in places like these: internal class PermissionManager internal class Permission internal class PermissionCommand However, if you like explicitly specifying the internal access modifier, then you can ...


4

IsInRole() I don't like how the method parameters are validated. IMO the null-propagation operator shouldn't be used for everything because it reduces the readability. Having a null-propagation operator for "safely" "accessing" a property is OK if the object is the first on the right hand of an assignment like in checking for var ident = principal?.Identity;...


3

Couple of items that might need a re-look: The now deprecated AzMan has very similar concepts. The conceptual equivalent for the 'Feature' in your application is an 'Operation' in AzMan. I'd suggest taking a look at it to understand how an application can use role based access to get the granularity that is desired. A Custom Role Provider can help you ...


3

This is really pointless: if (in_array($role, $admins)) { return true; } else { return false; } when you can just call return in_array($role, $admins); So: function has_access() { if (!is_user_logged_in()) { return false; } $role = get_current_user_role(); $admins = array( 'Administrator', 'Agent', ...


3

I really think that you should merge all of these if statements, make them an if/elseif/else statement. just move all the variable declarations to the top. then it turns into this [HttpPost] [ValidateAntiForgeryToken] public ActionResult Authorize(Administrator a) { var admin = _db.Administrators.Where(x => x.Username == a.Username).SingleOrDefault(...


3

By using some early returns, you could eliminate some nesting, and in my opinion, make the code more readable. In addition, I would ditch the ActiveUnAuthorizedUrl property and make it a function that returns a usable URL. Since "unauthorized" is a single English word, the "A" should not be capitalized. Prefer "unauthorized" to "unauthorization", as the ...


3

I found the solution somewhere from the documentation. I don't know why I had not seen that before. Symfony2 provides an easier way of doing what I was looking for: $builder = new MaskBuilder(); $mask = $builder->add($participant->getRight())->get();


3

This is completely unnecessarily complicated: needed_permissions_list = list(filter(None, needed_permission.split("."))) This is equivalent: needed_permissions_list = needed_permission.split(".") The return True at the end of the if item != item2 is unreachable, so you can delete that line. item2 is not a good name. needed_item would be better, so you ...


3

You should never use variable names like newUser. This is too easy to confuse with new User. While it is tempting to use such things, and it is the same reason the language designers chose to use new, I much prefer names like: User userToCreate = new User(...).


Only top voted, non community-wiki answers of a minimum length are eligible