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

Add access control to Organizations #17

Merged
merged 4 commits into from
Jan 12, 2022
Merged

Add access control to Organizations #17

merged 4 commits into from
Jan 12, 2022

Conversation

glrf
Copy link
Contributor

@glrf glrf commented Jan 11, 2022

Summary

Adds access control for organizations. This works by checking with the k8s API server if the action is allowed on a organizations.rbac.appuio.io resource. Watch and list only return objects that the user is also allowed to get.

See https://kb.vshn.ch/appuio-cloud/references/architecture/control-api-org.html#_access_control

OrganizationMembers and RoleBindings will be handled in separate PRs

Checklist

  • PR contains a single logical change (to build a better changelog).
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog.

glrf added 2 commits January 11, 2022 14:15
Also rework abstraction for authorization to reduce code duplication
@glrf glrf self-assigned this Jan 11, 2022
@glrf glrf added the enhancement New feature or request label Jan 12, 2022
@glrf
Copy link
Contributor Author

glrf commented Jan 12, 2022

I may have gone overboard with the test. The actual code change is quite minimal

@glrf glrf marked this pull request as ready for review January 12, 2022 10:56
@glrf glrf requested review from bastjan and ccremer January 12, 2022 12:08
Copy link
Contributor

@ccremer ccremer left a comment

Choose a reason for hiding this comment

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

implementation lgtm

@@ -2,6 +2,8 @@ package organization

Copy link
Contributor

Choose a reason for hiding this comment

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

This file has gotten quite large. Maybe we can split it up based on the verb, if that makes sense?
e.g. organization_get_test.go, organization_update_test.go etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that file got a bit large. I'll try to split it up.

Copy link
Contributor

@bastjan bastjan left a comment

Choose a reason for hiding this comment

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

LGTM

@glrf glrf merged commit 0e47773 into master Jan 12, 2022
@glrf glrf deleted the feat/auth branch January 12, 2022 15:22
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.

3 participants