* Removed unused objects... it seems some tests still relied on them.
* Adds validation keys that where missing
* Update the AMPF validator chaging the create folder validation
* Error handling on FileInfo validation and tests for the Site/List case
* Fix sharepoint factory to include the trailing slash
Co-authored-by: Jan Sandbrink <j.sandbrink@openproject.com>
Refactor DownloadLink for Storages.
Change DownloadLink input to accept file_id instead of file_link object. Update the contract and all usages in the API, providers, and specifications. Add optional origin_name for Nextcloud compatibility. Improves consistency between download and open link APIs.
Sometimes dry-validation would overwrite our custom english
messages with its default messages, recognizable by lacking a
dot at the end.
By using our own top_namespace AND ensuring that dry doesn't load
its own translations, this could be fixed. It means that we will have
to define all messages ourselves, we can't rely on default translations.
* Adds FeatureDecision. Initial steps on SharePoint -> Sharepoint rename
* Update I18n keys to use sharepoint over share_point
* Adds handling of the drives on the file picker
* Upload Strategy handling plus FileLink Contract ammendment
* Drafting a draft implementation draft
* Adds missing token exchange scope
* Remove all mention to list constants
* Changes to SharePoint rather than Sharepoint
* Addresses feedback by @kharonus
* Remove PROVIDER_TYPES
* Re-creates the Registry and Errors under the Adapters namespace.
* Bring Authentication and Strategies to Adapters
* Make Strategies work with Result and clean up a bit of the code
* Setup SetPermissions Command and tests
* Moves create folder, need to add the input value
* Adds the create folder input
* RenameFile migrated
* Files Query and some Result Objects
* Gets the sync service working with the new commands/query
* UploadLinkQuery ported
* FileInfoQuery ported
* FilePathToIdMap moved
* Cleanup unused files and warnings
* Moves DeleteFolder. Updates tests of OneDriveSyncService
* Add some tests for the the inputs
* Start moving the bare minimum for the NextcloudSync
* Moves nextcloud FilePathToIdMap
* Create and Delete Folder nextcloud commands
* Port Nextcloud FileInfo and RenameFile
* Implements the changes necessary for create folder on the file picker
* Moves the CreateFolderService to the Adapters
* Move Nextcloud SetPermissions
* AuthCheck moved. Missing teests. Slowly moving the API to Adapters
* Adds note to figure out the open queries
* Move the user and group manipulation to adapters
* Moves Nextcloud FilesQuery
* Makes NextcloudSync to run on top of the new Adapter namespace
* Disable Peripherals::Registry
* Update CopyTemplateFolderService
* Makes services green again. Moves the new Nextcloud contract to Adapters
* Moves the new nextcloud contracts and fixes some the now broken tests
* Reintroduces the Internal namespace in OneDrive. Updates the contracts for Strategy to optionally take a storage (OIDC issues)
* Moves User and DownloadLink Queries and supporting code.
* Start to move the API over the new commands/queries
* Migrates the StorgeFilesAPI to the adapters
* FileLinksAPI cleared
* Updates the Storages API specs and implementations
* OpenStorage API done
* Update capabilities query
* Move connection validators and fix some broken tests
* Delete old code, update hidden dependencies.
* Adds missing handling for sso tokens
Due to STI nature it is required. Otherwise undesired behavior is possible
in dev and test environments(where usually lazy loading is enabled).
The udesired behavior can be like:
Fetching not loaded yet STI model through its parent model
(e.g. `User.find(service_account_id)` raises `ActiveRecord::NotFound`, because
`ServiceAccount` has not been referenced yet.
`SubclassRegistry` has been removed, because:
1. `.register_subclasses` and `.registered_subclasses` produce unexpected results.
```ruby
# e.g.: Principal -> User -> ServiceAccount
Principal.register_subclass(User)
# Then
Principal.registered_subclasses == [User] # true
User.registered_subclasses == [User] # true
ServiceAccount.registered_subclasses == [User] # true
# Having User as a subclass of User and ServiceAccount seems to be weird.
```
2. There seems to be no big win in have the additional list of subclasses that have to be manually filled.
3. Used in commit apprach seems to be simpler.
It is just calling STI classes explicitly in to_prepare block of configuration.
Having a separate "ensure connection" link does not add
any value, because even the original "open" link does redirect
unconnected users properly. On the other hand, immediately returning
an ensure_connection URL is only possible if the storage was configured
for mutual OAuth 2 authentication, not for authentication through an
SSO provider.
The previous implementation was a deeply nested decision tree for
different error cases or other unexpected states.
Most of this was necessary, because setting up correct permissions is partially
happening asynchronously. Some of the redirects would eventually have rendered a modal
that would periodically check whether user permissions appeared in the background.
All of this was replaced by an approach where we immediately (synchronously) ensure
that the setup is correct.
There were several bad assumptions in the previous
implementation. First, the represented is not a storage,
but effectively a Hashie::Mash, so just a fancy hash.
Thus methods such a provider_type_nextcloud? don't exist,
but sadly no error is raised, because `?` methods will just
return false if a key with the same name is missing.
Also on a more general level, it doesn't make sense skip parsing
certain fields depending on the type, because we can't make sure
that the type is being parsed first. Thus we might skip parsing
fields and afterwards realize that we should have parsed them
in the first place.
If we wanted to prevent assigning attributes that we don't want to
assign, it would be the responsibility of SetAttributesService
and contracts to ensure that.
For example on the work package create UI, it
is already possible to link files, even though the
WP does not yet exist. In this case it's a usability
issue if the UI elements do not yet work as they should.
As long as there is no associated container, it should be no
issue to assume that the creator has all the necessary permissions
to view a file link. Downloading the associated file is still
limited by permissions on the storage, so this should not
be abusable to retain long-term access to a file.
We will now propagate HTTP errors upwards as failed service results,
top-most they are only logged, but will not raise an error anymore.
We can do this, because a missing remote identity will be shown to
the user as an error anyways. On the other hand raising an error
from the OpenProject::Notifications breaks the code performing
the original token fetching and prevents display of the nice error
message in the UI.
Introducing an explicit difference between users that were never linked
and those that have a link (i.e. a remote identity) but receive an authentication
error anyways. There is also differentiation between SSO users and non-SSO users.
When authenticating to the storage via SSO, it wouldn't make sense to show a login
button to users.
- Use SpecificBearerToken as a name for new auth strategy
- Add scope to filter storages by audience
- Do not emit REMOTE_IDENTITY_CREATED event if there are no changes to the model
- Use TOKEN_OBTAINED_EVENT for event exposed by openid_connect module
- Fix fetching of RemoteIdentity data in all cases to be correct, I hope :)
There were a few places left that hardcoded the userbound strategy
to OAuthUserToken, which used to be the only strategy for userbound
authentcation. However, by now there is also SSO-based authentication
which requires the use of a different strategy.
There is now add_replacements which accepts a hash, allowing to
add multiple replacements in one call. This should make mass-adding
replacements look less bulky.
Also including helpers to ActsAsOpEngine to make adding replacements
from a module nicer.
Instead of expecting modules to selectively overwrite
methods of this class, we offer an explicit extension point
where replacements can be registered explicitly.
Thus patching the implementation is not necessary anymore, instead
it can be configured from initializers.
- https://community.openproject.org/wp/59391
- files tab counter now only calls for file links of wp with page size 0
- file links endpoint returns paginated collections
- frontend requests file links with page size -1 (INFINITE)
- file sync is still executed for ALL file links of a work package, not
only the requested page
- this is practically no change for the product, as we do not fetch
single pages
- but fetching only the total numbers with page size 0 now does not
trigger a sync
Oliver Günther