Repository navigation
Adds roles for varied access - #290
Conversation
| namespace :roles do | ||
| desc "Creates the initial set of roles" | ||
| task initial_set: :environment do | ||
| ['admin', 'board_member'].each do |role| |
There was a problem hiding this comment.
@nellshamrell @hollomancer @kylemh - Outside of just a standard logged in, authorized user, what roles might we want to have initially? BTW, this can be changed at any time.
Here I have admin and board_member, but those are just placeholder ideas. In general, I am thinking something like:
- all access (something like
super_admin?) - less access (something like
admin?) - the least amount of access (something like
board_member?)
BTW, we would not need a role for "ID.me verified users", b/c that is just an attribute on the user object that is already available to the frontend (i.e. user.verified).
Thoughts on that? Recommendations?
There was a problem hiding this comment.
Overall, this looks good. I would perhaps do something like super_admin (all access), admin (less access), member (least access). Do our board members need special permissions to the site?
There was a problem hiding this comment.
@nellshamrell - We actually don't need a role for a standard member. Their auth privileges will be gated by Devise, through the current_user process.
In that same vein, we will not need a role for an ID.me verified veteran user, as that will be gated by an authorized user with a user.verified attribute of true.
So these roles only apply to people we want to give additional access to, on top of the standard member, or verified veteran member.
re: board_member, I was thinking something along the lines of where a board member could see certain stat pages that normal members could not see. Whereas an admin would have access to those pages, plus some additional write privileges. And then of course a super admin would have everything.
Those are just examples...trying to paint a picture. We can set things to be however we want them.
I'll create those three role for now, to get this PR merged, so we can move onto beta implementation. Then we can update at any time.
93d4cd8 to
cf0e2c7
Compare
| end | ||
|
|
||
| test "role gets the Role with an id of user#role_id, if there is one" do | ||
| user = User.create( |
There was a problem hiding this comment.
Why didn't you use the user factory here?
| standard_role = create :role, title: 'standard' | ||
| assert standard_role.valid? | ||
|
|
||
| duplicate_role = build :role, title: 'standard' |
There was a problem hiding this comment.
Instead of using build and then save, can you do this in one step and use create? Is there a benefit to doing one over the other?
duplicate_role = create :role, title: 'standard'
refute duplicate_role.valid?
There was a problem hiding this comment.
Good question. If you were to use create it would error silently, b/c validations are not met, and the test would fail.
By calling build then save, you are able to check for errors and validity, and the test continues to run its course.
| def self.fetch_social_user_and_redirect_path(data) | ||
| user = User.find_by(email: data.dig(:email)) | ||
| path = '/profile' | ||
| if user.nil? |
There was a problem hiding this comment.
Does the user model need a belongs_to relationship with the role model?
There was a problem hiding this comment.
Good question. Take a look at this commit message: fea4613
| class UserSerializer < ActiveModel::Serializer | ||
| attributes :id, :email, :zip, :first_name, :last_name, | ||
| :slack_name, :mentor, :verified | ||
| attributes :role |
There was a problem hiding this comment.
Why isn't :role added to the attributes already listed?
There was a problem hiding this comment.
Good question. With ActiveModel::Serializer, if the attribute is listed in the long list of attributes, it will directly call the method from the model. In this case, role is an association`.
So if it is left in the long list, it would return an object of the whole associated role, in the role key.
In this case, we just want the associated role's title. To do that, you create a method with the desired name (i.e. role), and have that method return whatever you want (i.e. just the title).
cf0e2c7 to
1e50dab
Compare
Reason being, with the belongs_to association, every single user record would *have to* have an associated role. That is not the use case for us. Standard users are authenticated, and do not need a named role. Authentication grants them the access level they need. The role will come into play for users that need increased access. For example an admin, or board member, or super admin, etc., etc.
1e50dab to
6228db4
Compare
|
Could this be a good way to add a role for the slack bot? |
|
No, but good thinking. We're going to need to be more secure than that. We'll be going through more of a token based authentication process. |
Description of changes
Creates a
Roletable giving certain users the ability to have varied access. This is accomplished in conjunction with Pundit.Todo
userserializer to include a user's roleIssue Resolved
Fixes #289