[73320] Create and use OAuth2 auth by shiroginne · Pull Request #22714 · opf/openproject

shiroginne · 2026-04-10T08:51:48Z

Ticket

73320

What are you trying to accomplish?

Add OAuth2 auth for the XWiki provider.
This PR also adds the creation of the OAuth clients for future development.

Screenshots

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)

…_and_routes

Fix merge issue

…73405-add-service-contract-layer-for-wiki-providers

Refactor the rest of the code

Updated specs

https://community.openproject.org/work_packages/73320

…er-for-wiki-providers

…i-providers' into implementation/73320-create-and-use-oauth2-auth

Fix some issues. Pour over some specs ✨

github-actions · 2026-04-14T15:33:08Z

Caution

The provided work package version does not match the core version

Details:

Work package URL: https://community.openproject.org/projects/stream-xwiki-integration/work_packages/73320
Work package version: not set
Core version: 17.4.0

Please make sure that:

The work package version OR your pull request target branch is correct

NobodysNightmare

I left some comments during an initial code review.

I didn't yet click through the application and will do that next.

NobodysNightmare · 2026-04-16T05:57:49Z

+        if oauth_application.present?
+          concat(render(Primer::Beta::Label.new(scheme: :success)) { I18n.t(:label_completed) })
+        else
+          concat(render(Primer::Beta::Label.new(scheme: :secondary)) { I18n.t("wikis.admin.wiki_providers.oauth.label_pending") })


🟡 For the translations here (and in any other component): Shouldn't we be able to use t (not I18n.t) and pass translation paths in their relative form.

Example:

Suggested change

concat(render(Primer::Beta::Label.new(scheme: :secondary)) { I18n.t("wikis.admin.wiki_providers.oauth.label_pending") })

concat(render(Primer::Beta::Label.new(scheme: :secondary)) { t("label_pending") })

Note that I chose this line as an example rather randomly, so let me elaborate further:

I'd do that on all translation strings that are specific to this current component

this way we ensure a consistent way to structure translations

translations will be grouped by the component they are used in

I'd not do it for translations where we know that we need the exact same translation in multiple places

e.g. a label_pending might already be defined somewhere extremely globally

but if we only know of a use-case for a pending label exactly in this component, we might only define it here

... and if we only know of cases of reusing it for wiki stuff, then somewhere in the wiki namespace

NobodysNightmare · 2026-04-16T06:02:43Z

        oauth:
          openproject_oauth: OpenProject OAuth
-          openproject_oauth_description: Allow XWiki to access OpenProject data using an OAuth.
+          openproject_oauth_description: Allow XWiki to access OpenProject data using an OAuth application. Copy the credentials below into your XWiki instance.


🟡 I see an issue about translation abstraction here.

The current key of this translation is wikis.admin.wiki_providers.oauth.openproject_oauth_description. So apparently this is a generic key describing OAuth authentication in a wiki context. It's certainly usable for any given wiki provider that supports OAuth...

But the translation value is specific to XWiki, so it can't be used generically. The two possible solutions:

Replace "XWiki" with "the wiki provider" and accept that the user-facing translation is not as specific as it could be

Indicate the specific provider being used in the translation key, ideally in a form that's automatable

e.g. wikis.admin.wiki_providers.<provider_type>.oauth.openproject_oauth_description

NobodysNightmare · 2026-04-16T06:04:47Z

      heading: Inline page links
    admin:
      wiki_providers:
        index_description: Add an external wiki service to link work packages to existing wiki pages or create new ones directly from OpenProject.


Side note: I just saw this and this is a prime example of using t(".description"), which would've probably put the translation key together with other translations for the index view.

NobodysNightmare · 2026-04-16T06:06:52Z

        label_new_xwiki_instance: New XWiki provider
        no_results_title: You don't have any wiki providers set up yet.
        no_results_description: Add a wiki provider to see them here.
        label_edit: Edit XWiki provider


One more side note: Up until this point the translations were sorted alphabetically.

I am still surprised, that there is no linting rule for that, because non-sorted translations really make it hard to find stuff in longer translation files.

NobodysNightmare · 2026-04-16T06:09:41Z

