[Google, Globus, Bitbucket] Ensure auth_state is JSON serializable (lists are, not sets)

[consideRatio]

This PR adds a test to ensure the auth_state is set to a serializable object, because if it isn't, then use of c.Authenticator.enable_auth_state = True will cause JupyterHub to error with a broken assumption.

This PR doesn't resolve the situation. The situation is caused by a change for various group related fields from list to set in the 16.0.0 release. The change was meant to reduce back and forth casting and confusion stemming from config being Set based but info about groups/teams ended up list based.

Fix implementation brainstorming

I think there are various strategies to consider to resolve this caused by putting set objects under auth_state.

1. To always cast back and forth between set/list when writing/reading from auth_state
2. To change config of Set type to List type if that relates to avoiding all set based auth_state without needing to cast back and forth
3. To not put set based content under auth_state, but in auth_model (then it won't be persisted)

I'm leaning on going for 1 or 2. To evaluate if 2 is an alternative, we should know if the traitlet based List type can accept being passed at set also. Probably towards 1?

Action points - help wanted

I'm on vacation so if someone wants to resolve this by completing this PR by pushing commits or opening another one, please go for it! Make a comment that you are in case I'll end up wanting to work this during my vacation.

• [x] Fix failing tests for BitBucket
• [x] Fix failing tests for Globus
• [x] Fix failing tests for Google

closes #667

[minrk]

Looking at this now. I think we should go with 1.

[minrk]

To evaluate if 2 is an alternative, we should know if the traitlet based List type can accept being passed at set also. Probably towards 1?

Yes, set traits accept lists as input and cast them. But I think it's still the right thing to do for the config to be a set.

I think casting to list to/from auth_state is a simple and straightforward change. The other simplest change would be to leave it a list everywhere use the set config as the left hand side and pass the list, because set.intersection accepts lists (any iterable, in fact):

In [2]: {'a', 'b'}.intersection(['b', 'c', 'd'])
Out[2]: {'b'}

I've implemented 1. and fixed some logic in the computation of checking for intersections, because:

if any(set_a & set_b):

is not the same as

if set_a & set_b:

which is what we want. These checks are meant to look for a non-empty intersection (i.e. truthy set, aka set with length > 0), whereas any(set_a & set_b) checks if any item in the intersection is truthy. This isn't a serious bug because the only falsy string ('') doesn't make sense as a group name, but the principle of the logic is not right.

The only situation where the answers would differ is a set containing only the empty string, i.e.:

In [3]: any({'', 'a'} & {''})
Out[3]: False

In [4]: bool({'', 'a'} & {''})
Out[4]: True

which can't really come up in practice.

[minrk]

I removed 'breaking' because I don't see any breaking changes here. Only a fix to allow auth_state to work. The type of an auth_state field is changed, but that's not an API to be consumed, and auth_state doesn't work at all until this PR because it's not persisted.

[consideRatio]

Thank you @minrk for working this!!!

I think its breaking by reverting a very small breaking change in 16.0.0. I consider it breaking as one can have have logic in a post_auth_hook that make an assumption about the type we stored the group/team as. I don't suggest a new major release, but that we merge this and make a notice in the 16.0.0 changelog that we reverted the change of making the auth_state store groups/teams etc as sets. Apparently that is something we only wrote about for Google, not also Globus and BitBucket.

[minrk]

I think that's fair, but I also think there shouldn't be an expectation of using auth_state when it doesn't work with auth_state enabled. I think "auth_state didn't work until now" invalidates expectations of the structure of auth_state while it didn't work.

[consideRatio]

Having reviewed Mins work on this, I'll go for a merge!

[ryanlovett]

Thanks @minrk and @consideRatio !