This warden strategy is primarily used to allow APIv3 requests
from the browser, which only authenticates using its session cookie.
Since this is susceptible to cross-site-request-forgery, prevention of
CSRF must take place. This was so far only ensured through the usage of
the X-Requested-With header. When a client sent along this header, the
server could know that a CORS-preflight request must have been made and
thus the browser most certainly has validated that the request is valid
according to CORS rules.
However, the header itself is a non-standard header and while some JavaScript
frameworks add it to requests, not all of them do. For us this was practically
visible on the API docs hosted under `/api/docs`.
The solution is to expect the browser to send the Sec-Fetch-Site header with a value
of same-origin. This header can't be set through JavaScript, but only by the browser
and the value "same-origin" ensures that scheme, host and port are the same for requester
and requested endpoint, thus eliminating CSRF concerns. This feature is widely supported by
all major browsers, the last of which was Safari which added support 3 years ago.
We might want to consider dropping the check for X-Requested-With entirely, since it should be
superfluous. For now it was left in place for greater compatibility.
The name of this setting was pretty outdated by now.
It might have disabled the entire API in the past, but that time
is long gone. By now the APIv3 can't be disabled at all and OpenProject
would fall apart if it was disabled.
The only thing that this setting changes, is whether users can create
an access token in their account settings and whether tokens created
this way are accepted by OpenProject. So naming and description have
been adapted accordingly.
The intention of this change is to always respond in the metadata-rich
version of the header that indicates things like the required scope and
the URL of the resource_metadata endpoint, which was previously hidden
and only visible if clients used a non-standard HTTP request header.
semantically it's probably the preferable version of the header by now
anyways, because:
* all APIs accept some kind of Bearer token, not all of them accept Basic auth
* Even API tokens can now be passed as Bearer tokens
Practically the Basic auth header also caused unintended browser pop-ups when the frontend
code didn't include the correct request header to avoid the Basic auth offer, this now can't
happen anymore, since the Basic auth version of the header is only returned, if the client actively
tried to authenticate through Basic auth.
We generate those tokens with a prefix, so that we
can decide by looking at a token, whether it's an API Token
or a different kind of token, so that we can decide which
code path to choose for validating the token.
The usage of access tokens as Bearer token has the usability advantage,
that you can paste them as plaintext into tools that expect you
to specify the token as a header.
Also the Basic auth approach for our old tokens usually rather caused
issues, such as browsers prompting for credentials in surprising situations.
If we were to deprecate basic authentication one day, this change today could've
been the first step towards that.
So far the MCP server only offers a single tool, but authentication
and integration is already built in a way that's intended to last.
Ideally extensions to this happen by adding additional tools or resources,
but will not require further architectural changes, though realistically we'll
probably identify more potential for reuse, once we added a few more tools.
The exact representation of results is still slightly to-be-discussed. Right
now we are using vanilla APIv3 representation, which might be enough, but possibly
we want to represent linked resources differently, so that they can be recognized
to be fetchable via MCP resources more easily.
The OpenTelemetry and Appsignal helpers were already able to
be hooked into logging, but didn't offer public methods to plainly
trace the occurence of an exception. For both helpers this was possible
to be extracted directly from the existing code.
We even had a spec testing behaviour of a string that needed
escaping, but due to the fact that we constructed the relatively large
header content in the spec, we never noticed that escaping was indeed
missing in that case.
This one is defined as optional by RFC 6750, which defines
the usage of bearer tokens. It allows a client to know, which
scopes are required to access a given resource when using Bearer tokens.
https://community.openproject.org/wp/67642
Attachments::ExtractFulltextJob is run twice:
- one time on creation
- one time when the direct upload is completed
When it runs on creation, and the attachments are stored on S3, the
attachment is in 'prepared' status and is not complete yet.
Due to a bug in carrierwave which is fixed since June 2023 (see
https://github.com/carrierwaveuploader/carrierwave/issues/2524), the
`#local_file` method raises the error "NoMethodError: undefined method
'body' for nil:NilClass". There is a separate issue for upgrading this
dependency one day: https://community.openproject.org/wp/67626.
The fix is to call `#local_file` only if the attachment is readable.
Additionnally:
- error handling has been updated to raise the error instead of
swallowing it silently, so well have proper reporting in AppSignal
next time.
- when a custom S3 endpoint is used (for local testing with minio for
instance), this custom endpoint is added to the CSP.
Co-authored-by: Jan Sandbrink <j.sandbrink@openproject.com>
- Use constants for semantic names. As a bonus it gives documentation
about the metric.
- Use a random UUID for the service name. It has to be unique for a
given service name and namespace.
- Use process type as service name.
- Add service version.
Do not set anything regarding the environment (edge / stage / prod) or
shard name as these will be available as k8s labels anyway.
Favorite is the correct term in the context of expressing a preference
for a particular project / other OpenProject domain object.
Updates `ActsAsFavorable` to `ActsAsFavoritable`, as well as filenames,
identifiers and strings to:
favored => favorited
favorable => favoritable
favoring => favoriting
- Use authenticated ServiceAccount in requests
- Scope User and Group requests by ScimClient related auth_provider_id
- Include ServiceAccount search to doorkeeper_oauth strategy
- Fix SCIM Server API specs.
This should be a tad faster for the case where the user is not activated, because
the result is already filtered when fetching from the database.
Co-authored-by: Pavel Balashou <ba1ashpash@gmail.com>
The current implementation opens a small window of
opportunity to authenticate through a previously issued
access token even after a user account was locked.
If access tokens had a longer lifetime, this could become
a large window of opportunity.
Important changes:
1. Use ignored_columns to try to avoid downtime.
2. Add unique constraint for auth_provider_id+external_id.
Co-authored-by: Jan Sandbrink <453584+username@users.noreply.github.com>
This was initiated by Rubocop complaining
about the method complexity.
Rewriting it with no changes.
Adding additional test cases to specify expected behaviour,
also making edge cases visible that are otherwise covered by
separate feature specs in a different location.
We introduced the ability to disable doorkeeper applications in
the past, but apparently we didn't check that an application whose
token we validate is also enabled.
Now we make sure that only tokens of enabled applications are accepted.
Oliver Günther