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

Add data/authorization to office users endpoint #2242

Open
wants to merge 4 commits into
base: master
from

Conversation

4 participants
@garrettqmartin8
Copy link
Contributor

commented Jun 6, 2019

Description

As an System Admin
I want to query the office user list admin endpoint and receive all users
So I can verify who has office permissions

Reviewer Notes

I'd like some feedback on using the separate authorization.go file so we can manage authorization functions for different user types in one place.

Setup

  1. make server_run
  2. make client_run
  3. Log in as admin user via devlocal auth
  4. Navigate to http://adminlocal:3000/admin/v1/docs#/office/indexOfficeUsers
  5. Click Try it out
  6. Click Execute
  7. You should see a 200 response with a list of all office users
  8. If you log out or log in as a different user type and repeat steps 4-7, you will get a 401 Unauthorized.

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@garrettqmartin8 garrettqmartin8 force-pushed the gm-office-users-endpoint branch from 0cd898a to 0386b3d Jun 6, 2019

type officeUserListFetcher struct {
builder officeUserListQueryBuilder
builder officeUserListQueryBuilder
authFunction authorizeAdminUser

This comment has been minimized.

Copy link
@garrettqmartin8

garrettqmartin8 Jun 6, 2019

Author Contributor

I used this pattern based on @mikena-truss's advice. Injecting the authorization function as a dependency of the service allows us to easily mock its behavior in tests. It also allows us to change how we authorize a particular user type. For example, I'll soon be splitting up IsSuperuser into IsSystemAdmin and IsProgramAdmin. We will only have to change the code in one place when changes like that become requirements.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 7, 2019

Contributor

I like this pattern 👍 My open question to others is if we need to flavor this auth model with more information. Ex. a a program admin may want to see a "slice" of data that they own. Is there way to incorporate that? Is our model versatile enough to make those updates in the future?

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 7, 2019

Contributor

Also, the nice part of using an internal interface here is that changes to a single service don't force us to propagate changes everywhere.

This comment has been minimized.

Copy link
@garrettqmartin8

garrettqmartin8 Jun 10, 2019

Author Contributor

a program admin may want to see a "slice" of data that they own. Is there way to incorporate that? Is our model versatile enough to make those updates in the future?

@mikena-truss I think this may look pretty close to what you outlined in this gist. Appending a filter based on which type of admin is logged in seems pretty straightforward.

@mikena-truss

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Adding @chrisrcoles who might have some input here.

@mikena-truss mikena-truss requested a review from chrisgilmerproj Jun 7, 2019

@mikena-truss

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Also @chrisgilmerproj for security reasons :)

@mikena-truss
Copy link
Contributor

left a comment

Left a few comments for input. I think I'm too deep in the design to approve it though.

@@ -2,17 +2,20 @@

This comment has been minimized.

Copy link
@mikena-truss

This comment has been minimized.

Copy link
@garrettqmartin8

garrettqmartin8 Jun 10, 2019

Author Contributor

@mikena-truss I removed the directory and ran make mocks_generate, but the handler tests in pkg/handlers/adminapi fail without it. I'm probably missing something here...

