the-control-group / authx

An authentication & authorization platform for service-oriented architectures.
MIT License
47 stars 6 forks source link

Add ability to create a user and assign to roles in a single request. #38

Closed arnav-aggarwal closed 5 years ago

mike-marcacci commented 5 years ago

We talked briefly about the very common use-case of creating a user and adding her to a role in a single request. While #25 would make this possible, it still requires a dynamically constructed query since assignment to an arbitrary number of roles is required. All of us would prefer avoiding this kind of thing.

The original proposed solution here is to add a roleIds field to the createUser mutation. This appears to initially be the most straightforward solution to this problem, but also conflicts with the underlying architecture: a role hold the list of assignments, rather than a user. This has practical implications, since it's possible that someone creating a user:

  1. Can create the user
  2. Can assign the user to Role A
  3. CANNOT assign the user to Role B

Under this proposed solution, we lack good semantics for communicating the error on step 3, especially since we may want steps 1 and 2 to succeed.


I have an alternate proposal which depends on #25: perhaps we should update our mutations to accept arrays of input objects:

For example, instead of (or in addition to):

updateUser(id: ID!, enabled: Boolean, name: String): User

we might accept:

type UpdateUserInput {
  id: ID!
  enabled: Boolean
  name: String
}

updateUsers(users: [UpdateUserInput]): [User]

This would make it possible to pass in a list of instructions about which roles to modify without dynamically generated queries. It also allows us to report an error in place of the effected entity in the returned array.

@lolosett @arnav-aggarwal how do you guys feel about this?

PS: This is probably for a separate issue, but we may want to eventually consider adding some sort of semantics for all-or-nothing changes. Currently each mutation is done independently of the others. I don't think we yet have the need to justify adding transaction-style support, but it's something we might want to consider.

mike-marcacci commented 5 years ago

Copying discussion from internal slack:

@arnav-aggarwal

I think both proposals would work well for us! If we can pass in an array of roles to update, that would be simple to implement. The first proposal of passing in roles with the createUser mutation would be slightly easier, but I see the benefit of the second proposal for error handling.

@mike-marcacci

In that case, I’m going to implement the latter, because I also really like the idea of being able to do these things in batches: it would, for example, also let you assign an existing user to 5 roles in a single request.

@arnav-aggarwal

Yeah if that makes more sense for you, perfect. Stoked to refactor our queries when it’s ready so they’ll be neat and tidy!

Ah yeah, didn’t think about that last part, allowing us to implement batch role changes for existing users. Even better


I'm going to go ahead and assign this to myself, as I think the proposal is mostly fleshed out. The only remaining "design decision" here is whether to return "result wrappers" to hold any errors, or rely on the less-ergonomic (but universally understood) GraphQL error scheme.

lolosett commented 5 years ago

Although the first proposal seems like the easiest to implement, the second seems like the way to go.

As Mike mentioned, the second option is consistent with the current authx architecture. It also allows for more specific error handling which is obviously important to communicate/handle in the dashboard. This solution gracefully tackles the issue of assigning users to multiple roles upon user creation/update both in the client and in the back end.

As far as the result wrappers are concerned, I'd need to know a little more about the limitations of the GraphQL error scheme. Would you be able to shed some light on that?

mike-marcacci commented 5 years ago

@lolosett absolutely! The GraphQL spec (which is amazingly readable) has a section on error formatting with code examples that is much better than anything I could write here.

What's so great about this part of the spec is that it allows errors to cause the minimal amount of disruption, while still being completely predictable and keeping all type guarantees.

There is, however, a subset of the community who wants to differentiate between "program errors" and "user errors" and advises that the latter be accomplished directly in your schema, as opposed to using true GraphQL errors. So for example, I might have:


type CreateSomethingResult {
  errors: [String]
  user: Something
}

createSomething(input: CreateSomethingInput): CreateSomethingResult!

...colocating "user errors" in the result. I'm not sure I like this approach, though, since it requires the front end to check for errors in two places, and creates additional types.

In fact, writing out this little explanation has made me quite sure that I will just continue doing what I've done in the past: using GraphQL's built-in error mechanism for these things.