Multipass group name in URL

[tomako]

Hi @ThiefMaster ,
I'm here again with my issue with the multipass group name. The group name is not unique on the identity provider (keycloak) side. The groups have a hierarchy, so the same group name can appear on different places in the structure. So the group name itself is not unique although I can use their path as unique group name. In this case the name looks like this "/Root/Group1/Subgroup1".
Here comes my problem. Using this as a group name does not work because the name is used directly in URL without escaping and the slashes cause issues.
What can I do?
a) We should quote/unquote the group name to make this work (require code change in Indico).
b) We could choose a different separator which is URL-friendly. For example back-slash. The name would look like this "\Root\Group1\Subgroup1". Hmm, maybe... (it can be manageable in our identity provider)
Other suggestion?

[ThiefMaster]

Wouldn't it be easier to requier unique group name on KC? Allowing non-unique group names sounds like a complete mess TBH.

In any case, how about using a separator that's not a (back)slash? --, . or even better if you can find something that's not valid in a group name (not sure what KC allows or if you can restrict what a group name can contain). In principle backslashes should also work but they are incredibly ugly in URLs...

---

Technically it would mostly work if we used the path url converter in these two endpoints to allow forward slashes:

_bp.add_url_rule('/<provider>/<group_id>/', 'group_details', RHGroupDetails)
_bp.add_url_rule('/<provider>/<group_id>/members', 'group_members', RHGroupMembers)

But it'd be problematic if you used a group named something/members... so best to avoid that!

[tomako]

I have no control over what users put into the name of a group. It's a free text field and there can be anything even reserved characters. So I think the correct way would be quote/unquote instead of trying to find some tricky workaround.
Actually this is a general problem regardless my initial issue. This is clearly user input and can't be safe to use it in URL without sanitizing.
It would mean 2 changes:

• in _MultipassGroupProxy.locator
return {'provider': self.provider, 'group_id': quote_plus(self.name)}
• in RHGroupBase._process_args
group = GroupProxy(unquote_plus(request.view_args['group_id']), request.view_args['provider'])

[ThiefMaster]

IIRC. quoting slashes in URLs is generally prone to breaking. Reverse proxies etc. like to unquote slashes.

Does Keycloak seriously not have an option to restrict what can be put in a group name? Or information on what's allowed in a group name (I'd find "anything" very odd)... :o

In any case, you could try to work around this in your multipass plugin by converting the group name to something not containing slashes...

[ThiefMaster]

OK, after having a look how groups work on Keycloak it seems like the problem is that internally keycloak identifies groups by their ID, and the "name" is really just meant for humans... So of course they have no reason at all to limit what can be in a group name.

I see two options here:

1. implementing support for a separate group name (identifier, in this case the UUID from keycloak) and title (the human-facing one) like you proposed in Multipass Group with ID #6235 - note that this would NOT be something that can be implemented only in Indico, it would need to be implemented in flask-multipass as well (ideally in a way that's fully backwards compatible)
2. doing a workaround in your integration between flask-multipass and keycloak - basically generating a name (that's stable and never changes, since it will be stored in the DB when adding the group to an ACL!) and returning that via the Group object to Indico. This may require connecting to a database from the multipass plugin so you can persistently store that mapping.

I think (2) is quite hacky, but if this keycloak integration is an internal thing only you use, you could expose the API in the un plugin and then import it from your multipass-"plugin" in order to use the database session/transaction from indico.

If you go for (1) - which would, on the long term, be the cleaner option - then it'd be important to avoid slowdowns loading e.g. a protection page in indico even if there are let's say 10 groups in the ACL (because I have the feeling you'd need to make one API request to keycloak per group to fetch its title - not sure if KC has an API to query such things in bulk). Caching those titles for a while, similar to what we do for membership check results, would be one possibility.

---

Another possibility, in case you do not need nesting groups: Limit which groups can be used in Indico to top-level groups or groups that are within a certain group (e.g. within a group named "Indico Groups" ). Then the only problem you'll have is with groups containing a / but maybe documenting that this is not allowed would be fine (and filtering out such groups in the multipass plugin so they cannot be added to ACLs etc either)? Renaming groups would of course also be a no-go...

[tomako]

As I see you don't really prefer quote/unquote :) Because of the possible side effects on reverse proxies? We could use different encoding, it would affect only the URL. Or is there other reason?

[ThiefMaster]

"different encoding" would be kind of what I suggested as option 2 before. Since those groups names are really completely unrestricted, any char you somehow use for encoding could be mis-used. And it would probably look bad for the user since name/title are not separate...

And it's not just the URL. Any change in group structure would also become problematic later on, because the group name that gets stored would need to include this. So unfortunately the only truly clean options are either limiting yourself to a flat group structure (easiest option) or really adding proper support (option 1 from my last comment) for separating group names and titles everywhere.

[tomako]

Relying on group names (in general on names) is a fragile solution anyway. It doesn't really matter if that's a simple renaming or moving the group within the hierarchy (when we store the full path to ensure uniqueness). Adding group ID taking from the IdentityProvider I think the way. In fact it means more changes here and there as you mentioned.

What I suggested as a "quick fix" or improvement for the current solution to avoid unwanted characters in URL. And it would affect only URLs because it wouldn't change how the group name is stored. Check the 2 lines I mentioned. I would use the encoded group name in the locator for generating URL and the other one is decoding it in the group request handler. The encoding could be something else (eg. base64) if the simple quote considered error prone. So that is only about how the group name is represented in the URL nothing else.

[ThiefMaster]

TBH as a quickfix I'd just use something like --- as the separator. Unlikely to be used in a group name, does not require changes to the core, and not too ugly since users will see it in the ACLs as well

[tomako]

Eventually we are transforming the group names using the full path like this: "\Root\Group1\Subgroup1" -> "Root > Group1 > Subgroup1".
This is the current workaround until we find some resources to implement Multipass Group with ID in a proper way.