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

AuthenticationMap role #407

Merged
merged 4 commits into from
Jun 5, 2024
Merged

AuthenticationMap role #407

merged 4 commits into from
Jun 5, 2024

Conversation

slemrmartin
Copy link
Contributor

@slemrmartin slemrmartin commented May 21, 2024 •
edited
Loading

This PR adds a role field to AuthenticationMap and map_type == 'role'

Role field is relation to RBAC's RoleDefinition through name (it's a CharField), so it's not a real foreign key

Conditions:

  • If map_type is organization, team, or role
    • role field is mandatory
  • if map_type is organization
    • role's content type must be 'organization'
  • if map_type is team
    • role's content type must be 'team'
  • if map_type is role
    • role's content type can be any or blank
  • If role's content type is organization:
    • fields organization can't be blank
  • If role's content type is team:
    • fields team and organization can't be blank

@slemrmartin slemrmartin self-assigned this May 21, 2024
@slemrmartin slemrmartin changed the title AuthenticationMap role [WIP] AuthenticationMap role May 21, 2024
@slemrmartin slemrmartin force-pushed the auth-map-role branch 4 times, most recently from e3427d0 to 0969d02 Compare May 22, 2024 09:55
@slemrmartin slemrmartin changed the title [WIP] AuthenticationMap role AuthenticationMap role May 22, 2024
@slemrmartin slemrmartin force-pushed the auth-map-role branch 2 times, most recently from e74490c to daf46fb Compare May 22, 2024 11:11
@john-westcott-iv john-westcott-iv added the comments left This PR was reviewed with comments label May 22, 2024
@AlanCoding
Copy link
Member

My very-fast gut reaction to this is:

    organization = models.CharField(
        max_length=512,
        null=True,
        default=None,
        blank=True,

A related RoleDefinition is similar in nature to a related Organization. This model doesn't have a ForeignKey link to the organization model due to the massive complexities that would introduce. It just keeps the name of it. I would favor a solution like that for roles as well.

However, we need to talk more about the proposal for a secondary identifier for role definitions, before we commit to using names. Upstream discussion I started is in #408, which doesn't get into custom roles, but that needs to be a part of this too.

@slemrmartin
Copy link
Contributor Author

@AlanCoding if I'm correct, we need to make this compatible with hub, which uses pulp rbac atm. So both RoleDefinition and ManagedRoleDefinition introduces the same issue, which is RBAC dependency. Thus using the role as a name is not a question for this version.

Anyway I need to clarify the 2nd paragraph of your previous comment, what does it mean

@slemrmartin slemrmartin added Ready for review This PR is ready for review either initially or comments have been address and removed comments left This PR was reviewed with comments labels May 28, 2024
@slemrmartin slemrmartin force-pushed the auth-map-role branch 5 times, most recently from eb402ab to ce86cb5 Compare May 28, 2024 10:54
john-westcott-iv
john-westcott-iv reviewed May 30, 2024
View reviewed changes
ansible_base/authentication/models/authenticator_map.py Outdated Show resolved Hide resolved
ansible_base/authentication/models/authenticator_map.py Show resolved Hide resolved
ansible_base/authentication/serializers/authenticator_map.py Outdated Show resolved Hide resolved
ansible_base/authentication/serializers/authenticator_map.py Outdated Show resolved Hide resolved
ansible_base/authentication/serializers/authenticator_map.py Show resolved Hide resolved
ansible_base/authentication/serializers/authenticator_map.py Outdated Show resolved Hide resolved
ansible_base/authentication/serializers/authenticator_map.py Outdated Show resolved Hide resolved
ansible_base/authentication/serializers/authenticator_map.py Show resolved Hide resolved
test_app/tests/authentication/serializers/test_authententicator_map.py Outdated Show resolved Hide resolved
docs/apps/authentication/authentication.md Outdated Show resolved Hide resolved
@slemrmartin slemrmartin force-pushed the auth-map-role branch 4 times, most recently from 7b4dbe1 to 3563eb6 Compare June 4, 2024 08:39
@slemrmartin slemrmartin mentioned this pull request Jun 4, 2024
Merged
AlanCoding
AlanCoding approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@AlanCoding AlanCoding left a comment

Choose a reason for hiding this comment

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

I see the final bit for functionality is planned after this to fully apply the permissions. So I'm on board with the plan finally.

I left some minor comments unresolved that are minor organizational or aesthetic suggestions.

@slemrmartin slemrmartin force-pushed the auth-map-role branch 2 times, most recently from 6f384f7 to 59fb0b8 Compare June 5, 2024 10:37
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 5, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
92.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@slemrmartin slemrmartin merged commit 379802b into ansible:devel Jun 5, 2024
8 checks passed
@slemrmartin slemrmartin deleted the auth-map-role branch June 5, 2024 11:07
@slemrmartin slemrmartin removed the Ready for review This PR is ready for review either initially or comments have been address label Jun 5, 2024
relrod added a commit to relrod/django-ansible-base that referenced this pull request Jun 6, 2024
@relrod
Add missimg migration from ansible#407
af9b9d8 
Signed-off-by: Rick Elrod <rick@elrod.me>
@relrod relrod mentioned this pull request Jun 6, 2024
Merged
relrod added a commit that referenced this pull request Jun 6, 2024
@relrod
Add missimg migration from #407
5663b33 
Signed-off-by: Rick Elrod <rick@elrod.me>
slemrmartin added a commit that referenced this pull request Jun 11, 2024
@slemrmartin
Reconcile User Claims with AuthMap roles (#431)
a791568 
- **depends on** #407 

This PR applies AuthenticationMap RBAC roles to the Reconcile User
Claims.

The AuthenticatorMap.remove_users = True now removes all
role_user_assignments (it used to removed only Team Member, Organization
Member)
thedoubl3j pushed a commit to thedoubl3j/django-ansible-base that referenced this pull request Jun 18, 2024
@slemrmartin @thedoubl3j
AuthenticationMap role (ansible#407)
b22e1bf 
This PR adds a `role` field to AuthenticationMap and `map_type ==
'role'`

Role field is relation to RBAC's RoleDefinition through name (it's a
CharField), so it's not a real foreign key

Conditions:

---

- If map_type is `organization`, `team`, or `role`
  - `role` field is mandatory

---

- if map_type is `organization`
  - role's content type must be 'organization'
- if map_type is `team`
  -  role's content type must be 'team'
- if map_type is `role`
  -  role's content type can be any or blank

---

- If role's content type is `organization`:
  - fields `organization` can't be blank
- If role's content type is `team`:
  - fields `team` and `organization` can't be blank
thedoubl3j pushed a commit to thedoubl3j/django-ansible-base that referenced this pull request Jun 18, 2024
@relrod @thedoubl3j
Add missimg migration from ansible#407
560c00b 
Signed-off-by: Rick Elrod <rick@elrod.me>
thedoubl3j pushed a commit to thedoubl3j/django-ansible-base that referenced this pull request Jun 18, 2024
@slemrmartin @thedoubl3j
Reconcile User Claims with AuthMap roles (ansible#431)
e75c1a2 
- **depends on** ansible#407

This PR applies AuthenticationMap RBAC roles to the Reconcile User
Claims.

The AuthenticatorMap.remove_users = True now removes all
role_user_assignments (it used to removed only Team Member, Organization
Member)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlanCoding AlanCoding AlanCoding approved these changes

@john-westcott-iv john-westcott-iv john-westcott-iv left review comments

Assignees

@AlanCoding AlanCoding

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@slemrmartin @AlanCoding @john-westcott-iv