[74542] Add an authentication strategy for the wikis module#23115
[74542] Add an authentication strategy for the wikis module#23115shiroginne wants to merge 9 commits intodevfrom
Conversation
- Add `BearerToken` auth strategy and all the code required by it - Updated `UserQuery` to use `BearerToken` auth strategy instead of just passing the token
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
NobodysNightmare
left a comment
NobodysNightmare
left a comment
There was a problem hiding this comment.
I'd suggest to extend the scope of this PR to all the queries (and callers of those queries) that we already built. Mostly, because this gives a more complete picture on the real requirements we have.
We already have services calling queries. And we already have working queries for the internal provider. In its current state, this PR wouldn't be able to work for internal provider queries, because there is no internal.authentication.user_bound strategy registered.
Trying to apply the current state of the PR to the existing services would've also shown that these now need to obtain token information that's:
- hard to obtain for a service (they would have to go to the access tokens table as well)
- not necessary for all queries
One additional note for internal.authentication.user_bound: Assuming we build authentication strategies to accept a user: So far the internal queries implicitly use User.current for authentication. I think once auth strategies are in place in makes sense to pass the user into the queries via the auth strategy. E.g. a WikiPage.visible could then become something like
auth_strategy.call do |user|
WikiPage.visible(user)
end| def extract_origin_user_id(token) | ||
| resolve("queries.user").call(Wikis::Adapters::Input::UserQuery.new(access_token: token.access_token)) | ||
| auth_strategy = Wikis::Adapters::Registry | ||
| .resolve("xwiki.authentication.user_bound") |
There was a problem hiding this comment.
🟡 This could also be self.resolve("authentication.user_bound"), which is the much more likely call pattern for a generic caller.
There was a problem hiding this comment.
Changed to the
def extract_origin_user_id(token)
auth_strategy = auth_strategy_for(token.user)
resolve("queries.user").call(auth_strategy:)
end
self here is not needed, and rubocop would cut it out 😄
| resolve("queries.user").call(Wikis::Adapters::Input::UserQuery.new(access_token: token.access_token)) | ||
| auth_strategy = Wikis::Adapters::Registry | ||
| .resolve("xwiki.authentication.user_bound") | ||
| .call(token.access_token) |
There was a problem hiding this comment.
🔴 This doesn't change a lot for callers yet. The caller still has to obtain an access token from a random database table first to use this authentication strategy.
Even worse: Imagine building code for a caller that doesn't know which provider is going to be called. Such a caller also wouldn't know which underlying strategy (here: Bearer token) would be used. So they couldn't possibly know that calling the strategy with a bearer token is the correct thing to do.
Every authentication strategy should be able to work from the same interface. For example one thing that could most likely work is to pass a user into the auth strategy and let the auth strategy figure out how to get to the data that it needs based on the user and the provider (the latter would by default be passed through a provider.resolve(...) call.
There was a problem hiding this comment.
Fair point. The user passed to the strategy is the way to go 👍🏻
to clean up the code
For internal queries
…ub.com/opf/openproject into implementation/74542-add-auth-strategy
Alright, let's do it. I've added some code and pushed it as well, but it still WIP. |
2 participants
Ticket
Follow up for the 74542
What are you trying to accomplish?
BearerTokenauth strategy and all the code required by itUserQueryto useBearerTokenauth strategy instead of just passing the tokenMerge checklist