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(scaffolder): add permission for template management features #26946

Merged

Conversation

stephenglass
Copy link
Contributor

@stephenglass stephenglass commented Oct 3, 2024

Hey, I just made a Pull Request!

I have a need to disable the template management features (dry runner, playground, field explorer, etc). I've found in the Backstage Discord there are others with this same ask as well.

This PR adds a new permission that can be used to disable the template management routes and the "Manage Templates" option from appearing in the scaffolder page context menu.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@stephenglass stephenglass requested review from a team as code owners October 3, 2024 03:21
@github-actions github-actions bot added the area:scaffolder Everything and all things related to the scaffolder project area label Oct 3, 2024
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Oct 3, 2024

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-scaffolder-common plugins/scaffolder-common patch v1.5.6
@backstage/plugin-scaffolder-react plugins/scaffolder-react patch v1.13.3-next.2
@backstage/plugin-scaffolder plugins/scaffolder minor v1.26.3-next.2

Signed-off-by: Stephen Glass <stephen@stephen.glass>
@stephenglass stephenglass force-pushed the scaffolder-permission-manage1 branch from cab58a3 to f61d4cc Compare October 7, 2024 00:19
@benjdlambert
Copy link
Member

benjdlambert commented Oct 7, 2024

@stephenglass thanks for raising this PR!

I think that this is one of many improvement PR's that have been raised over the last few weeks with regards to permissions.

My concern with all of those permission PR's is that they're released in a way that they're not backwards compatible as theres no default policy option for new permissions, so users if using permissions will lose functionality like this until they implement the permissions necessary.

I really want to try and push through us coming up with a fix for providing default policy responses like ALLOW instead of them being DENY so that we can ship these safely without breaking functionality for end users.

Me and @Rugvip spoke about this last week, and thinking that we might get some chance to fix that up this week, so if you don't mind I'd like to get that in before proceeding with this and #25969 and #26947 🙏

@vinzscam
Copy link
Member

@stephenglass thanks for raising this PR!

I think that this is one of many improvement PR's that have been raised over the last few weeks with regards to permissions.

My concern with all of those permission PR's is that they're released in a way that they're not backwards compatible as theres no default policy option for new permissions, so users if using permissions will lose functionality like this until they implement the permissions necessary.

I really want to try and push through us coming up with a fix for providing default policy responses like ALLOW instead of them being DENY so that we can ship these safely without breaking functionality for end users.

Me and @Rugvip spoke about this last week, and thinking that we might get some chance to fix that up this week, so if you don't mind I'd like to get that in before proceeding with this and #25969 and #26947 🙏

mmm I don't think this is a concern as the recommendation is to ALLOW unknown permissions and this is usually the pattern that people take. So in this case, people won't loose functionalities if a new permission is added.

Signed-off-by: Stephen Glass <stephen@stephen.glass>
Signed-off-by: Stephen Glass <stephen@stephen.glass>
@stephenglass
Copy link
Contributor Author

Added templateManagementPermission permission check to the /v2/dry-run endpoint since IMO it makes sense if the only way you can call the endpoint via the UI is using the template dry runner, which is being protected under the same permission check.

@acierto
Copy link
Contributor

acierto commented Oct 19, 2024

Hey @stephenglass 👋

Can you please solve conflicts?

…scaffolder-permission-manage1

Signed-off-by: Stephen Glass <stephen@stephen.glass>
@stephenglass
Copy link
Contributor Author

Hey @stephenglass 👋

Can you please solve conflicts?

@acierto Done! Thanks!

@@ -766,7 +767,7 @@ export async function createRouter(
const credentials = await httpAuth.credentials(req);
await checkPermission({
credentials,
permissions: [taskCreatePermission],
permissions: [taskCreatePermission, templateManagementPermission],
Copy link
Member

Choose a reason for hiding this comment

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

let's keep the two concept separated as you suggested!

Suggested change
permissions: [taskCreatePermission, templateManagementPermission],
permissions: [taskCreatePermission],

---
'@backstage/plugin-scaffolder-common': patch
'@backstage/plugin-scaffolder-react': patch
'@backstage/plugin-scaffolder-backend': patch
Copy link
Member

Choose a reason for hiding this comment

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

then this one should go away if we don't make any changes to scaffolder-backend

Suggested change
'@backstage/plugin-scaffolder-backend': patch

'@backstage/plugin-scaffolder-common': patch
'@backstage/plugin-scaffolder-react': patch
'@backstage/plugin-scaffolder-backend': patch
'@backstage/plugin-scaffolder': patch
Copy link
Member

Choose a reason for hiding this comment

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

@acierto would this be a minor then? 🤔

Suggested change
'@backstage/plugin-scaffolder': patch
'@backstage/plugin-scaffolder': minor

Signed-off-by: Stephen Glass <stephen@stephen.glass>
@stephenglass
Copy link
Contributor Author

@vinzscam I've addressed the comments 👍

plugins/scaffolder-common/src/permissions.ts Outdated Show resolved Hide resolved
plugins/scaffolder-common/src/permissions.ts Outdated Show resolved Hide resolved
stephenglass and others added 2 commits November 6, 2024 15:45
Co-authored-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Stephen Glass <stephen@stephen.glass>
Signed-off-by: Stephen Glass <stephen@stephen.glass>
Copy link
Member

@vinzscam vinzscam left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@benjdlambert benjdlambert left a comment

Choose a reason for hiding this comment

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

Thanks @stephenglass! 🎉

@benjdlambert benjdlambert merged commit f969062 into backstage:master Nov 7, 2024
24 checks passed
Copy link
Contributor

github-actions bot commented Nov 7, 2024

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.33.0 release, scheduled for Tue, 19 Nov 2024.

@awanlin
Copy link
Collaborator

awanlin commented Nov 7, 2024

Hi @stephenglass, a little late here but any chance you could submit a follow up PR with some documentation around the new permissions, please? This isn't a great spot for them but for now it's the one that makes the most sense to me: https://backstage.io/docs/features/software-templates/authorizing-scaffolder-template-details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:scaffolder Everything and all things related to the scaffolder project area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants