Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

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

wants to merge 2 commits into from
+89 −61

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Sep 25, 2023 •
edited
Loading

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
GeorgianaElena reviewed Sep 25, 2023
View reviewed changes
Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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
Copy link
Member

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?

@consideRatio consideRatio marked this pull request as draft November 27, 2023 07:28
@consideRatio consideRatio mentioned this pull request Feb 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgianaElena GeorgianaElena GeorgianaElena left review comments

Assignees
No one assigned
Labels
maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CILogon] Rename allowed_idps to idps in a non-breaking way?
2 participants
@consideRatio @GeorgianaElena