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

PR: Get and Cache List of Roles + Helper Functions: has_role?/2 #7

Merged
merged 24 commits into from
Sep 17, 2020

Conversation

nelsonic
Copy link
Member

@nelsonic nelsonic commented Sep 13, 2020

This Pull Request adds:

  • functions to check if a person has a role: RBAC.has_role?/2 (String) and RBAC.has_role_any?/2 (List)
  • Code to fetch the list of roles from Auth App and cache them in ETS: Get List of Roles for App  auth#110

@nelsonic nelsonic added enhancement New feature or request in-progress labels Sep 13, 2020
@nelsonic nelsonic self-assigned this Sep 13, 2020
@nelsonic nelsonic changed the title Get List of Roles PR: Get and Cache List of Roles Sep 14, 2020
@nelsonic nelsonic mentioned this pull request Sep 14, 2020
5 tasks
@codecov
Copy link

codecov bot commented Sep 14, 2020

Codecov Report

Merging #7 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master        #7   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            4        38   +34     
=========================================
+ Hits             4        38   +34     
Impacted Files Coverage Δ
lib/rbac.ex 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0743253...ad51cc1. Read the comment docs.

@nelsonic nelsonic marked this pull request as ready for review September 14, 2020 20:50
@nelsonic
Copy link
Member Author

@th0mas this PR is in a reviewable state. 🔍 🙏
It's reasonably independent from dwyl/auth#85 as it already works. ✅
Please take a look at the code and if you want to improve any of it, go for it! 👍
Thanks. ☀️

@nelsonic nelsonic assigned th0mas and unassigned nelsonic Sep 14, 2020
@nelsonic nelsonic requested a review from th0mas September 14, 2020 20:55
README.md Show resolved Hide resolved
lib/rbac.ex Outdated Show resolved Hide resolved
lib/rbac.ex Outdated Show resolved Hide resolved
lib/rbac.ex Outdated Show resolved Hide resolved
lib/rbac.ex Outdated Show resolved Hide resolved
lib/rbac.ex Outdated Show resolved Hide resolved
@th0mas th0mas mentioned this pull request Sep 15, 2020
@th0mas
Copy link
Collaborator

th0mas commented Sep 15, 2020

@nelsonic can you give me write access to this branch so I can push some of my refactoring commits?

@nelsonic
Copy link
Member Author

@th0mas please confirm you have write access. Thanks. 🔑
image

@th0mas
Copy link
Collaborator

th0mas commented Sep 15, 2020

@nelsonic got it cheers

@nelsonic
Copy link
Member Author

@th0mas I have made a few updates addressing all your PR review comments.
I think the code is now more versatile and there's a clear reason for having a separate re-useable module/repo.

RBAC no longer makes any assumption of using auth or auth_plug, it can be used 100% independently/offline.

You can call RBAC.has_role?([1,14,31], "admin") with a List of Integers as the first argument,
but I have left the RBAC.has_role?(conn, "admin") in as a convenience, with the appropriate transformation behind the scenes. Which means the Channels use case is still available (not depending on Plug.Conn).
I want to write RBAC.has_role_any?(conn, [:superadmin, :admin]) as I feel this code is brief but descriptive. 🤔

Please take another look at the PR when you're back at your desk tomorrow. 🙏

Note: get_role_from_cache/1 now logs an error with a stack trace so devs can fix their role name typos. 📣
get_role_from_cache/1 is only used internally but the function is public in case anyone wants to use it.
If you prefer to return an explicit :error in get_role_from_cache/1 we need to handle it in the calling function. 💭

LMK your thoughts. 👍

@th0mas
Copy link
Collaborator

th0mas commented Sep 17, 2020

Looks good 👍

@th0mas th0mas merged commit 57605f0 into master Sep 17, 2020
@nelsonic nelsonic deleted the get-list-of-roles branch September 17, 2020 13:31
@th0mas th0mas mentioned this pull request Sep 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants