Implement oidc#628

Open

Jeidnx wants to merge 25 commits intoTeamPiped:masterfrom

Jeidnx commented Jun 18, 2023

see TeamPiped/Piped#2571


          Implement oidc

604fa65

Jeidnx mentioned this pull request

Add support for oidc TeamPiped/Piped#2571

Open

sonatype-lift bot reviewed

View reviewed changes

src/main/java/me/kavin/piped/consts/Constants.java Outdated Show resolved Hide resolved

FireMasterK reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/ServerLauncher.java Outdated Show resolved Hide resolved

Jeidnx and others added 11 commits

June 19, 2023 13:24


          Better Error handling for oidc config

375ee58


          Save redirect in state

143711c


          Show warning message before oidc login

18d9317


          Only show warning when not redirecting to configured frontend

f4b9dff


          Merge branch 'master' into oidc

53d9b9d

# Conflicts:
#	build.gradle


          Merge remote-tracking branch 'origin/master' into oidc

97889f3


          Simplify config handling.

847f80c


          Add missing newline.

946ac45


          Format all code.

0eb2351


          Merge branch 'master' into oidc

9b7246a


          Implement account deletion and cleanup some code

e7f2187

FireMasterK requested changes

View reviewed changes

Member

FireMasterK left a comment

One thing I don't like is there's too much business logic in ServerLaunche, I would ideally like to move it to a new file like OidcHandlers.java or to the existing UserHandlers class.

Jeidnx and others added 5 commits

October 26, 2023 13:24


          Refactor oidc logic into UserHandlers

c1fde37


          Add database migration for username length change.

024435f


          Revert "Add database migration for username length change."

470efd8

This reverts commit 024435f.


          Add code from the meeting.

5f6a83a


          Merge branch 'master' into oidc

868103c

Jeidnx force-pushed the oidc branch from e86c915 to 8669079 Compare

November 12, 2024 14:34

Author

Jeidnx commented Nov 12, 2024 •

edited

Loading

I have cleaned up the code and made sure everything works. Here are a few things worth mentioning:

This only supports providers which have the openid connect discovery standard (.well-known/openid-configuration) implemented, which should be all relevant providers.
User deletion currently only works if the provider supports the optional max_age parameter and properly returns the auth_time claim. We could drop this restriction, but that would allow one-click deletion of accounts without verifying the identity of the user.
This also only works with providers which support the PKCE code authorization flow, which again should be most of them.

Please let me know if there are any questions or concerns. I have tested this and from my view this could be merged

edit: Found the alternative for TokenRequest..


          chore: properly implement oidc

074e4bc

Jeidnx force-pushed the oidc branch from 8669079 to 074e4bc Compare

November 13, 2024 09:56

FireMasterK added 3 commits

November 18, 2024 00:23


          Simplify oidc hash generation.

580eb7f


          Simplify error handling code a little.

9520a3c


          Remove debug code and format.

b0725f8

FireMasterK reviewed

View reviewed changes

src/main/resources/changelog/version/2-add-oidc.xml Outdated Show resolved Hide resolved

FireMasterK reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java Outdated Show resolved Hide resolved

FireMasterK reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java Outdated Show resolved Hide resolved

Jeidnx added 4 commits

November 20, 2024 12:20


          Move OidcData to db + some cleanup

74a6751


          randomize username

f76f8e0


          add redirect to oidc delete; more cleanup

e4ba195


          explicitly reject empty hashes

77cd736

Bnyro reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/ServerLauncher.java

Comment on lines +537 to +541

+                      for (int i = 0; i < Constants.OIDC_PROVIDERS.size(); i++) {
+                          OidcProvider curr = Constants.OIDC_PROVIDERS.get(i);
+                          if (curr == null || !curr.name.equals(provider)) continue;
+                          return curr;
+                      }

Member

Bnyro Jan 8, 2025

Suggested change

      
                    for (int i = 0; i < Constants.OIDC_PROVIDERS.size(); i++) {
          
                        OidcProvider curr = Constants.OIDC_PROVIDERS.get(i);
          
                        if (curr == null || !curr.name.equals(provider)) continue;
          
                        return curr;
          
                    }
          
                    for (OidcProvider curr : Constants.OIDC_PROVIDERS) {
          
                        if (curr != null && curr.name.equals(provider)) return curr;
          
                    }

Bnyro reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java

+                              .state(new State(state))
+                              .nonce(data.getOidNonce()).build();
+                      if (redirectUri.equals(Constants.FRONTEND_URL + "/login")) {

Member

Bnyro Jan 8, 2025

We probably shouldn't hardcode the url path here, maybe it makes sense to parse the domain of the redirectUri and see if equals Constants.FRONTEND_URL? Or we could just do a redirectUri.startsWith(Constants.FRONTEND_URL + "/").

Bnyro reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java

+                      return HttpResponse.ok200().withHtml(
+                              "<!DOCTYPE html><html style=\"color-scheme: dark light;\"><body>" +
+                                      "<h3>Warning:</h3> You are trying to give <pre style=\"font-size: 1.2rem;\">" +
+                                      redirectUri +

Member

Bnyro Jan 8, 2025

redirectUri needs to be HTML escaped or checked if it's a real URL, otherwise that would allow HTML injection.

Bnyro reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java

+                      OidcData data = DatabaseHelper.getOidcData(authResponse.getState().toString());
+                      if (data == null) {
+                          return HttpResponse.ofCode(400).withHtml(

Member

Bnyro Jan 8, 2025

Since we don't provide any styling or color-scheme: dark light;, withPlainText would probably be better to not become flashed in dark mode

Bnyro reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java

+                      AuthorizationCode code = authResponse.getAuthorizationCode();
+                      if (code == null) {
+                          return HttpResponse.ofCode(400).withHtml(

Member

Bnyro Jan 8, 2025

Same as above (withPlainText)

Bnyro reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java

Comment on lines +210 to +213

+                          return HttpResponse.ofCode(400).withHtml("Received a bad token. Please try again");
+                      } catch (JOSEException e) {
+                          System.err.println("Token processing error: " + e);
+                          return HttpResponse.ofCode(500).withHtml("Internal processing error. Please try again");

Member

Bnyro Jan 8, 2025

withPlainText

Bnyro reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java

+                      UserInfoResponse userInfoResponse = UserInfoResponse.parse(ur.toHTTPRequest().send());
+                      if (!userInfoResponse.indicatesSuccess()) {
+                          return HttpResponse.ofCode(500).withHtml(

Member

Bnyro Jan 8, 2025

withPlainText

Bnyro reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java


		cr.select(root).where(root.get("sub").in(sub));

		OidcUserData dbuser = s.createQuery(cr).uniqueResult();

Member

Bnyro Jan 8, 2025

nit:

Suggested change

      
                        OidcUserData dbuser = s.createQuery(cr).uniqueResult();
          
                        OidcUserData dbUser = s.createQuery(cr).uniqueResult();

Bnyro reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java

Comment on lines +283 to +288

+                          for (OidcProvider oidcProvider : Constants.OIDC_PROVIDERS) {
+                              if (oidcProvider.name.equals(oidcUserData.getProvider())) {
+                                  provider = oidcProvider;
+                                  break;
+                              }
+                          }

Member

Bnyro Jan 8, 2025

Suggested change

      
                        for (OidcProvider oidcProvider : Constants.OIDC_PROVIDERS) {
          
                            if (oidcProvider.name.equals(oidcUserData.getProvider())) {
          
                                provider = oidcProvider;
          
                                break;
          
                            }
          
                        }
          
                        provider = ServerLauncher.getOidcProvider(oicdUserDate.getProvider());

Bnyro reviewed

View reviewed changes

src/main/java/me/kavin/piped/server/handlers/auth/UserHandlers.java

Comment on lines +322 to +369

+                      AuthenticationSuccessResponse sr = parseOidcUri(requestUri);
+                      OidcData data = DatabaseHelper.getOidcData(sr.getState().toString());
+                      if (data == null) {
+                          return HttpResponse.ofCode(400).withHtml(
+                                  "Your oidc provider sent invalid state data. Try again or contact your oidc admin"
+                          );
+                      }
+                      String redirect = data.data.split("\\|")[1];
+                      String session = data.data.split("\\|")[0];
+                      URI callback = new URI(Constants.PUBLIC_URL + "/oidc/" + provider.name + "/delete");
+                      AuthorizationCode code = sr.getAuthorizationCode();
+                      if (code == null) {
+                          return HttpResponse.ofCode(400).withHtml(
+                                  "Your oidc provider sent an invalid code. Try again or contact your oidc admin"
+                          );
+                      }
+                      AuthorizationGrant codeGrant = new AuthorizationCodeGrant(code, callback, data.getOidVerifier());
+                      ClientAuthentication clientAuth = new ClientSecretBasic(provider.clientID, provider.clientSecret);
+                      TokenRequest tokenRequest = new TokenRequest.Builder(provider.tokenUri, clientAuth, codeGrant).build();
+                      TokenResponse tokenResponse = OIDCTokenResponseParser.parse(tokenRequest.toHTTPRequest().send());
+                      if (!tokenResponse.indicatesSuccess()) {
+                          TokenErrorResponse errorResponse = tokenResponse.toErrorResponse();
+                          return HttpResponse.ofCode(500).withHtml("Failure while trying to request token:\n\n" + errorResponse.getErrorObject().getDescription());
+                      }
+                      OIDCTokenResponse successResponse = (OIDCTokenResponse) tokenResponse.toSuccessResponse();
+                      JWT idToken = JWTParser.parse(successResponse.getOIDCTokens().getIDTokenString());
+                      IDTokenClaimsSet claims;
+                      try {
+                          claims = provider.validator.validate(idToken, data.getOidNonce());
+                      } catch (BadJOSEException e) {
+                          System.err.println("Invalid token received: " + e);
+                          return HttpResponse.ofCode(400).withHtml("Received a bad token. Please try again");
+                      } catch (JOSEException e) {
+                          System.err.println("Token processing error: " + e);
+                          return HttpResponse.ofCode(500).withHtml("Internal processing error. Please try again");
+                      }

Member

Bnyro Jan 8, 2025 •

edited

Loading

That's just a lot of duplicate code from oidcLoginCallback, it would certainly make sense to refactor the common code into a new method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet