Repository navigation
[SE-664] Allow to create Hocuspocus / documents access tokens via the APIv3 - #25879
judithroth wants to merge 9 commits into
Conversation
judithroth
changed the title
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
da25991 to
57bf9fc
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent refreshes can reuse one previous token and both successfully mint replacements.
1 open finding
What changed in this PR
Adds APIv3 creation and rotation of Hocuspocus collaboration tokens while restricting their OAuth access to document operations.
Changes:
- Adds collaboration-token API resources, representers, encryption, rotation, and delayed revocation.
- Introduces internal/restricted OAuth scope enforcement.
- Adds OpenAPI documentation and comprehensive tests.
| File | Description |
|---|---|
spec/requests/oauth/oauth_authorization_server_metadata_spec.rb |
Tests hidden internal scopes. |
spec/lib/open_project/authentication/strategies/warden/doorkeeper_oauth_spec.rb |
Tests restricted-scope authentication. |
spec/controllers/oauth/applications_controller_spec.rb |
Tests scope filtering in forms. |
spec/contracts/oauth/applications/base_contract_spec.rb |
Tests internal-scope validation. |
modules/documents/spec/support/shared/collaboration_token_endpoint.rb |
Adds shared endpoint tests. |
modules/documents/spec/services/documents/oauth/token_with_metadata_service_spec.rb |
Tests token rotation behavior. |
modules/documents/spec/services/documents/oauth/generate_token_service_spec.rb |
Tests generated token scope. |
modules/documents/spec/services/documents/oauth/ensure_application_service_spec.rb |
Tests application scope migration. |
modules/documents/spec/services/documents/oauth/decrypt_token_service_spec.rb |
Tests token decryption failures. |
modules/documents/spec/requests/api/v3/documents/edit_documents_scope_spec.rb |
Tests restricted endpoint access. |
modules/documents/spec/requests/api/v3/documents/collaboration_token_resource_spec.rb |
Tests collaboration-token API. |
modules/documents/spec/models/document_spec.rb |
Tests collaboration availability. |
modules/documents/spec/api/v3/utilities/path_helper_spec.rb |
Tests the new API path. |
modules/documents/spec/api/v3/documents/document_representer_rendering_spec.rb |
Tests collaboration action links. |
modules/documents/lib/open_project/documents/engine.rb |
Registers paths and OAuth scopes. |
modules/documents/lib/api/v3/documents/documents_api.rb |
Mounts and scopes endpoints. |
modules/documents/lib/api/v3/documents/document_representer.rb |
Adds token-creation action link. |
modules/documents/lib/api/v3/documents/collaboration_token_representer.rb |
Represents issued tokens. |
modules/documents/lib/api/v3/documents/collaboration_token_api.rb |
Implements token endpoint. |
modules/documents/config/locales/en.yml |
Adds document OAuth messages. |
modules/documents/app/services/documents/oauth/token_with_metadata_service.rb |
Implements token rotation. |
modules/documents/app/services/documents/oauth/token_encryptor.rb |
Shares cipher configuration. |
modules/documents/app/services/documents/oauth/generate_token_service.rb |
Uses restricted document scope. |
modules/documents/app/services/documents/oauth/ensure_application_service.rb |
Enforces application scope. |
modules/documents/app/services/documents/oauth/encrypt_token_service.rb |
Uses shared encryptor. |
modules/documents/app/services/documents/oauth/decrypt_token_service.rb |
Decrypts collaboration tokens. |
modules/documents/app/services/documents/oauth.rb |
Defines the internal scope. |
modules/documents/app/models/document.rb |
Adds collaboration availability helper. |
modules/documents/app/controllers/documents_controller.rb |
Reuses availability helper. |
lib/api/root_api.rb |
Enforces endpoint OAuth scopes. |
lib/api/errors/insufficient_scope.rb |
Adds scoped-access error. |
lib_static/open_project/authentication/strategies/warden/doorkeeper_oauth.rb |
Accepts registered restricted scopes. |
lib_static/open_project/authentication.rb |
Adds OAuth scope registries. |
docs/api/apiv3/tags/documents.yml |
Documents collaboration workflow. |
docs/api/apiv3/paths/document.yml |
Documents action links. |
docs/api/apiv3/paths/document_collaboration_token.yml |
Documents the new endpoint. |
docs/api/apiv3/openapi-spec.yml |
Registers path and schema. |
docs/api/apiv3/components/schemas/document_model.yml |
Adds collaboration action schema. |
docs/api/apiv3/components/schemas/collaboration_token_model.yml |
Defines token response schema. |
config/locales/en.yml |
Adds insufficient-scope message. |
app/views/oauth/applications/_form.html.erb |
Hides internal scopes. |
app/controllers/oauth_metadata_controller.rb |
Omits internal scopes from metadata. |
app/contracts/oauth/applications/base_contract.rb |
Prevents external internal-scope assignment. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
6599f27 to
65dc80e
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
judithroth
changed the title
github-actions
Bot
added
the
ai: Collaborative 💻
judithroth
changed the title
NobodysNightmare
left a comment
NobodysNightmare
left a comment
There was a problem hiding this comment.
I commented with some thoughts. I only reviewed the part about accepting additional scopes in API requests.
We already talked about this, but I want to say it in writing once more as well: Adding narrower scopes for API requests is a fundamental change. It's one that I'd have done in a separate PR just for that purpose and after a longer discussion with more parts of the dev team to consider how exactly we want to go about this. A symptom of this is that you see me asking fundamental questions here as well.
I am also not yet sure if I've asked all the questions I should have asked.
| return if endpoint_scopes.any? { |scope| access_token.includes_scope?(scope) } | ||
|
|
||
| raise ::API::Errors::InsufficientScope.new(granted_scopes: access_token.scopes.to_a, | ||
| required_scope: authentication_scope) |
There was a problem hiding this comment.
🔴 As far as I understand the code the required_scope must also include endpoint_scopes, because these are required to access the endpoint.
There was a problem hiding this comment.
Good catch. The way it was implemented would have worked as long as there is no other more narrowed down scope than edit_documents (or whatever we will end up calling it)
| # scope and only grant access to endpoints opting into it via `route_setting :oauth_scopes`. | ||
| def enforce_oauth_scope | ||
| access_token = env[OpenProject::Authentication::Strategies::Warden::DoorkeeperOAuth::ACCESS_TOKEN_ENV_KEY] | ||
| return if access_token.nil? || access_token.includes_scope?(authentication_scope) |
There was a problem hiding this comment.
❓ Just to be sure I understand the intention of the change correctly: authentication_scope is api_v3 in our case, correct?
This means the idea would be that api_v3 becomes a universal scope to access the entirety of API v3 and other scopes (such as the one for reading documents) could be used as a narrower permission than api_v3, but replacing the need to also carry the api_v3 scope, right?
There was a problem hiding this comment.
As far as I understand it, api_v3 is already a universal scope to access the entirety of APIv3 and I think we should keep it this way. So yes, other scopes which are more specific should work without the api_v3 scope.
| end | ||
| module Documents | ||
| module OAuth | ||
| EDIT_DOCUMENTS_SCOPE = "edit_documents" |
There was a problem hiding this comment.
Given that this scope is so far added to the list of internal_oauth_scopes and (as far as I understand) those internal scopes are designed to remove them from any UI advertising scopes, I guess the idea is to hide this new scope as much as possible.
However, there's no way around that the code introduced here is a fundamental change to authorization in APIv3. IMO we need to think about how we would scale the change added here to cover the API as a whole, even if that coverage itself is out-of-scope for this PR.
One starting question that I have in mind is naming and which kinds of scopes will actually exist.
The current naming looks like <verb>_<resource>. For composite resources like work_package this would mean read_work_package. While this is not a strong blocker, I think it makes sense to just choose a different delimiter. Also considering that usually the resource is more important than the verb, I'd probably turn the order around. This would give me something like: documents:read / documents:write
In the new style one could argue that singular is enough, so maybe document:read.
These scopes we'd define this way would all grant access to different slices of our API. However, there was also the idea to use scopes to slice access differently. E.g. only grant access to a subset of projects. So maybe (and this is absolutely a "maybe", because I do not know) we should add another prefix to define that this is the one kind of scope, so we don't get into naming troubles with the other kind of (yet undefined) scope.
When thinking about managing tokens through an IDP, it would even make sense to have some OpenProject prefix in the name, just so that we have a lower chance of colliding with other tools. E.g. users:manage has a high likelyhood to also exist elsewhere. So maybe the additional layer could be something like op-resource:documents:read (or op-res:documents:read for brevity). This would give us some opportunity to define op-BLUBB:... later on, where "BLUBB" would define what that kind of scope signifies.
There was a problem hiding this comment.
I guess the idea is to hide this new scope as much as possible
Well, I don't think we need to hide it, but I also don't want to promote it. I now moved it to OpenProject::Documents (Similar to how e.g. OpenProject::Bim adds bcf_v2_1). If you see a better place for putting this constant, I'm open to that 🙂
it makes sense to just choose a different delimiter.
Agreed, good point. I'm happy with : as well.
I'd like to stay with plural. We're not allowing to read only a single document with something like documents:read, but all the user has permission to.
there was also the idea to use scopes to slice access differently. E.g. only grant access to a subset of projects
I don't see that coming soon because we already have that through the permissions. But maybe you know more than I do 🙂 You're right that it's good to stay flexible, so I'm fine with adding resource to the scope name.
it would even make sense to have some OpenProject prefix in the name, just so that we have a lower chance of colliding with other tools.
You're right, when an IDP could be involved, an op prefix would be useful. Why - as delimiter this time instead of :?
So I'd go with op:resource:documents:edit if you're okay with that?
I wonder if we should also mention apiv3 in the scope name. What's your opinion on this?
There was a problem hiding this comment.
I like your proposal. You are right, multiple : work well and read well.
I wonder if we should also mention apiv3 in the scope name. What's your opinion on this?
Personal opinion (i.e. gut feeling): Let's leave it out for now. I'd rather move into a direction where the scopes dictate which resources you can access, not the means of which API you use. E.g. there's no practical difference in whether you use /api/v3/documents to obtain a list of documents or whether you use a search_documents MCP tool. In both cases you get access to documents, only the client compatibility will be different.
So I could imagine those two (long term) futures:
- Tokens can be equally used for different APIs (e.g. APIv3 and MCP)
- We actually require a scope that determines which API you can use in addition to the resource scopes
- e.g.
op:api:v3 op:resource:documents:readto read documents via API v3 andop:api:mcp op:resource:documents:readto read documents via both MCP and API v3
- e.g.
Specifying the type of API in the resource scope feels redundant to me.
| @@ -37,6 +37,8 @@ module Warden | |||
| class DoorkeeperOAuth < ::Warden::Strategies::Base | |||
There was a problem hiding this comment.
🟡 Sadly GitHub doesn't give me a better place to comment this, so let me do it here: The JwtOidc strategy most likely also needs to be adapted accordingly, so that it would accept the new kinds of scopes in API requests.
There was a problem hiding this comment.
In the long run, yes, but I think it's out of scope ( 😄 ) for this PR, since this is only about OAuth tokens. Building scopes for other authentication mechanims would be dead code for the time being. I think we should build that when we need it.
There was a problem hiding this comment.
I am torn here. On the one hand I understand why it's not necessary for the feature that you are building and given that we invest some amount in hiding the new feature, we ensure that nobody expects it at least ^^
However, on the other hand JwtOidc is a "free" way to make this new feature useful.
Building scopes for other authentication mechanims would be dead code for the time being.
This is not the case for this strategy. This strategy utilizes access tokens created in a third-party identity provider (e.g. Keycloak). The great thing is that you can freely customize what those tokens look like. This means that this strategy might be the first to utilize any additional scoping features we are building. Already today it allows some use cases that OpenProject could not realize through its internal Doorkeeper implementation.
So this code would most certainly not be "dead". There is no additional "internal" code needed to make it work. On the other hand I agree, that it only really becomes useful, once we extended the work you started to more endpoints. But it's really not a long shot.
There was a problem hiding this comment.
Seen from another angle: I think if we supported it here, then all the places that currently work with scopes would properly handle the new mechanism you added as well. It would prove that the approach can be scaled.
I kinda want to avoid that scopes are "a bit" supported in multiple places, but that all of these places support a different subset of scopes.
| def self.insufficient_scope_headers | ||
| lambda do | ||
| header = OpenProject::Authentication::WWWAuthenticate.response_header(env:, | ||
| scope: authentication_scope, |
There was a problem hiding this comment.
🔴 Passing the wrong scopes here. As far as I can tell authentication_scope would practically only be api_v3, when we really needed insufficient_scope_exception.required_scope instead.
| # | ||
| # @param [Symbol] warden_scope The Warden scope (see `OpenProject::Authentication::Scope`). | ||
| # @param [String] oauth_scope The OAuth scope. | ||
| def add_restricted_oauth_scope(warden_scope, oauth_scope) |
There was a problem hiding this comment.
🟡 I think we should be more structured about the endpoint scopes we add. Right now, we don't restrict the scope names in any way. So the documents module can go for edit_documents, meetings could use read_meeting (singular) and storages could use op-resource:storage:read while the last one chooses banana.
As I wrote in my longer comment about naming, I think we should come up with a structure for our scope names. This registry here should support us in keeping that structure.
E.g. the DSL could look like
add_resource_scope(:documents, :edit)which would cause the creation of a scope called op-resource:documents:edit, but also the creation of a scope called op-resource:documents:all which would grant access to all actions on the given resource. Maybe the latter behaviour is out of scope for the current PR, but it's one of those things that we'd want to be named the same across all resources, so it would make sense to harmonize it as well.
(this is one of the comments I wanted to make earlier but forgot about)
There was a problem hiding this comment.
I've added a method to ensure a structure on the restricted OAuth scope names 👍 It's good to give clear instructions on how we expect a scope name to be called.
Like you mentioned, I think op-resource:documents:all is out of scope for this PR.
4d2b70d to
2c9402b
Compare
Deploying openproject with ⚡ PullPreview
|
https://community.openproject.org/wp/SE-664 - POST /api/v3/documents/:id/collaboration_token creates an encrypted token for the collaboration server (Hocuspocus). - An optional `token` revokes the previous token 30 seconds later; each token can be revoked only once (race-free). - Requests authenticated with a collaboration token (an OAuth token issued to the Documents OAuth application) are rejected with 403. - Includes the OpenAPI docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
https://community.openproject.org/wp/SE-664 They can not connect anyway, so why give them tokens. Fixing the behaviour for anonymous users on instances that are public and have a public project with documents is out of scope of this PR though.
This reverts commit eed5d55.
https://community.openproject.org/wp/SE-664 - Collaboration tokens only carry `edit_documents`, accepted solely by GET/PATCH /api/v3/documents/:id (`route_setting :oauth_scopes`). An existing Documents OAuth application is switched to that scope. - All other APIv3 endpoints answer 403 `InsufficientScope`. - The scope is internal: it's hidden from OAuth apps and metadata. - It replaces the application-uid check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2c9402b to
2345374
Compare
judithroth
left a comment
judithroth
left a comment
There was a problem hiding this comment.
thanks for your review! I tried to address your concerns.
Actually, I think adding scopes would have been a really nice passion time project, because it's important, but never makes it to the roadmap... However, I just want it to secure my endpoint. Because, as far as I know, it's the OAuth way of having a whitelist of allowed endpoints. But I don't feel like it's incredibly complex - I don't want to invent a wheel, I just want to attach it to my vehicle so to say. The only complexity comes from OpenProject itself.
I'd be happy if you could take a look again. Mark wants those changes for the API so the mobile app can use Hocuspocus. Let's see if we can bring this to a place where at least you and I agree that it's not bad and then we can still consider what parts of it (e.g. the naming scheme) we want to discuss with the developer team, OK?
| # scope and only grant access to endpoints opting into it via `route_setting :oauth_scopes`. | ||
| def enforce_oauth_scope | ||
| access_token = env[OpenProject::Authentication::Strategies::Warden::DoorkeeperOAuth::ACCESS_TOKEN_ENV_KEY] | ||
| return if access_token.nil? || access_token.includes_scope?(authentication_scope) |
There was a problem hiding this comment.
As far as I understand it, api_v3 is already a universal scope to access the entirety of APIv3 and I think we should keep it this way. So yes, other scopes which are more specific should work without the api_v3 scope.
| return if endpoint_scopes.any? { |scope| access_token.includes_scope?(scope) } | ||
|
|
||
| raise ::API::Errors::InsufficientScope.new(granted_scopes: access_token.scopes.to_a, | ||
| required_scope: authentication_scope) |
There was a problem hiding this comment.
Good catch. The way it was implemented would have worked as long as there is no other more narrowed down scope than edit_documents (or whatever we will end up calling it)
| @@ -37,6 +37,8 @@ module Warden | |||
| class DoorkeeperOAuth < ::Warden::Strategies::Base | |||
There was a problem hiding this comment.
In the long run, yes, but I think it's out of scope ( 😄 ) for this PR, since this is only about OAuth tokens. Building scopes for other authentication mechanims would be dead code for the time being. I think we should build that when we need it.
| # | ||
| # @param [Symbol] warden_scope The Warden scope (see `OpenProject::Authentication::Scope`). | ||
| # @param [String] oauth_scope The OAuth scope. | ||
| def add_restricted_oauth_scope(warden_scope, oauth_scope) |
There was a problem hiding this comment.
I've added a method to ensure a structure on the restricted OAuth scope names 👍 It's good to give clear instructions on how we expect a scope name to be called.
Like you mentioned, I think op-resource:documents:all is out of scope for this PR.
| end | ||
| module Documents | ||
| module OAuth | ||
| EDIT_DOCUMENTS_SCOPE = "edit_documents" |
There was a problem hiding this comment.
I guess the idea is to hide this new scope as much as possible
Well, I don't think we need to hide it, but I also don't want to promote it. I now moved it to OpenProject::Documents (Similar to how e.g. OpenProject::Bim adds bcf_v2_1). If you see a better place for putting this constant, I'm open to that 🙂
it makes sense to just choose a different delimiter.
Agreed, good point. I'm happy with : as well.
I'd like to stay with plural. We're not allowing to read only a single document with something like documents:read, but all the user has permission to.
there was also the idea to use scopes to slice access differently. E.g. only grant access to a subset of projects
I don't see that coming soon because we already have that through the permissions. But maybe you know more than I do 🙂 You're right that it's good to stay flexible, so I'm fine with adding resource to the scope name.
it would even make sense to have some OpenProject prefix in the name, just so that we have a lower chance of colliding with other tools.
You're right, when an IDP could be involved, an op prefix would be useful. Why - as delimiter this time instead of :?
So I'd go with op:resource:documents:edit if you're okay with that?
I wonder if we should also mention apiv3 in the scope name. What's your opinion on this?
| def self.insufficient_scope_headers | ||
| lambda do | ||
| header = OpenProject::Authentication::WWWAuthenticate.response_header(env:, | ||
| scope: authentication_scope, |
I tagged everyone who seemed interested in this but don't expect 4 reviews 😄 (at least one would be nice though)
Ticket
https://community.openproject.org/wp/SE-664
What are you trying to accomplish?
What approach did you choose and why?
Out of scope (will be a separate PR)
AI involvement
Collaborative – AI generated a substantial part of the code; I reviewed and understand every line.
Merge checklist