Draft: Choose auth method including token exchange #17837
Conversation
| .with_user(user) | ||
| UserBound = ->(user:, storage:) do | ||
| # README: This check does not match for user that are only hijacked by an IDP based login. | ||
| token_exchange_preferred = storage.audience.present? && user.authentication_provider.present? |
There was a problem hiding this comment.
I'd call this sso_preferred, since the underlying strategy does not solely work with Token Exchange, but with all kinds of tokens obtained through SSO.
| else | ||
| # README: If for a user - storage combination no possible authentication method is found, the | ||
| # noop strategy will result in non authorized requests. | ||
| ::Storages::Peripherals::StorageInteraction::AuthenticationStrategies::Noop.strategy |
There was a problem hiding this comment.
Will this result in us making actual requests that will then be rejected?
IIRC we talked about how we should avoid sending out requests that we know will fail eventually.
Would it be better to cause an error/exception here that can be caught before a request goes out?
There was a problem hiding this comment.
That is a question we should answer together. I can understand both approaches.
| # In the case above, the token will be used without refresh, which will lead to an unauthorized response | ||
| # once the token is no longer valid. This can only be solved right now by logging out | ||
| # and in again - which will replace the account token itself. | ||
| # Open question: Do we want to support this case? |
There was a problem hiding this comment.
My opinion: If we want to keep our implementation simple here by enforcing that we know an expiration time, we should at least try to carry the expiration time communicated by the token endpoint along. This should give us an expiration time for the majority of OAuth 2.0 implementations.
If you agree I'd write a feature for that and would probably exchange with Wieland whether this limitation is acceptable.
There was a problem hiding this comment.
Not sure I understand, what is this feature proposal :D
There was a problem hiding this comment.
Let me back up: Right now we only know of an expiration time if we can parse the token as JWT, read it successfully and extract the exp claim. In all other situations, we don't know the expiration and would try to use the token forever.
You are suggesting here, that we never try to refresh the token, unless we know that it's expired.
My opinion: Right now, this is not good enough, because we would fail too often.
My suggestion: We should know the expiration time in more cases, to make your proposal feasible. That's the feature I want to write. Important remark is that we will not be able to cover 100% of cases according to the OAuth 2.0 specification. That's where I'd clarify with Wieland, that we are "good enough" from a product perspective.
There was a problem hiding this comment.
You have my support in raising this as a feature proposal ;)
| when :oauth_user_token | ||
| when :oauth_user_token || :sso_user_token | ||
| # README: So, this might lead to problems. How can we ensure, that at this point in time, | ||
| # the remote identity is already created for token exchange? | ||
| origin_user_id = RemoteIdentity.where(user_id: auth_strategy.user, oauth_client: storage.oauth_client) |
There was a problem hiding this comment.
Can't we "simply" turn this into a "find or create" style method for the SSO case?
I.e. if we find a remote identity, we take it. If we don't find one, we make an API call to figure it out and store it.
This means the first time that an SSO user needs a remote identity, they will fetch it via an API call. At all subsequent times, we will have it "cached" in our remote identities.
There was a problem hiding this comment.
From our IRL discussion: The current plan is that there might an an event triggered from the token exchange or from the request of a token for a certain audience.
Since those events are handled synchronously, there is no race condition that can happen here. Once we arrive at this point, the event should have created the RemoteIdentity.
Essentially, we can keep the code as-is.
| @@ -70,6 +70,10 @@ def authenticate_via_idp? | |||
| authentication_method == "oauth2_sso" | |||
| end | |||
|
|
|||
| def audience | |||
| nextcloud_audience | |||
There was a problem hiding this comment.
It's not yet too late to rename the nextcloud_audience property to audience, making this alias unnecessary.
There was a problem hiding this comment.
the method gives us another possibility: having control about which type of storage CAN have an audience.
I have no big pain in keeping it like this for now. But open for anything.
| ::Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken | ||
| .strategy | ||
| .with_user(user) | ||
| UserBound = ->(user:, storage:) do |
There was a problem hiding this comment.
Should we, instead of passing the whole storage, just sending its audience?
There was a problem hiding this comment.
You mean UserBound = ->(user:, audience:) { ... }?
I'd prefer the current way, as it seems to make more sense from a caller's perspective. You hand in a storage and a user to figure out how the user might authenticate.
Otherwise the caller would have to be concerned with how to get an audience from a storage and what that might mean. It also feels like the more brittle interface to offer.
There was a problem hiding this comment.
More akin to sso_supported.
The thing is, we spread the knowledge of a storage to a thing that has no business knowing that it even exists.
There was a problem hiding this comment.
@mereghost I think sending just the audience is something very specific. We might fall into the same pit we had for requests, that were build for specific nextcloud and then we had trouble in writing those for OneDrive.
My thoughts are coming from a yesterdays experiment, were Jan and me created an Azure SSO, and we were able to make server to server request with fully obfuscated audiences, it was a weird UUID.
So, point is, maybe we should have a better abstraction here. What is the entity, that holds the information about all configured authentication methods? right now, it is the storage object.
we spread the knowledge of a storage
true, but not here. this adapter is called nextcloud_strategies. it is bound to a storage type.
There was a problem hiding this comment.
I'd +1 what Eric said.
Also note that even accepting a parameter like sso_supported will also spread knowledge, because the caller of this thing, will suddenly have to know about SSO, to pick an auth strategy, when previously it just needed to know that an auth strategy is broadly related to a user and a storage.
There was a problem hiding this comment.
true, but not here. this adapter is called nextcloud_strategies. it is bound to a storage type.
I'd say that it has to know about the "authentication bits" of nextcloud, not the storage itself.
I get the feeling that we are missing an actual abstraction here. We started to draft this with the OAuthConfiguration but maybe that was the wrong name.
| ::Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken | ||
| .strategy | ||
| .with_user(user) | ||
| UserBound = ->(user:, storage:) do |
There was a problem hiding this comment.
Would if be worth making UserBound into a full blown object? 🤔
There was a problem hiding this comment.
How would it look like? Do you mind writing down in pseudo code some of the interface you have in mind?
There was a problem hiding this comment.
Not very different than it looks like now, but since it is getting smarter and smarter, the fact that it is a lambda kinda bothers me. We can even keep the signature.
Very::Good::Name::UserBound.call(user:, storage:)It would probably lead to better test-ability in the end too.
What are you trying to accomplish?
And now?
Please check the inline comments I prefixed with
#README. Those are the thoughts I had. Those should serve as a basis for discussion.