Conversation
|
|
||
| The current `satellite/console/consoleauth/sso/service.go` implementation already supports multiple providers through | ||
| the `providerOidcSetup` map, but doesn't distinguish between provider types. The most straightforward approach is to | ||
| prefix the general provider names with a type identifier; e.g., `general-google` for Google SSO. This allows easy listing |
There was a problem hiding this comment.
nit - mostly sharing some thoughts, but I am not completely sure about the best approach. I just wanted to write down an alternative
Your current design branches "enterprise" vs. "general" behavior based on if the general prefix is included in the "provider key". Technically, I think that is okay from a functionality perspective, but it also feels risky from a configuration/maintenance perspective. It is not obvious to someone reading the satellite config, that there is a functionality difference of providers passed into the same STORJ_SSO_OIDC_PROVIDER_INFOS config, exclusively because of the existence of a specific prefix.
An alternative (again, we don't necessarily have to do this depending on how you feel about it):
- split into two configs: e.g.
sso.oidc-providers.generalandsso.oidc-providers.enterprise. Deprecate existing config and remove once configs are migrated to the new ones - don't worry about enforcing the provider key across the two configs - if there happens to be a duplicate, we can either (1) use enterprise config, or (2) return error on startup due to conflict
- we can still call the general provider key
general-google, for example, but in this alternative approach, the code isn't branching because of the existence of thegeneral-prefix. The code is branching because of the configuration.
- we can still call the general provider key
There was a problem hiding this comment.
I'm thinking adding new configurations will be extra overhead for maintenace because we already have so many configs already. If we can properly document what distinguishes general sso providers we can put them all in the same config. (we could add a new property General bool too.
I don't feel strongly about this though so if you still disagree, I can update the doc.
| ## Open Questions | ||
| * Should we allow users to unlink their SSO accounts and revert to email/password only? | ||
| * Should we allow general SSO users to change their email? | ||
| * Should we store state tokens in the db instead of cookies? |
There was a problem hiding this comment.
can you elaborate on what "state tokens" are and what they are usually used for in other apps?
Is it a special cookie coming from the ID provider?
There was a problem hiding this comment.
the state token is a signed string we add to the request to being the SSO flow to the provider. The provider will send back this token when it calls back our server so we can validate that the auth request is from us.
|
Some notes after design sync today
One additional thought I had: |
2 participants
Closes https://github.com/storj/storj-private/issues/1311