Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configuration for authentication method #17614

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

NobodysNightmare
Copy link
Contributor

Preparing for users to choose how they want to authenticate their nextcloud storage and if using a common IDP, what the client_id of nextcloud is.

Ticket

https://community.openproject.org/projects/cross-application-user-integration-stream/work_packages/60153

What are you trying to accomplish?

Adding configuration for the authentication method to the storage provider, so that we can work on two pieces of code afterwards:

  • Building the UI that sets this configuration
  • Consuming these settings to perform authentication using either method

Merge checklist

  • Added/updated tests

@NobodysNightmare NobodysNightmare force-pushed the configure-authentication-method branch 3 times, most recently from b9541bc to cad63db Compare January 15, 2025 14:59
@NobodysNightmare NobodysNightmare requested a review from a team January 16, 2025 07:44
@mereghost mereghost requested a review from Kharonus January 16, 2025 10:49
@@ -0,0 +1,9 @@
# frozen_string_literal: true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 do we need this copyright thingy here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It was, there was a discussion and all files should have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL. Just now I realized that this comment was about the copyright thingy missing 🙈

I added it. I believe the rest should be fine now as well, w.r.t. to the previous comments.

@@ -35,11 +35,15 @@ class NextcloudStorage < Storage
username: "OpenProject"
}.freeze

AUTHENTICATION_METHODS = %w[mutual_oauth common_oidc_idp].freeze
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 I think I now raise the obvious discussion: naming 🤯

I'm not a too big fan of mutual_oauth, mainly because it is not reflected anywhere in our domain speech.

suggestions:

  • authorization _code_flow
  • oauth2
  • bidirectional_oauth2

first one is probably too specific, as oauth enables other methods, too, as client credentials (just NC is not able to do it (yet?)).

second one is my favorite, precise yet still generic enough to bring everything of this method under the hood.

For common_oidc_idp I'd probably drop the common and just go with oidc_idp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 on

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with oauth2 and oidc (maybe even oidc_sso).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, so let me put my least favorite pair first: oauth2 vs oidc 😅

I can explain: I've been thinking quite a bunch about what differentiates the two flows. And the first thing I noticed is that we shouldn't call any of the two just oauth2, because OAuth 2.0 is part of the OIDC spec. Or in other words: Everything that is OIDC is also OAuth 2.0.

Strictly speaking we don't even need the IDP to be OIDC compliant, OAuth 2.0 is mostly enough. This means we are close to oauth2 vs oauth2, which would be confusing. So what is the difference between the two strategies we want to offer?

In the beginning token_exchange would have been an acceptably good name for the new strategy, because that was a distinguishing feature and requirement. However, we later weakened the requirements to not always need OAuth 2.0 Token Exchange, by allowing to have the IDP issue tokens that work for both apps, so the name doesn't make sense anymore.

In the end, I settled for the difference being the topology of the parties. In the old strategy we have two equal parties that care about authenticating users on their own, but for the matter of cross-application calls, each one acts as OAuth 2.0 Authorization Server (AS) for the other. So when OP calls NC, then NC is the AS and when NC calls OP, then OP is the AS. The two are equal and mutually accept each other as AS.
This is different from our new strategy, where the two agree on a third party AS that hands out the tokens and both of them only need to trust that (central/common) third party. This is where I source my mutual vs common naming.

</wall-of-text>

tl;dr:

  • That's why I don't like oauth2 vs oidc
  • That's why I suggested mutual_oauth vs common_oidc_idp
  • Iterating from @mereghost 's suggestion: I think I could accept the new strategy being called sso or oidc_sso
    • It's a bit simplified, but captures the idea pretty well still
    • But then oauth2 would still be a bad name for the other strategy... Brainstorming: two-way-oauth2? 🤔 bidi-oauth2 💭 2-oauth2 🤯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S.:

mainly because it is not reflected anywhere in our domain speech

I totally intend to take whatever we decide on and make it part of our domain speech.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point here, but I would disagree on a statement like: "variable names must reflect the technical and algorithmic correctness of methods".

So, to paraphrase: If you feel, OAuth2 is not the correct term to describe our first method of authentication in the domain language, than we should challenge the domain language.

But, everytime, I would disagree on using different words in code and in the domain language, because with this, we introduce misunderstandings by design.

So, what is the next step. How are our both auth methods called? In latest meetings I always understood that we are talking about OAuth2 and OIDC. That behind that we, as technical experts understand, that the first is a barebone OAuth2 with bidirectional access and two different providers - and the second is the OIDC standard, which internally also uses OAuth2, is completely irrelevant IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you feel, OAuth2 is not the correct term to describe our first method of authentication in the domain language, than we should challenge the domain language.

That's what I am doing pretty actively. E.g. these are the current wireframes for the configuration UI:
image
image

and the second is the OIDC standard, which internally also uses OAuth2, is completely irrelevant IMHO

The more I think about this, the less I want to include the term OIDC at all (neither in domain language, nor in code). We do not rely on OIDC in the new method at all.

We rely on either or both of the following two standards for the new implementation:

  • OAuth 2.0 Token Exchange
  • The access token being readable as a JSON Web Token
    • this allows us to know, whether we are the audience of a token
    • however, OIDC does not require access tokens to be JWTs, it's just a nice conincidence, that most OIDC IDPs use these kinds of tokens, because they are cool and they are required to use them for the ID token (which we do not rely on)

The only thing that we will probably ask admins to do, that is technically (i.e. unimportantly) related to OIDC, is to add the scope offline_access to the requested scopes or to otherwise ensure that we obtain long-lived refresh tokens from the IDP. The offline_access scope is indeed part of OIDC, but it's pretty much a side-part of the spec and we don't actively code against that. If admins can otherwise ensure that we get a long-lived refresh_token it will also work.

My current favorite pair is sso vs. two-way-oauth2. Alternatively oauth2-sso vs two-way-oauth2. This would indicate that both strategies are OAuth strategies and leave room for us to introduce fancy saml-sso or similar in the future (i.e. not burning the term SSO in general)...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After IRL discussion we landed on the last two names, though we want to convert them to snake-case for Ruby happiness:

  • oauth2_sso
  • two_way_oauth2

@Kharonus Kharonus requested a review from a team January 17, 2025 12:39
@@ -35,11 +35,15 @@ class NextcloudStorage < Storage
username: "OpenProject"
}.freeze

AUTHENTICATION_METHODS = %w[mutual_oauth common_oidc_idp].freeze
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with oauth2 and oidc (maybe even oidc_sso).

@NobodysNightmare NobodysNightmare force-pushed the configure-authentication-method branch from cad63db to 8486b2c Compare January 20, 2025 14:35
@NobodysNightmare NobodysNightmare force-pushed the configure-authentication-method branch from 8486b2c to b5a4a14 Compare January 21, 2025 16:04
@NobodysNightmare NobodysNightmare force-pushed the configure-authentication-method branch from b5a4a14 to 4ede681 Compare January 22, 2025 06:45
Preparing for users to choose how they want to authenticate their
nextcloud storage and if using a common OAuth 2.0 IDP, what the client_id
of nextcloud is.
@NobodysNightmare NobodysNightmare force-pushed the configure-authentication-method branch from 4ede681 to 5ecc78e Compare January 22, 2025 09:48
Copy link
Member

@machisuji machisuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@NobodysNightmare NobodysNightmare merged commit e6c95f3 into dev Jan 23, 2025
11 checks passed
@NobodysNightmare NobodysNightmare deleted the configure-authentication-method branch January 23, 2025 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants