-
Notifications
You must be signed in to change notification settings - Fork 31
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
Conversation
9645de9
to
6074861
Compare
7856bdc
to
56db0db
Compare
Codecov ReportAttention: Patch coverage is
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. |
There was a problem hiding this 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..
...epo/migrations/20240417031421_update_oauth_clients_table_to_rename_url_and_add_endpoints.exs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
Review / change requests progress
|
@christad92 @elias-ba, some feedback so far 👆 Also if I add only one scope (as in type without entering a It's rather weird having to enter 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. |
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. |
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. |
There was a problem hiding this 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.
<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"> →</span>
</a>
</p>
<.helpblock provider={@provider} type={@type} />
</div>
</div>
</div> |
371a5d3
to
96295ed
Compare
… will render a warning banner
96295ed
to
1faa6b9
Compare
There was a problem hiding this comment.
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?
There was a problem hiding this 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 🙌 .
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.
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 oldOAuthFormComponent
. The transition was necessary to provide a better user experience and improve code quality (the old one is using a lot ofsend_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 theUberOauth
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
/credentials
or/projects/<project_id>/settings#credentials
.2. Add New OAuth Client
3. Manage the Created OAuth Client
4. Add New Credential Using OAuth Client
5. Configure the Credential
6. Complete the Authorization Process
7. Confirmation and Profile Information
Congratulations 🎉
Notes:
Notes for the reviewer
Related issue
Fixes #2035 #1921 #1920 #1933 #1919 #1900 #1923 #1922
Review checklist
:owner
,:admin
,:editor
,:viewer
) have been implemented and tested