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

feat: Custom Applications multiple permissions #2799

Merged
merged 12 commits into from
Dec 9, 2022

Conversation

kark
Copy link
Contributor

@kark kark commented Sep 7, 2022

https://jira.commercetools.com/browse/SHIELD-580

In preparation for the Custom Applications Multiple Permissions support.

@changeset-bot
Copy link

changeset-bot bot commented Sep 7, 2022

🦋 Changeset detected

Latest commit: 86ff5a8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 35 packages
Name Type
@commercetools-frontend/application-config Minor
@commercetools-frontend/application-shell Minor
@commercetools-frontend/constants Minor
@commercetools-frontend/cypress Minor
@commercetools-frontend/mc-scripts Minor
@commercetools-frontend/mc-dev-authentication Minor
@commercetools-frontend/mc-html-template Minor
@commercetools-applications/merchant-center-template-starter-typescript Minor
@commercetools-applications/merchant-center-template-starter Minor
@commercetools-local/playground Minor
@commercetools-frontend/actions-global Minor
@commercetools-frontend/application-components Minor
@commercetools-frontend/application-shell-connectors Minor
@commercetools-frontend/react-notifications Minor
@commercetools-frontend/sdk Minor
@commercetools-frontend/sentry Minor
@commercetools-local/visual-testing-app Minor
@commercetools-website/components-playground Minor
@commercetools-frontend/permissions Minor
@commercetools-frontend/i18n Minor
@commercetools-frontend/l10n Minor
@commercetools-backend/eslint-config-node Minor
@commercetools-backend/express Minor
@commercetools-backend/loggers Minor
@commercetools-frontend/assets Minor
@commercetools-frontend/babel-preset-mc-app Minor
@commercetools-frontend/browser-history Minor
@commercetools-frontend/codemod Minor
@commercetools-frontend/create-mc-app Minor
@commercetools-frontend/eslint-config-mc-app Minor
@commercetools-frontend/jest-preset-mc-app Minor
@commercetools-frontend/jest-stylelint-runner Minor
@commercetools-frontend/notifications Minor
@commercetools-frontend/url-utils Minor
@commercetools-website/custom-applications Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

Deploy preview for merchant-center-application-kit ready!

✅ Preview
https://merchant-center-application-nc0pepxzh-commercetools.vercel.app
https://appkit-sha-9b937d63a03d0628f86af44dbdd65b6aa43c2fb6.commercetools.vercel.app
https://appkit-pr-2799.commercetools.vercel.app

Built with commit 86ff5a8.
This pull request is being automatically deployed with vercel-action

@kark kark force-pushed the SHIELD-580-additionalOAuthScopes-field branch from 66c0e0f to 3caad79 Compare September 9, 2022 08:46
@kark kark marked this pull request as ready for review September 9, 2022 11:36
@kark kark requested a review from a team September 9, 2022 11:36
Copy link
Contributor

@CarlosCortizasCT CarlosCortizasCT 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 very promising 💪

I just left some comments with some questions/suggestions.

'@commercetools-frontend/application-config': patch
---

Define new field `additionalOAuthScopes` in the Custom Application config.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we describe what this new field should be use for?
Or maybe add a link to the updated documentation?

website/src/content/api-reference/application-config.mdx Outdated Show resolved Hide resolved
website/src/content/api-reference/application-config.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@Rhotimee Rhotimee left a comment

Choose a reason for hiding this comment

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

Looks good. Just some nitpicking.


## `additionalOAuthScopes.view`

