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.

Sign up for GitHub

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

Jump to bottom

feat: OAuth 2.0 Client Credentials Basic Auth #2164

Open
wants to merge 4 commits into
base: fix/oauth2
Choose a base branch
from
Open

feat: OAuth 2.0 Client Credentials Basic Auth #2164

wants to merge 4 commits into from
+247 −40

Conversation

pietrygamat
Copy link
Contributor

@pietrygamat pietrygamat commented Apr 25, 2024 •
edited
Loading

Description

With this change the OAuth2 authorization on request and collection level is now including additional configuration option for handling client credentials when accessing token endpoint. This applies to all currently supported grant_types.

The new default option is to send the credentials as Authorization: Basic header:

auth:oauth2 {
  ...
  client_id: brunoclient
  client_secret: {{client_secret}}
  client_secret_method: client_credentials_basic
}

The old behavior to include them in the request body is still available.

auth:oauth2 {
  ...
  client_id: brunoclient
  client_secret: {{client_secret}}
  client_secret_method: client_credentials_post
}

resolves #2106
#1003

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.

Publishing to New Package Managers

Please see here for more information.

@pietrygamat pietrygamat changed the title Feature/oauth2 basic auth feat: OAuth 2.0 Client Credentials Basic Auth Apr 25, 2024
@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch 2 times, most recently from 8a24c7f to 684a4bb Compare April 25, 2024 07:55
pietrygamat
pietrygamat commented Apr 25, 2024
View reviewed changes
...es/bruno-app/src/components/RequestPane/Auth/OAuth2/ClientCredentialsMethodSelector/index.js
import { useDispatch } from 'react-redux';
import { humanizeOAuth2ClientSecretMethod } from 'utils/collections';

const ClientCredentialsMethodSelector = ({ item, collection, oAuth }) => {
Copy link
Contributor Author

@pietrygamat pietrygamat Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The same component is used in RequestPane and in CollectionSettings/Auth. While the convention in the project seems to be using duplicate, almost identical components, I feel this approach is easier to manage. Perhaps it should be moved into a different location then?

Copy link
Contributor

Choose a reason for hiding this comment

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

seconded - perhaps we should have a shared components directory

packages/bruno-electron/src/ipc/network/interpolate-vars.js Outdated
Comment on lines 126 to 180
request.data = {
grant_type: 'password',
username,
password,
client_id: clientId,
client_secret: clientSecret,
scope
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am pretty sure the interpolate-vars has no business updating request.data like this. The oauth2-helper knows best what is and what is not to be included in request data, right?

@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch from c4c8e2b to 54527fe Compare April 25, 2024 08:40
@pietrygamat pietrygamat mentioned this pull request May 5, 2024
Open
5 tasks
@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch 2 times, most recently from 65cc7cc to 050b588 Compare May 5, 2024 23:44
@pietrygamat pietrygamat mentioned this pull request May 6, 2024
Draft
@rmaheedharan
Copy link

Much Needed!

@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch from 050b588 to 6e47aff Compare May 22, 2024 16:21
@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch 4 times, most recently from 636f545 to 99a82de Compare June 5, 2024 19:40
@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch from 99a82de to ab91302 Compare June 21, 2024 19:45
@pietrygamat
Copy link
Contributor Author

Rebased and slightly simplified. It should be more clear what is happening in oauth2 helper.

@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch from ab91302 to 61e36b4 Compare June 21, 2024 20:52
@RolphH
Copy link

RolphH commented Jul 8, 2024

Looks good, when can we expect this to be merged (and available in the main package)?

@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch from 61e36b4 to a137f95 Compare July 15, 2024 21:06
@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch from a137f95 to 47c4eeb Compare July 26, 2024 17:29
@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch from 47c4eeb to 316f8fd Compare August 22, 2024 21:50
@helloanoop
Copy link
Contributor

This is a significant PR, and we need some time to thoroughly review it. Enhancing the OAuth 2.0 interface is a top priority for us in September 2024. We’ll provide more updates in the coming days.

@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch from 316f8fd to 06dae8f Compare September 3, 2024 17:28
pietrygamat
pietrygamat commented Sep 3, 2024
View reviewed changes
packages/bruno-app/src/utils/collections/index.js
Comment on lines +689 to +750
export const humanizeOAuth2ClientSecretMethod = (mode) => {
let label = 'N/A';
switch (mode) {
case 'client_credentials_basic': {
label = 'As Basic Auth Header';
break;
}
case 'client_credentials_post': {
label = 'In Request Body';
break;
}
}

return label;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: move to i18n/translation, but this should best be done in separate PR

@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch from 06dae8f to 23cfb49 Compare September 23, 2024 17:17
@tbskp
Copy link

tbskp commented Nov 20, 2024

This is a significant PR, and we need some time to thoroughly review it. Enhancing the OAuth 2.0 interface is a top priority for us in September 2024. We’ll provide more updates in the coming days.

May I ask if there are any news on this? :-)

@helloanoop helloanoop mentioned this pull request Nov 28, 2024
Open
@pietrygamat
feat: OAuth2 - Client Credentials as Basic Auth Header - bru data model
5fe4e69 
Adds additional configuration field for oauth2 in bru schema, allowing users
to decide whether to include client credentials in request's Authorization
header or in its body as application/x-www-form-urlencoded parameters
@pietrygamat
feat: OAuth2 - Client Credentials as Basic Auth Header - request logic
c055a43 
Update OAuth2 request logic to make use of client_secret_method option
@pietrygamat
feat: OAuth2 - Client Credentials as Basic Auth Header - user interface
b312193 
Add GUI controls for selecting client_secret_method
@pietrygamat
feat: OAuth2 - Client Credentials as Basic Auth Header - update test …
f434d63 
…data
@pietrygamat pietrygamat force-pushed the feature/oauth2-basic-auth branch from 23cfb49 to f434d63 Compare January 11, 2025 18:30
@pietrygamat pietrygamat changed the base branch from main to feature/oauth2.0 January 11, 2025 18:30
@pietrygamat pietrygamat changed the base branch from feature/oauth2.0 to fix/oauth2 January 11, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizziemac lizziemac lizziemac left review comments

Assignees

@lohxt1 lohxt1

Labels
pr-feature size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OAuth 2.0 Client Credentials Basic Auth
8 participants
@pietrygamat @rmaheedharan @RolphH @helloanoop @tbskp @lizziemac @lohxt1 @sanjai0py