Add granular RBAC SSO support#7733
Conversation
fixes #7396 Includes both SMAL and LDAP Also includes a fix for the direct SSO login buttons on login page
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7733 +/- ##
==========================================
- Coverage 75.39% 74.96% -0.44%
==========================================
Files 428 429 +1
Lines 22540 22790 +250
Branches 5945 6032 +87
==========================================
+ Hits 16994 17084 +90
- Misses 5546 5706 +160
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
knolleary
left a comment
knolleary
left a comment
There was a problem hiding this comment.
I've done a first pass and spotted a couple things.
It would be helpful to have some more inline comments (similar to the existing code) to help make the logic more explicit.
I need to spend some more time testing it to verify the behaviour.
| hasApplication: async function (nameOrId) { | ||
| const applicationId = M.Application.decodeHashid(nameOrId) | ||
| const application = await app.db.models.Application.findOne({ | ||
| where: { |
There was a problem hiding this comment.
Missing a where clause to ensure its an Application in this team.
| if (application) { | ||
| if (providerOpts.groupAllTeams || (providerOpts.groupTeams || []).includes(teamSlug)) { | ||
| if (desiredTeamApplicationroles[team.hashid]) { | ||
| desiredTeamApplicationroles[team.hashid][application.hashid] = Math.max(desiredTeamApplicationroles[teamSlug][application.hashid] || 0, applicationRole) |
There was a problem hiding this comment.
There is a mix of team.hashid and teamSlug when keying into desiredTeamApplicationroles to read from it, but its only written to via team.hashid. Need to verify if that's intentional.
There was a problem hiding this comment.
yeah, they probably should all be teamSlug
Given it's at the team level and we only ever use slugs for team ids in group names it should be OK. But I didn't test with 2 groups for the same application (one name, one hash id) with different role values, I will do that and push needed changes.
@knolleary fixed things based on first pass comments
2 participants
fixes #7396
Description
Includes both SMAL and LDAP
Also includes a fix for the direct SSO login buttons on login page
Related Issue(s)
#7396
Checklist
flowforge.yml?FlowFuse/helmto update ConfigMap TemplateFlowFuse/CloudProjectto update values for Staging/ProductionLabels
area:migrationlabel