Open consideRatio opened 1 year ago
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:
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:
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!
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?
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.