[CILogon] Rename allowed_idps to idps (deprecation, not removal)

[consideRatio]

• Fixes [CILogon] Rename allowed_idps to idps in a non-breaking way? #683

Note that the tests passes for the first commit that runs tests against the old name, but also for the last commit that runs tests against the new name. Like this, I feel quite confident we haven't messed up our deprecation logic, and that makes me confident enough to suggest this name change.

[GeorgianaElena]

Personally, I believe the original allowed_idps makes more sense than idps.

Configuring idps is about describing what users we can authenticate/recognize, while allowed refers to what users we authorize/allow.

I believe that by configuring the idps dict (i.e. allowed_idps) we are not describing what users we can authenticate, but what users we authorize.

By default, CILogon allows users to authenticate using any idp from the entire list they support at https://cilogon.org/idplist/. If you go to https://cilogon.org/ you are presented with the entire list of idps you can use to authenticate.

But by configuring the allowed_ips dict, we are actually choosing to authorize only the users that use a specific idp to login. And we do this here:

oauthenticator/oauthenticator/cilogon.py

Lines 32 to 34 in 4bad3e8

	# selected_idp should be a comma separated string
	allowed_idps = ",".join(self.authenticator.allowed_idps.keys())
	extra_params["selected_idp"] = allowed_idps

where we choose to only show the allowed_idps as authentication options.

And then at the lines below if any other idp is used

oauthenticator/oauthenticator/cilogon.py

Lines 302 to 305 in 4bad3e8

	if not self.allowed_idps.get(user_idp):
	message = f"Login with identity provider {user_idp} is not pre-configured"
	self.log.error(message)
	raise web.HTTPError(403, message)

.

[consideRatio]

Ah think I understand what you are saying!

I agree that relating to CILogons default, we are saying what subset of idps we can use to authenticate users with - and that is a denial of authornization. I was using a narrower meaning of authorization, thinking about granting or not granting access to an already authenticated user.

---

I agree in how it can make sense to call this config allowed_idps, and there is some value to describing that only some idps can be used, but I'd like to champion use of idps further as I think the naming allowed_idps have too large drawbacks:

• Reading allowed_idps next to other config involving alllow, it seems ambgious what it does. I think the other allow config hints that it could mean the first thing below, but in practice it means the latter:
  • "allow all users authenticated via this IdP"
  • "only these idp among all cilogons supported idps are allowed to authenticate users"
• The allowed_idps behavior is different from allowed_groups in the sense that its sufficient to be part of a group in allowed_groups, but its not sufficient to be a user authenticated with an allowed_idps - the latter is just one initial requirement.

---

Maybe we could have a voice chat about this? I've found myself struggling to effectively discuss various topics by writing comments, and as this comment took me ~45-60 minutes to write it could be very helpful for me to transition to a voice chat!

[GeorgianaElena]

Maybe we could have a voice chat about this? I've found myself struggling to effectively discuss various topics by writing comments, and as this comment took me ~45-60 minutes to write it could be very helpful for me to transition to a voice chat!

Sorry for missing this message @consideRatio :( !Let me share my thoughts super quickly below and let's sync to have a chat if we still don't manage to converge.

Thank you for detailing why you are advocating to remove the allowed prefix from the name, given the existing context of other allow prefixed config. I understand and agree with you that it might cause confusion about what's its assumed meaning given the context vs what it actually does which is to only permit or to configure a list of supported IDPs for login.

In this spirit, what do you think about renaming it to permitted_idps or supported_idps, or use other prefix that implies what it does? In this way the assumed understanding about what this config does is more close to the truth, without being influenced by the assumed allow prefix meaning?