A list of "view-only" [OAuth Scopes](https://docs.commercetools.com/api/scopes) required by the Custom Application and associated to the `View` permission.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A list of "view-only" [OAuth Scopes](https://docs.commercetools.com/api/scopes) required by the Custom Application and associated to the `View` permission.
A list of "view-only" [OAuth Scopes](https://docs.commercetools.com/api/scopes) required by the Custom Application and associated with the additionalOAuthScopes `View` permission.


## `additionalOAuthScopes.manage`

A list of "manage-only" [OAuth Scopes](https://docs.commercetools.com/api/scopes) required by the Custom Application and associated with the `Manage` permission.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A list of "manage-only" [OAuth Scopes](https://docs.commercetools.com/api/scopes) required by the Custom Application and associated with the `Manage` permission.
A list of "manage-only" [OAuth Scopes](https://docs.commercetools.com/api/scopes) required by the Custom Application and associated with the additionalOAuthScopes `Manage` permission.

@@ -30,6 +30,37 @@ Notice here how the OAuth Scopes are grouped by the two fields `view` and `manag

This grouping determines the **mapping and relation between OAuth Scopes and user permissions**.

# Additional OAuth Scopes

Defining `oAuthScopes` in the Custom Application config allows to use permissions limited to 1 unique pair (view/manage) specific to the Custom Application.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Defining `oAuthScopes` in the Custom Application config allows to use permissions limited to 1 unique pair (view/manage) specific to the Custom Application.
Defining `oAuthScopes` in the Custom Application config allows using permissions limited to 1 unique pair (view/manage) specific to the Custom Application.


Defining `oAuthScopes` in the Custom Application config allows to use permissions limited to 1 unique pair (view/manage) specific to the Custom Application.

For more granular permissions, for example to allow team access to only certain parts or functionality of the Custom Application, [additional OAuth Scopes](https://docs.commercetools.com/api/scopes) can be requested as part of various permission groups.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For more granular permissions, for example to allow team access to only certain parts or functionality of the Custom Application, [additional OAuth Scopes](https://docs.commercetools.com/api/scopes) can be requested as part of various permission groups.
For more granular permissions, for example, to allow the team access to only certain parts or functionality of the Custom Application, [additional OAuth Scopes](https://docs.commercetools.com/api/scopes) can be requested as part of various permission groups.

@emmenko emmenko marked this pull request as draft September 12, 2022 11:08
@emmenko
Copy link
Member

emmenko commented Sep 12, 2022

FYI: marking this PR as draft/blocked as we shouldn't merge this yet, only when it's time to release the feature. We can use this as the base branch I suppose.

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Have you had a chance to test the OIDC flow locally?

@@ -0,0 +1,5 @@
---
'@commercetools-frontend/application-config': patch
Copy link
Member

Choose a reason for hiding this comment

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

Nit: new features usually should be considered a minor version

.changeset/fresh-coins-shout.md Show resolved Hide resolved
@@ -327,7 +327,7 @@ You can have "view-only" or "manage-only" OAuth Scopes and leave the other list

## `oAuthScopes.view`

A list of "view-only" [OAuth Scopes](https://docs.commercetools.com/api/scopes) required by the Custom Application and associated to the `View` permission.
A list of "view-only" [OAuth Scopes](https://docs.commercetools.com/api/scopes) required by the Custom Application and associated with the `View` permission.
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer the documentation changes to be defined in a separate PR. Main reason being that merging this PR will publish the docs changes but the packages are not being released yet.

So the docs changes can be merged separately once the packages have been released.

Can you extract these docs changes into a separate PR? Thanks 🙂

@kark kark force-pushed the SHIELD-580-additionalOAuthScopes-field branch from 5e11208 to 564f59a Compare September 15, 2022 11:53
@kark kark changed the title refactor(config): define new field additionalOAuthScopes in the Custom Application config feat: Custom Applications multiple permissions Sep 15, 2022
@emmenko emmenko added the 🚀 Type: New Feature Something new label Nov 8, 2022
@kark kark force-pushed the SHIELD-580-additionalOAuthScopes-field branch 2 times, most recently from 2e06827 to 55b0790 Compare November 22, 2022 07:50
@kark kark marked this pull request as ready for review November 24, 2022 07:59
Copy link
Contributor

@CarlosCortizasCT CarlosCortizasCT left a comment

Choose a reason for hiding this comment

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

Awesome job 🚀

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

🚀

@emmenko emmenko merged commit 033d95e into main Dec 9, 2022
@emmenko emmenko deleted the SHIELD-580-additionalOAuthScopes-field branch December 9, 2022 10:19
@ghost ghost mentioned this pull request Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants