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

Generic OAuth Credentials #2042

Merged
merged 40 commits into from
May 17, 2024
Merged

Generic OAuth Credentials #2042

merged 40 commits into from
May 17, 2024

Conversation

elias-ba
Copy link
Contributor

@elias-ba elias-ba commented Apr 30, 2024

Overview

This PR introduces Generic OAuth 🎉, a set of new features designed to allow users to easily add OAuth credentials by creating re-usable OAuth clients.

  • Firstly, I introduced few tables to the database using Ecto migrations and models, aligning closely with the provided schema (see image below). This foundational work ensures that all necessary tables and relationships are in place to support the functionality that follows.
313514888-5e257db7-7061-4e25-87e1-f02526779a4d
  • Building on this foundation, CRUD operations have been implemented, enabling users to seamlessly create, update, delete, and transfer OAuth clients.

  • To complement these functional changes, a revamped UI has been introduced. This includes a split button that provides easy access to different options, a data table for listing OAuth clients, and detailed forms for managing clients. Special attention has been given to scope management, with new inputs that allow users to edit or delete scope tags effortlessly.

  • When creating an OAuth client, you can make it globally accessible, meaning they can be used by any project within the instance.

  • A new GenericOauthFormComponent was created to facilitate credential creation using OAuth clients, replacing the old OAuthFormComponent. The transition was necessary to provide a better user experience and improve code quality (the old one is using a lot of send_updates and events like that. And it was not necessary at all), ultimately leading to the eventual deprecation of the older component. Furthermore, a custom HTTP client module was created for API calls (OAuthHTTPClient), moving away from the UberOauth library for greater flexibility and control.

  • Testing was a crucial part of this work, with extensive and structured tests ensuring that all features function correctly and reliably.

  • Finally, special care was taken to implement audit logging for OAuth client operations, ensuring that all changes are logged in the Audit table, same as we do with credentials.

In summary, this PR represents a significant enhancement to the OAuth features, providing a more user-friendly, and and extensibility for Lightning to create OAuth connections with systems.

Validation Steps

OAuth Client and Credential Validation Steps

1. Navigate to Credentials Page

  • Go to /credentials or /projects/<project_id>/settings#credentials.

2. Add New OAuth Client

  • Click the Add New split button and select Oauth Client from the dropdown options.
  • Complete the form with valid data to create your OAuth Client. For example, you could set up a Google OAuth client by following this guide.
  • Save the client.

3. Manage the Created OAuth Client

  • Verify that the newly created client appears in the data table.
  • Click the appropriate action button to Edit or Delete the client.

4. Add New Credential Using OAuth Client

  • Click the Add New split button again, but this time select Credential.
  • In the credential type list, locate and select your newly created OAuth client.
  • Click the Configure Credential button.

5. Configure the Credential

  • In the form presented, specify a name for the credential and select the desired scopes from those available.
  • Click the Authorize with 'your client name' button.

6. Complete the Authorization Process

  • You will be redirected to the OAuth2 provider's authentication page.
  • Follow the instructions on that page to complete the authentication process.

7. Confirmation and Profile Information

  • After authentication, you will be redirected back to the app.
  • Verify that your profile information (name and picture) is displayed. Note that this depends on the Userinfo URL you added in your client.

Congratulations 🎉

  • You have successfully created an OAuth2 credential in Lightning.

Notes:

  • During the process, errors related to network issues, incorrect OAuth client configuration, or provider issues may occur. The app will display these errors in an informative way.
  • Adjust your OAuth client data or retry if necessary.

Notes for the reviewer

Related issue

Fixes #2035 #1921 #1920 #1933 #1919 #1900 #1923 #1922

Review checklist

  • I have performed a self-review of my code
  • I have verified that all appropriate authorization policies (:owner, :admin, :editor, :viewer) have been implemented and tested
  • If needed, I have updated the changelog
  • Product has QA'd this feature

Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 97.68575% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 90.74%. Comparing base (a0709fb) to head (4265828).

Files Patch % Lines
...eb/live/credential_live/generic_oauth_component.ex 96.38% 8 Missing ⚠️
lib/lightning/auth_providers/oauth_http_client.ex 85.00% 6 Missing ⚠️
lib/lightning/oauth_clients.ex 94.28% 2 Missing ⚠️
...ive/credential_live/oauth_client_form_component.ex 98.48% 2 Missing ⚠️
lib/lightning_web/live/project_live/settings.ex 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2042      +/-   ##
==========================================
+ Coverage   90.09%   90.74%   +0.65%     
==========================================
  Files         254      264      +10     
  Lines        8286     8937     +651     
==========================================
+ Hits         7465     8110     +645     
- Misses        821      827       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@midigofrank midigofrank left a comment

Choose a reason for hiding this comment

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

Great job @elias-ba 🙌 . I've left a few comments regarding migrations.
Since this is all in one branch, there's no need of having 3 separate migrations all modifying the same table..

Copy link

@christad92 christad92 left a comment

Choose a reason for hiding this comment

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

Thanks for this. Please see my review below:

  • Remove default validation on the text fields when a modal loads - the data validation should only show when a user tries to save or request authorization without providing required information.
  • Allow users to be able to the add credentials to projects from the project settings and user credentials setting.
  • MsGraph not showing fallback image and user info after authorisation
  • Fix the blinking select boxes when adding projects in credential/client set up
  • Ensure that input text boxes are active across the full width of the input box.
  • Remove client ID on the project credentials table to the reduce the width and keep the page compact . Please check this on a smaller screen to see how this looks for users.

https://www.loom.com/share/44e8ab2ed937480ca911d666a252172d

@elias-ba
Copy link
Contributor Author

elias-ba commented May 6, 2024

Review / change requests progress

  • Remove default validation on the text fields when a modal loads - the data validation should only show when a user tries to save or request authorization without providing required information.
  • Allow users to be able to the add credentials to projects from the project settings and user credentials setting.
  • MsGraph not showing fallback image and user info after authorisation
  • Fix the blinking select boxes when adding projects in credential/client set up
  • Ensure that input text boxes are active across the full width of the input box.
  • Remove client ID on the project credentials table to the reduce the width and keep the page compact . Please check this on a smaller screen to see how this looks for users.

assets/js/hooks/index.ts Outdated Show resolved Hide resolved
lib/lightning/projects.ex Outdated Show resolved Hide resolved
@stuartc
Copy link
Member

stuartc commented May 10, 2024

image

@christad92 @elias-ba, some feedback so far 👆

Also if I add only one scope (as in type without entering a ,) and click save it doesn't add it. I mean it's 'minor' and making it better (assuming we want to keep the label chips) would require a little bit of component effort.

It's rather weird having to enter , to add a scope entry.

EDIT ok so 'Permissions' are actually 'scopes', why are they called permissions? That seems odd to me - OAuth doesn't have the concept of permissions, even though scopes often translate to permissions.

@christad92
Copy link

Hey @stuartc, thanks flagging this. I didn't experience this padding and blank section components on end yesterday.

For the chips, users can either use comma or space to create chips. if they do a copy and paste, the input can automatically create chips.

@elias-ba
Copy link
Contributor Author

image

@christad92 @elias-ba, some feedback so far 👆

Also if I add only one scope (as in type without entering a ,) and click save it doesn't add it. I mean it's 'minor' and making it better (assuming we want to keep the label chips) would require a little bit of component effort.

It's rather weird having to enter , to add a scope entry.

EDIT ok so 'Permissions' are actually 'scopes', why are they called permissions? That seems odd to me - OAuth doesn't have the concept of permissions, even though scopes often translate to permissions.

The empty scopes is fixed. It happens when you save a client with no scopes. The fix is rendering the scopes picker component only when the client has scopes.

Copy link
Member

@taylordowns2000 taylordowns2000 left a comment

Choose a reason for hiding this comment

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

Not a specific change request, but a test request: Please ensure that additional fields specified on the oauth client (such as sandbox for Salesforce) for use on the credential are properly handled during create & update of the credential. We had a bug with the previous Salesforce Oauth implementation that made them not get set on updates. See conversation:

Taylor: when you solve this please leave a note on your own generic oauth PR as this will need to be handled there as well. I haven’t tested, but I assume it’s vulnerable to the same bug

Elias: Might be but I don't think so. Generic OAuth is a fresh new component that I created and doesn't necessarily inherit nothing from salesforce. The main reason I did that was to avoid this situations and slowly deprecate salesforce and googlesheet implementations. But yes I will double check !

Taylor: I’m saying that the “additional fields” part of generic oauth, where the user specifies stuff that goes into the credential that isn’t a scope or standard oauth—presumably that is impacted in the same way as the additional fields here for salesforce.

Elias: I heard you. I am just saying that there's a chance yes, but I am also informing you that with generic oauth we created a whole new component with the mindset of having something stronger than how we manage additional fields with salesforce oauth or google sheets. But as I said, I will check it out and confirm.

Taylor: Ah ok, I gotcha now! I’ll paste this Slack conversation into the PR now.

@taylordowns2000
Copy link
Member

    <div class="rounded-md bg-blue-50 p-4">
      <div class="flex">
        <div class="flex-shrink-0">
          <svg
            class="h-5 w-5 text-blue-400"
            viewBox="0 0 20 20"
            fill="currentColor"
            aria-hidden="true"
          >
            <path
              fill-rule="evenodd"
              d="M18 10a8 8 0 11-16 0 8 8 0 0116 0zm-7-4a1 1 0 11-2 0 1 1 0 012 0zM9 9a.75.75 0 000 1.5h.253a.25.25 0 01.244.304l-.459 2.066A1.75 1.75 0 0010.747 15H11a.75.75 0 000-1.5h-.253a.25.25 0 01-.244-.304l.459-2.066A1.75 1.75 0 009.253 9H9z"
              clip-rule="evenodd"
            />
          </svg>
        </div>
        <div class="ml-3 flex-1 md:flex md:justify-between">
          <p class="text-sm text-blue-700">
            That seemed to work, but we couldn't fetch your photo. If that's unusual for your system you can try again, or save the credential and use it as is.
          </p>
          <p class="mt-3 text-sm md:ml-6 md:mt-0">
            <a
              href="#"
              class="whitespace-nowrap font-medium text-blue-700 hover:text-blue-600"
              phx-click="try_userinfo_again"
              phx-target={@myself}
            >
              Try again <span aria-hidden="true"> &rarr;</span>
            </a>
          </p>
          <.helpblock provider={@provider} type={@type} />
        </div>
      </div>
    </div>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @elias-ba , just double checking, did you intend to delete this migration?

Copy link
Collaborator

@midigofrank midigofrank left a comment

Choose a reason for hiding this comment

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

This looks great @elias-ba 🙌 .

@taylordowns2000 taylordowns2000 dismissed christad92’s stale review May 17, 2024 06:06

he approved separately

@taylordowns2000 taylordowns2000 merged commit 118def4 into main May 17, 2024
8 checks passed
@taylordowns2000 taylordowns2000 deleted the 1900-oauth-clients branch May 17, 2024 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

change api_version to the apiVersion in Generic Oauth config.
5 participants