+    class GeneralInformationContract < BaseContract
+      attribute :authentication_method
+      validates :authentication_method, presence: true
+      validates :authentication_method,


🟢 Shouldn't it be possible to combine the two validates statements into one or is there a semantical difference?

NobodysNightmare · 2026-04-16T06:35:33Z


      def new
-        @wiki_provider = Wikis::XWikiProvider.new
+        @wiki_provider = Wikis::XWikiProvider.new if @wiki_provider.blank?


🟡 Since ensure_valid_wizard_parameters is only called for new, can we directly call this here? I was surprised to see that @wiki_provider could be set sometimes and didn't know what would set it.

It would be clearer if the code read like this:

@wiki_provider = continue_from_wizard_params || Wikis::XWikiProvider.new

(note that I renamed the method in this example for additional clarity)

NobodysNightmare · 2026-04-16T06:40:34Z

+
+      def wiki_provider_wizard(wiki_provider)
+        Wikis::Adapters::Registry.resolve("#{wiki_provider}.components.setup_wizard")
+                                 .new(model: wiki_provider, user: current_user)


👀 This is something that we should collectively keep an eye on. I was surprised that this wasn't written as wiki_provider.resolve("components.setup_wizard"), until I realized that it required passing in the current_user as well.

This means two thing:

Not all registry components can be instantiated the same way

This alone can be a cause for concern, if we aimed at a very uniform interface

but we might want to accept it

Not all registry components can be instantiated through the resolve method right now

If we accept that the interface to create things might require additional inputs sometimes, we could extend resolve to support that, e.g.

def resolve(registry_path, **init_options) Adapters::Registry["#{self.class.registry_prefix}.#{registry_path}"].new(self, **init_options) end

This would mean we can call it like wiki_provider.resolve("components.setup_wizard", user: current_user)

CC @Kharonus @mereghost

NobodysNightmare · 2026-04-16T06:43:36Z

+        label: I18n.t("activerecord.attributes.wikis/xwiki_provider.authentication_method"),
+        required: true
+      ) do |select|
+        Wikis::XWikiProvider::AUTHENTICATION_METHODS.each do |method|


🟡 This is another case of mixing abstraction layers. We are in a class called Wikis::Admin::AuthenticationMethodSelectForm. Very generic, not XWiki related, but it acts on the things supported by Wikis::XWikiProvider.

The two ways forward that I see:

Make this form generic, but hand in information about the provider, so that the form can know which auth methods are supported for the current provider

Communicate that this is a specific form, by indicating the XWiki in its name

NobodysNightmare · 2026-04-16T06:49:09Z

    has_many :page_links, dependent: :destroy

    scope :enabled, -> { where(enabled: true) }
+    scope :visible, ->(user = User.current) { user.admin? ? all : none }


🔴 Is this what visibility is supposed to be for wiki providers?

This would mean that a non-admin is not allowed to see any wiki providers ever.

Considering that we might want to show UI elements that indicate the name of a wiki provider, this doesn't seem to be feasible to me.

I understand that we don't want to expose all details of a wiki provider to everyone (once an API for wiki providers exists), but the basic ability to see a wiki provider should be given for anyone that's able to view_wiki_links in any project.

NobodysNightmare · 2026-04-16T06:53:12Z

-
-    def url_is_https
-      return if url.blank?
+    def authenticate_via_oauth2?


🟡 Note that both authentication methods are OAuth2 based and both have OAuth2 in their name. This was intentional, because OIDC is not a requirement for any of them, still everyone keeps calling one of the two methods "OIDC based" 🤷

This method should be called accordingly. E.g.

Suggested change

def authenticate_via_oauth2?

def authenticate_via_two_way_oauth2?

shiroginne added 29 commits March 23, 2026 17:32

Add draft of the UI to create a wiki platform

1e5ecf0

Merge remote-tracking branch 'origin/dev' into 72976_create_edit_view…

65a9d4e

…_and_routes

Add base persistence for xwiki

a1f3a25

Add basic validations

f79c57f

Refactor + add destroy confirmation

0f1dbd5

Update general_info_form_component.rb

beb5c76

Merge remote-tracking branch 'origin/dev' into 72976_create_edit_view…

cffef0a

…_and_routes

Refactor + fix specs

f30d6d8

Update static_links.yml

4610192

Update engine.rb

d7c3ab9

Clean up

65cc353

Merge remote-tracking branch 'origin/dev' into 72976_create_edit_view…

670b4fc

…_and_routes

Update engine.rb

dcceda5

Update engine.rb

f98b295

Update provider.rb

e15be38

Fix merge issue

Add base contracts and services for xwiki provider

3cc1266

Address PR comments

c6c41ea

Merge branch '72976_create_edit_view_and_routes' into implementation/…

b9b7c2c

…73405-add-service-contract-layer-for-wiki-providers

Add delete service and contract

0f4d8d7

Refactor the rest of the code

Update .rubocop.yml

2dea3e9

Add translations

240e3a3

Updated specs

Update base_contract_spec.rb

ea70ee4

De-double-spacing in license

0b1e446

[#73320] Create & use OAuth2 auth

af70f9e

https://community.openproject.org/work_packages/73320

WIP add a wizard to create OAuth 2 for xwiki

0dd69b5

Merge branch 'dev' into implementation/73405-add-service-contract-lay…

a18c578

…er-for-wiki-providers

Update base_contract.rb

dbc7010

Merge branch 'implementation/73405-add-service-contract-layer-for-wik…

d44174f

…i-providers' into implementation/73320-create-and-use-oauth2-auth

Add oauth clients for the xwiki

753a77a

shiroginne self-assigned this Apr 10, 2026

shiroginne added 4 commits April 10, 2026 15:48

Add .visible scope

4dafdce

Fix some issues. Pour over some specs ✨

Fix rubocop & specs

0dd42c2

Refactor & spray some specs on top

ce79618

Update oauth_clients_controller_spec.rb

ff792fc

Base automatically changed from implementation/73405-add-service-contract-layer-for-wiki-providers to dev April 13, 2026 14:45

shiroginne added 4 commits April 13, 2026 17:24

Merge branch 'dev' into implementation/73320-create-and-use-oauth2-auth

c8d9013

Refactor + implement GeneralInformationContract

fa878b9

Small DRY + remove useless files

e5f2073

Merge branch 'dev' into implementation/73320-create-and-use-oauth2-auth

da41feb

shiroginne marked this pull request as ready for review April 14, 2026 15:32

shiroginne requested a review from a team April 14, 2026 15:32

shiroginne added 3 commits April 15, 2026 11:12

Update general_information_contract.rb

1bd1f4a

Merge branch 'dev' into implementation/73320-create-and-use-oauth2-auth

6d57d3d

Merge branch 'dev' into implementation/73320-create-and-use-oauth2-auth

96716c8

NobodysNightmare requested changes Apr 16, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[73320] Create and use OAuth2 auth#22714

[73320] Create and use OAuth2 auth#22714
shiroginne wants to merge 40 commits intodevfrom
implementation/73320-create-and-use-oauth2-auth

shiroginne commented Apr 10, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Apr 14, 2026

Uh oh!

NobodysNightmare left a comment

Uh oh!

NobodysNightmare Apr 16, 2026

Uh oh!

NobodysNightmare Apr 16, 2026

Uh oh!

NobodysNightmare Apr 16, 2026

Uh oh!

NobodysNightmare Apr 16, 2026

Uh oh!

NobodysNightmare Apr 16, 2026

Uh oh!

NobodysNightmare Apr 16, 2026

Uh oh!

NobodysNightmare Apr 16, 2026

Uh oh!

NobodysNightmare Apr 16, 2026

Uh oh!

NobodysNightmare Apr 16, 2026

Uh oh!

NobodysNightmare Apr 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

	concat(render(Primer::Beta::Label.new(scheme: :secondary)) { I18n.t("wikis.admin.wiki_providers.oauth.label_pending") })
	concat(render(Primer::Beta::Label.new(scheme: :secondary)) { t("label_pending") })

	def authenticate_via_oauth2?
	def authenticate_via_two_way_oauth2?

Conversation

shiroginne commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

Screenshots

Merge checklist

Uh oh!

github-actions bot commented Apr 14, 2026

Uh oh!

NobodysNightmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

shiroginne commented Apr 10, 2026 •

edited

Loading