Skip to content

Conversation

@bravo-kernel
Copy link
Contributor

Fixes:

  • fetching available roles from database
  • fetching user's associated roles

Open:

  • should user's associated roles be cached or is the query fast enough?
  • test using Model
  • fix broken tests

@bravo-kernel
Copy link
Contributor Author

I would suggest changing the aclKey and aclTable to roleColumn and roleTable to keep the naming more in line with the code (and what they are used for).

@dereuromark
Copy link
Owner

Cool.
Do we need some new tests?

Renaming these keys is fine with me.

@bravo-kernel
Copy link
Contributor Author

We do need some new tests but I would prefer to consult on how to continue first. Going through this part is see there is logic that is either no longer applicable or was intended to be used otherwise.

@bravo-kernel
Copy link
Contributor Author

Now supports 3 scenarios:

  • single-role with available roles using Configure array
  • single-role with availble roles using database table
  • multirole (requires database table and join table)

Tests need updating

@dereuromark
Copy link
Owner

IMO it should support both single and multi role with either DB or Configure.
They way they are retrieved shouldn't matter IMO, as it didnt before.
Is there a specific issue why that cannot be supported this way in your opinion?

@bravo-kernel
Copy link
Contributor Author

I'm not 100% sure but I don't think multirole using only Configure worked before, maybe we should discuss this to prevent miscommunication.

@dereuromark
Copy link
Owner

It must have worked before, especially in 2.x :)
At least I thought I used it in one app. But no big deal either way.
We should just try to make it work in a way that the actual source is irrelevant (maybe even cached at some point then in the future) as it will just work with both ways the same way.

@bravo-kernel
Copy link
Contributor Author

Let's discuss on IRC when you have the time, it might be my lack of understanding.

@bravo-kernel
Copy link
Contributor Author

Looks good to me.

dereuromark pushed a commit that referenced this pull request Mar 3, 2015
Fixes Cake 3.x multirole authorization
@dereuromark dereuromark merged commit f791d01 into dereuromark:master Mar 3, 2015
@bravo-kernel
Copy link
Contributor Author

Ty for merging :)

@bravo-kernel bravo-kernel deleted the multirole branch March 4, 2015 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants