1w ago

Are code reviews allowed? I wrote a pretty basic RBAC module while working on a separate learning project and was hoping to get the opinion of more experienced Gophers

github.com GitHub - AsqewLabs/rbacteria: Simple RBAC in Go

Simple RBAC in Go. Contribute to AsqewLabs/rbacteria development by creating an account on GitHub.

It appears to pass all tests, but I'm not sure if my test writing skills are necessarily that great, I semi-followed the learn go with tests eBook. I appreciate any feedback, and if this isn't an appropriate post for this community just let me know and I'll remove.

Thanks!!

*** Update *** Updated the repository to be a bit more discriptive of what the library is for and added in a small example project of it in action.

7 comments

I haven't written go in a little while, so things may have a changed since. I'll give this a go regardless.
First thing I notice is there's no explanation of what this is supposed to do. A little blurb in the readme would help. I have no idea what rbac is, so it would give me some context.
Then, there's no main function. Where's the entry point? This is a bit where I'm doubting myself now. Maybe go has changed, but when I was writing it, it requires a main function to even run.
I also notice that many of your functions and types start with a capital letter, making them public, but everything is in the same package. This is maybe nitpicky, but I would start with everything as private. As the project grows, and things get organised in packages, you open up the things you need.
With all that said, if it runs and does what you expect it to, I'd say that looks good!
- Since it has no main, I guess it's not an executable but meant as a library, which also explains the exports, as a third party should be able to consume the library
  
  That's a bingo! Yeah I decided to dip my toes into Go by writing a simple library on a topic I was learning about
- there’s no explanation of what this is supposed to do.
  Totally right, sorry about that, I'll update the Github, but it brief this is a library that's supposed to help a developer set up a Role Based Access Control system for an API for web service. Role Based Access Control is a method of access control whereby (And this is my very beginner's understanding of it) users are assigned roles, and these roles are in turn issued different permissions based off what that role is supposed to have access to. When checking if a user is authorized access to a certain resource, the roles assigned to them are checked for the permissions needed for the resource. If they have permission then they are granted access to the resource, otherwise they are denied access.
  This library manages roles, permissions assigned to roles, and checking of permissions against roles via an http middleware.
  Then, there’s no main function. Where’s the entry point? This is a bit where I’m doubting myself now. Maybe go has changed, but when I was writing it, it requires a main function to even run.
  Well, this is supposed to be a library that's used by other people, so it has no main function itself, rather it's called by other people

I haven't gone through it thoroughly but the first thing that stuck out was the use of a default logger. I don't think libraries should log by default. If you do allow a logger to be injected in I think it should be an interface.
- I don't think libraries should log by default
  That's a fair point, interfaces are still a concept that boggle my mind a bit, but maybe this is the problem that will help me actually grasp them. Thanks!

I personally try to avoid deeply nested if/else. Or else in general, because I think it makes code more readable. Especially when one of the branches is just an exit condition.

    
if exitCondition {
    return false
}
// long ass code execution

is way more readable than

    
if !exitCondition {
    // long ass code execution
} else {
   return false
}

In a loop, you can just return the value instead of passing it to a "retVal" variable.

With those in mind, you could refactor HasPermissions to

    
func (r *RBAC) HasPermission(assignedRoles []string, requiredPermission string, visited map[string]bool) bool {
    for _, assigned := range assignedRoles {
        if visited[assigned] {
            continue
        }
        role, ok := r.Roles[assigned]
        if !ok {
            //role does not exist, so skip it
            continue
        }
        for _, permission := range role.Permissions {
            if permission.String() == requiredPermission {
                //Permission has been found! Set permitted to true and bust out of the loop
                return true
            }
        }
        //check inherited roles
        if permitted := r.HasPermission(role.Inherits, requiredPermission, visited); permitted {
            return true
        }
    }
    return false
}

The same could be applied to LoadJSONFile and I think that really would approve the readability and maintainability of your code.

edit: This refactor is not tested

7 comments