kobotoolbox / kpi

kpi is the (frontend) server for KoboToolbox. It includes an API for users to access data and manage their forms, question library, sharing settings, create reports, and export data.
https://www.kobotoolbox.org
GNU Affero General Public License v3.0
133 stars 179 forks source link

Allow setting SSO providers to "private" #4263

Closed david-code closed 1 year ago

david-code commented 1 year ago

Description

The goal of this feature will be to let SSO provider applications be set as private applications. This means the following:

I'll add a comment below outlining my proposed solution once I've had a chance to examine the code more closely.

david-code commented 1 year ago

Proposed Solution

david-code commented 1 year ago

@bufke could you look this over and let me know what you think?

bufke commented 1 year ago

I don't think I like SOCIALACCOUNT_PROVIDERS_openid_connect_SERVERS_0_private because it would be a KPI setting, not a SOCIALACCOUNT setting. Unless we're talking about extending the open_id provider class itself - then maybe it makes sense.

I certainly would prefer to modify SocialApp and never touch django settings ever.

It might make sense to extend SocialApp with a new model via a one-to-one relationship. In the future, that would allow us to add more information such as the oidc server URL (and would thus need to modify the provider as well). Another use case would be associating one social app to one organization to indicate that users of that organization are required to use the social auth provider. The disadvantage would be additional table joins, but joins are cheap. If this extension of SocialApp is not set, then I would expect the default allauth behavior to continue and the social app is considered public.

I think the existence of the Social App extension object would imply that it's private. An alternative would be a boolean to set public/private status. That would allow three states though (unset/true/false). Currently that isn't desirable, it would not be normalized to have unset and false effectively mean the same thing. I suppose a possible case for this would be a valid state where there is a public Social app that is also associated to a specific org. I'm kind of fishing for a use case use.

What do you think @david-code ?

david-code commented 1 year ago

@bufke Yes, that seems like a better solution. I agree with your reasoning for not wanting to put this info in the Django settings.

I think the existence of the Social App extension object would imply that it's private.

I agree with this, I'll implement it this way.

Thanks for the feedback!