@@ -28,7 +29,7 @@ func NewAdminAPIHandler(context handlers.HandlerContext) http.Handler {
adminAPI.OfficeIndexOfficeUsersHandler = IndexOfficeUsersHandler{
HandlerContext: context,
NewQueryFilter: query.NewQueryFilter,
OfficeUserListFetcher: user.NewOfficeUserListFetcher(queryBuilder),
OfficeUserListFetcher: user.NewOfficeUserListFetcher(queryBuilder, auth.AuthorizeAdminUser),

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 7, 2019

Contributor

I think we'll want NewAuthorizeAdminUser user here that returns the function. We've been trying to use constructors in anticipation of a dependency injection framework.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 7, 2019

Contributor

Do we really want authorization in the handler? Or would this be better to handle at a middleware level? I feel like passing it down to the fetcher puts it in the wrong layer of the stack. We probably want to stop unauthorized users well in advance of getting to the handler I imagine.

This approach might work if we had more nuanced access controls like roles or groups for admin users. But right now all we have is the superuser designation which is really an on-off for the entire API.

This comment has been minimized.

Copy link
@garrettqmartin8

garrettqmartin8 Jun 10, 2019

Author Contributor

@mikena-truss aren't we already injecting the authorization dependency here? Forgive me if I'm misunderstanding your comment.

@chrisgilmerproj that's a good question. As I understand it, we currently do most of our authorization in handlers. Do you think pushing these checks into the middleware level is something we should try to start adopting across the board? I also don't know what that would really look like in our codebase, so I'd love to pair a bit to get a better picture of what you're imagining.

The access controls are going to get a bit more granular as the admin features are built out - there will be a check for whether a user is a system admin or a program admin. Not sure that case really changes the situation you're commenting on.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 11, 2019

Contributor

You make a good point. But for the admin I feel like we could handle this authorization in the middleware so we don't have the possibility of opening up a security hole in the admin interface because we forgot to secure one of our many handlers.

Previously our session cookies wasn't secure enough to allow us to put authorization information inside of it. But now we have both secure and httpOnly set on our cookies so we could choose to put the authorization in that cookie and then check it in middleware.

I think we've been doing authorization like we would for access controls and I personally dislike it. But I also don't want to rock the boat too much. But we are trying a lot of new things with the admin API so this might be a good time to try it out.

This comment has been minimized.

Copy link
@garrettqmartin8

garrettqmartin8 Jun 11, 2019

Author Contributor

@chrisgilmerproj it looks like we already do some form of what you're talking about in the UserAuthMiddleware function, which I didn't know about:

if session.IsAdminApp() && !session.IsSuperuser {
    logger.Error("unauthorized user for admin.move.mil", zap.String("email", session.Email))
    http.Error(w, http.StatusText(401), http.StatusUnauthorized)
    return
}

That may remove the need for much of what I've done on this PR 😅

So the only real reason to do authorization in handlers/services would be to do access controls, like if I'm a program admin making a request to the /office_users endpoint. In that case, they would pass the middleware check but then the handler/service would return different data or throw an error based on an access control check.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 11, 2019

Contributor

Unless I'm missing something, using the middleware does not protect against hitting a handler you're "unauthorized" to view. It will always pass as long as you're 1) have a session with any app and 2) you are hitting the domain for that app.

For example, I could create a session with the my.move.mil app, then use my session to hit the admin handlers (it would just be my.move.mil/admin instead of admin.move.mil/admin. We've even done this in testing. Just log into prod then go to https://my.move.mil/admin/v1/office_users. You'll get a 200.

From what I can tell, the current middleware only does authentication and the "authorization" is really just a convenience layer for telling someone they're trying to use the wrong app. In fact, I think the way our app currently does this is confusing.

To use middleware, we'd have to decorate the handlers with a different authorization middleware that only passes for the admin app, not everything in this block: https://github.com/transcom/mymove/blob/master/pkg/auth/authentication/auth.go#L34-L53. This is ok to do, and reasonable if you want those handlers to be inaccessible to anyone other than system and program admins.

If there's some difference in access for admins because we created roles or groups or something then we'd probably have something in the handler.

Resource level authorization checks have to happen at the handler level or lower. The purpose of abstracting it from the session, is that we do want to do role based authorization at some point. System and program admins will have different levels of permissions. Doing this work now is setting the table for that eventual work, rather than retroactively refactoring all the admin authorization later on.

The reason it is inserted as a dependency in the service layer is to split the authorization checks from the code performing the service action. This allows us to look at authorization in a more declarative way, rather than sifting through multiple handlers/services to collect that information. I could be convinced that burying it in the service layer is fine. But it does have to happen on a per service basis, because they will eventually have different permissions than a blanket middleware would.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 11, 2019

Contributor

For your first points I think I'm confused as to how you get a 200 from hitting a handler if you're not a superuser and you're not in the admin app. My understanding is that the intention of the middleware as listed above is to stop any non-superusers using the admin app from proceeding down to the handler level. If that's not correct then we should fix it because its the behavior I expect.

Resource level authorization checks have to happen at the handler level or lower. The purpose of abstracting it from the session, is that we do want to do role based authorization at some point. System and program admins will have different levels of permissions. Doing this work now is setting the table for that eventual work, rather than retroactively refactoring all the admin authorization later on.

I agree with most of this. But this PR is using the is_superuser property of the user to define if they are authorized at the handler level, which I think is incorrect. If we do have different levels of permissions later then I think this PR is the correct way to implement them. My beef here is in using the is_superuser permission and not having actual roles/groups worked out prior to the implementation.

The reason it is inserted as a dependency in the service layer is to split the authorization checks from the code performing the service action. This allows us to look at authorization in a more declarative way, rather than sifting through multiple handlers/services to collect that information. I could be convinced that burying it in the service layer is fine. But it does have to happen on a per service basis, because they will eventually have different permissions than a blanket middleware would.

I don't mind the dependency injection, but again it relies on having a clear idea of what the authorization model will look like before implementation. I would prefer to see at least one permission type fleshed out here that is not the blanket is_superuser so that I can understand what is trying to be achieved.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 11, 2019

Contributor

For your first points I think I'm confused as to how you get a 200 from hitting a handler if you're not a superuser and you're not in the admin app. My understanding is that the intention of the middleware as listed above is to stop any non-superusers using the admin app from proceeding down to the handler level.

It's the same for any of our handlers. Admin isn't a special case.

I have a session with my.move.mil and am hitting this part of the auth code and passing: https://github.com/transcom/mymove/blob/master/pkg/auth/authentication/auth.go#L37

That app session value comes from here: https://github.com/transcom/mymove/blob/master/pkg/auth/cookie.go#L254. Other than this part of the code, the backend has no real concept of an "app".

Since we can hit any handler from any of our app domains, that effectively negates any of these checks. The only requirement is that you hit the same domain as your session says.

This is fine, since we're checking authorization in lower layers. But like I said, that essentially makes it a convenience check for the client, not an auth check.

If that's not correct then we should fix it because its the behavior I expect.

Agreed. It is the same thing I expected when I read the code the first time and had to spend time dissecting it to eventually find all this out. All our authorization relies on the handlers or models. I would be interested if someone could tell me this code had a separate purpose, but so far I haven't found one.

I agree with most of this. But this PR is using the is_superuser property of the user to define if they are authorized at the handler level, which I think is incorrect. If we do have different levels of permissions later then I think this PR is the correct way to implement them. My beef here is in using the is_superuser permission and not having actual roles/groups worked out prior to the implementation.

At a minimum, this work will be followed up by changing IsSuperUser to IsSystemAdmin and adding IsProgramAdmin. These will have different permissions.

Eventually, we'll have to both deny users based on those roles and slice data based on those roles.

I think the open question is what the return should look like and how the service should use it. But even without knowing that, it seems easier to me to change a function yielding the authorization information than to edit each service.

Session functions are relatively immutable. We can add to them, but removing/changing their underlying function has consequences.. If you use IsSuperUser everywhere, then decide it's not sufficient, you can't change that function because it has been propagated throughout the app. Putting it behind a typed function (or interface) that is independent of the session means it's now much easier to change. Just look at our current code and imagine changing everywhere that does IsOfficeUser user to a different type of authorization check.

It's possible we're trying to solve too many things at once. If we want to use session.IsSuperuser and punt on this, I think that's fine. But I would become wary if that became a regular pattern.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 11, 2019

Contributor

I agree with everything you've said here and I appreciate the extra context. I'd punt for now until you know what permissions you're going to have. My understanding of the intent of is_superuser is that it should only be used to explicitly enable you to pass through the middleware and for nothing else. So I'm really wary of building anything atop it right now.

I'd like to see a real permissions model added. It should probably be separate from the users table since we already know that in staging/experimental that only login_gov_uuid/email pairs are unique and emails alone are not. Given that we enabled is_superuser via emails I think we're at a place where we might potentially be giving the wrong users expanded permissions when we don't mean to. So continuing to build out permissions in the users table will likely bite us.

FWIW, I think the spirit of this PR and a lot of the code is in the right place.

This comment has been minimized.

Copy link
@chrisrcoles

chrisrcoles Jun 17, 2019

Contributor

Just catching up on this but I'd agree with some of the dissenting points that were brought up. I think we should be using role-based access controls. Ideally most checks would happen at the middleware level and then more granular access based controls would occur within the proper context - service object if possible, handler if necessary. Without knowing the needs of the future system, making sure to build authorization system in a way that is flexible is very important; roles and permissions allow us to do that. If we are going to introduce a new way of handling authorization, I'd like us to make sure we build the system we want as opposed to the system we can deal with.

// Here is where NewQueryFilter will be used to create Filters from the 'filter' query param
queryFilters := []services.QueryFilter{
h.NewQueryFilter("id", "=", "d874d002-5582-4a91-97d3-786e8f66c763"),
// h.NewQueryFilter("id", "=", "d874d002-5582-4a91-97d3-786e8f66c763"),

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 7, 2019

Contributor

I think this can even be deleted at this point. Leftover from manual testing.

type officeUserListFetcher struct {
builder officeUserListQueryBuilder
builder officeUserListQueryBuilder
authFunction authorizeAdminUser

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 7, 2019

Contributor

I like this pattern 👍 My open question to others is if we need to flavor this auth model with more information. Ex. a a program admin may want to see a "slice" of data that they own. Is there way to incorporate that? Is our model versatile enough to make those updates in the future?

type officeUserListFetcher struct {
builder officeUserListQueryBuilder
builder officeUserListQueryBuilder
authFunction authorizeAdminUser

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 7, 2019

Contributor

Also, the nice part of using an internal interface here is that changes to a single service don't force us to propagate changes everywhere.

@@ -0,0 +1,11 @@
package auth

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 7, 2019

Contributor

Not positive, but directory structure may need to change here.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 7, 2019

Contributor

I really dislike how pkg/auth/ and pkg/auth/authorization/ is laid out since they mix authorization and authentication logic along with middleware and other things.

However, couldn't you use the session.IsAdminUser() function from session.go for this?

This comment has been minimized.

Copy link
@garrettqmartin8

garrettqmartin8 Jun 10, 2019

Author Contributor

Should I move authorization into its own package instead of living in the auth package?

However, couldn't you use the session.IsAdminUser() function from session.go for this?

@chrisgilmerproj do you mean to replace the call to session.IsSuperuser below? Or to replace this entire function?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 11, 2019

Contributor

Yeah, I think you could remove the whole function. I believe you're looking at the same data in the session in any case.


func AuthorizeAdminUser(session *Session) error {
if !session.IsSuperuser {
return errors.New("USER_UNAUTHORIZED")

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 7, 2019

Contributor

If you do end up wanting this could you make a new error type specific to this kind of superuser admin check?

This comment has been minimized.

Copy link
@garrettqmartin8

garrettqmartin8 Jun 10, 2019

Author Contributor

Do you mean to create an actual type definition? Would you want to return a different status code?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 11, 2019

Contributor

Yeah, I do mean create a new type definition. Or use one of the pre-build 401 error types we have defined in models.

err := o.authFunction(session)
if err != nil {
return nil, err
}

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 7, 2019

Contributor

I guess I'd re-iterate here that this feels like the wrong place to do auth for superusers but instead it might be ok for roles/groups. And having to build this into all the queries now seems like overreach if we really don't have a more nuanced admin model yet.

@chrisgilmerproj
Copy link
Contributor

left a comment

I'd love to understand the authorization story better before this moves forward. I don't really want to block but more like I'd like to be on the same page about what the intent is.

@garrettqmartin8 garrettqmartin8 force-pushed the gm-office-users-endpoint branch 2 times, most recently from 7f216ed to 6ed66b0 Jun 11, 2019

@garrettqmartin8

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@mikena-truss @chrisgilmerproj for posterity, here's what we decided on in a call yesterday:

  1. This PR should try to push admin authorization into the middleware layer for now.
  2. This PR should also add an admin_users table that will contain program/system admin permissions.

Please correct me if I'm wrong 😄

@garrettqmartin8 garrettqmartin8 force-pushed the gm-office-users-endpoint branch 2 times, most recently from 8a6af7b to 4c5bc9d Jun 13, 2019

garrettqmartin8 added some commits Jun 5, 2019

@garrettqmartin8 garrettqmartin8 force-pushed the gm-office-users-endpoint branch from 4c5bc9d to 4d58c69 Jun 13, 2019

@donaldthai donaldthai removed their request for review Jun 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.