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

[FEATURE] allow grant type to be specified for client:auth #499

Conversation

sandragolden
Copy link
Contributor

@sandragolden sandragolden commented Apr 26, 2023

closes #245

I do not see this as a breaking change because everything will continue to work normally but we have a new option that will allow you to force client_credentials grant type

Related to Resource Owner Password Credentials Authentication Failure

What PR adjusts:
Allows grant type to be specified for client:auth. For example: sfcc-ci client:auth -t client_credentials. Default grant type will remain unchanged and is set to password by default.

Why?
I'm running a local project where I'm running into the Error: Authentication failed: Resource owner authentication failed mentioned in the related doc above. Because I have a dw.json file in my project, the grant type of password is always being used. Due to my Account Manager setup, this is throwing an error. If I force the grant type of client_credentials, everything works as expected because I've added the necessary permissions to my client.

@sandragolden sandragolden marked this pull request as draft April 26, 2023 00:10
@sandragolden sandragolden marked this pull request as ready for review April 26, 2023 00:30
@clavery
Copy link
Contributor

clavery commented Oct 17, 2023

@tobiaslohr Could we get this into the next version? It's a pitfall many folks run into when using sfcc-ci with a dw.json file present in the same dir.

@jlbruno
Copy link
Member

jlbruno commented Nov 10, 2023

Any chance of getting this fix merged?

@sandragolden
Copy link
Contributor Author

@tobiaslohr I just updated the branch with the latest from master, any chance we can get this merged now?

@SGD1953 SGD1953 self-requested a review January 26, 2024 12:29
Copy link
Contributor

@SGD1953 SGD1953 left a comment

Choose a reason for hiding this comment

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

Good stuff!

@jlbruno
Copy link
Member

jlbruno commented Aug 1, 2024

Been a while since I've checked in on this. @tobiaslohr mentioned on Slack that anyone with admin rights can merge PRs - can someone else maybe get this merged in and get a release out with this fix?

@jlbruno
Copy link
Member

jlbruno commented Aug 1, 2024

Looks like this is still coming up from other users as an issue in Slack:
https://sfcc-unofficial.slack.com/archives/CAUFG3SHF/p1716557209126099?thread_ts=1716556187.213669&cid=CAUFG3SHF
https://sfcc-unofficial.slack.com/archives/CAUFG3SHF/p1711142480505479?thread_ts=1711134237.260059&cid=CAUFG3SHF

@tobiaslohr how do we get @sandragolden or @clavery to be admins on this project if your other work priorities aren't letting you maintain this project anymore? SFCC-CI is a tool that many rely on and it seems as thought Salesforce has simply abandoned its users at this point.

@tobiaslohr
Copy link
Contributor

Hi @sandragolden, thanks a lot for contributing!

We can merge this. However, the way how we approach this is not ideal. It should not be up to the user of the CLI to tell it which grant type to use. This should be up to the tool to determine. For this particular case, I suggest we simply stop using the user credentials from the dw.json to make a token request with resource_owner_password_credentials grant.

Reason being is, that neither AM nor any Commerce application require the use of this grant type any longer. It's also discouraged for security reasons.

My recommendation is we merge the PR, but we adjust this to (remove the new argument -t client_credentials) and stop reading the user credentials from the dw.json. If you are ok with this, let me know. Afterwards we have to build a new major version, as this is a not backwards compatible.

@tobiaslohr tobiaslohr self-assigned this Sep 13, 2024
@sandragolden
Copy link
Contributor Author

sandragolden commented Sep 13, 2024

My recommendation is we merge the PR, but we adjust this to (remove the new argument -t client_credentials) and stop reading the user credentials from the dw.json. If you are ok with this, let me know. Afterwards we have to build a new major version, as this is a not backwards compatible.

This sounds great to me, many thanks for reviewing and adjusting @tobiaslohr

@tobiaslohr tobiaslohr merged commit 40599b0 into SalesforceCommerceCloud:master Sep 24, 2024
4 checks passed
tobiaslohr added a commit that referenced this pull request Sep 24, 2024
@tobiaslohr
Copy link
Contributor

For the records: by changing the working directory which does not contain the dw.json and execute the command from there. Then it does not pick up the user credentials from the dw.json, as it's not finding it any longer.

We are not following my previous suggestion at #499 (comment) in order to build this in a backwards compatible manner as this had been requested by folks. In a next major version we will remove the Resource Owner Password Credentials grant entirely.

@tobiaslohr tobiaslohr added the enhancement New feature or request label Sep 24, 2024
@tobiaslohr tobiaslohr added this to the 2.12.0 milestone Sep 24, 2024
@tobiaslohr
Copy link
Contributor

Closes #200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow client:auth to force client_credential grant
7 participants