Open LeoUrzua opened 4 years ago
Thanks for putting this together! I would love to see a React binding for this library.
I have a few nit-picks about the naming and structure of things, but also a few questions.
The first is what version fo React we want to support? I think hooks are becoming very popular and so would like to make this use those as our first implementation -- but possibly in the future we could also support HOCs or renderProps if there was desire for those.
If we go that route, I think we should take a look at https://usehooks.com/useAuth/ as an example of how this code could be structured. That looks very clean to me. I think this is similar to the intent you have here.
One of the larger changes would be that the consumers of this package should not really need to "know" that we are using context at all. They should just include the AccessDecisionMangerProvider
component just below their "UserProvider", and then have access to an useIsGranted
hook anywhere else in their app.
he first is what version fo React we want to support? I think hooks are becoming very popular and so would like to make this use those as our first implementation -- but possibly in the future we could also
I agree about the hooks implementation. And yes, I was doing in two different ways in the example (the one commented and the other one).so show different options but I also prefer the one that you suggested.
Here is how I think it is going to look (there are some missing details but I'll be iterating over them)
type ADMContextType = {
isGranted: (attribute: string, subject: any)=> Promise<Boolean>;
}
const admContext = createContext<ADMContextType>({
isGranted(){
return Promise.resolve(false);
}
}
);
export const ProvideADM = ({voters, children}: {voters: Voter[], children: any}) => {
const accessDecisionManager = new AccessDecisionManager(null, voters, null);
const adm = useProvideADM(accessDecisionManager);
return <admContext.Provider value={adm}>{children}</admContext.Provider>
};
export const useADM = () => {
return useContext(admContext);
};
const useProvideADM = (accessDecisionManager: AccessDecisionManager) => {
const isGranted = async (attribute:any, subject:any) : Promise<Boolean> => {
const isGranted = await accessDecisionManager.isGranted(attribute, subject);
return isGranted;
};
return {
isGranted
}
};
About the naming, I updated some of the variables
Please let me know you think about
Component name -> ProvideADM Context -> useADM Hook name -> isGranted
There are still a couple of things that I'd like to review.
Since we are using react we have the state management which is going to manage
user, context
So, the user is going to be provisioned by a father component ... something like this
<ProvideUser>
<ProvideADM user={user} voters={voters({})}>
In this case, the user variable would be something like:
const user = {
id: <T>
....
So, the ADM would have an interface similar to this:
export const ProvideADM = ({voters, user, context,children}: {voters: Voter[], user: any, context: any, children: any}) => {
And I was wondering if that was ok. since we might want to get the user and the context from somewhere else.
Would it be better to change user->getUse?
so we would getUser from the state(hopefully using a hook that ProvideUser
implements
and the same case for context (if it is not provisioned then we are going to try to get from any parent component->hook)
Summary:
Do we want to get the user from a hook within the React
package or just inject the user variable?
And the same question for context
My personal point of view:
I think we need to inject the user in the component <ProvideADM user={user | currentUser |.. etc}
In that way, a user can be injected using whatever variable it is stored like the line above.
I think we are converging on a similar idea, which is a good sign!
AccessDecisionManagerProvider
is probably a good name for that component.use*
, so I think I'm still leaning towards useIsGranted
there.AccessDecisionManagerProvider
If I am reading this correctly, I like your final suggestion of this taking the following props:
user
-- this should be of the same type of the ADM, which I believe is an optional anything. Most of the time this will be an object, but I don't think this object has any requirements on it. For example while it might be common for this object to have an id
it also might be totally possible for it to use email
as the primary identifier. Similarly, this "user" could be just a number/string that represents the ID of the user. I don't think this would be my preferred implementation, but I don't think there is anything about the ADM patter that would prevent this.voters
-- again, similar to the ADM this will be an array of Voters.options
" -- This should mirror the work we do in https://github.com/wizeline/access-decision-manager/pull/23 . I'm not sure if we want to accept an options object here or if we would do something like:
const AccessDecisionManagerProvider = (props) => {
const { children, user, voters, ...options } = props;
/* ... */
}
useIsGranted
hookI think the way that this would be the easiest to use would be if this hook took 1 required, and 1 optional argument, just like adm.isGranted
. This hook would internally fetch the context to get the ADM, and call it's isGranted
method.
const MyPostComponent = (props) => {
const userCanEdit = useIsGranted('edit-post', props.post);
}
Yes! it is taking a better shape. I feel that it is time to start to code the concept.
I totally agree with the naming.
AccessDecisionManagerProvider
1.,2.
Agree
3.
I am afraid that I didn't get it.
What is the interface of the options
argument?
{
createContext?: Function()
strategy?: Function()
}
Q: What is going to be the default value that we are going to failover if a context is not provided?
const context =
typeof createContext === 'function'
? createContext() : <AnySuggestion>?;
or are these options the ones that voters
use? like we do with the PSK
? in the express binding
If that's the case, we can use those options as we did it (within the voters, for example, some PSK | apiKey)
app.use(
AccessDecisionManagerProvider(
(req) => req.user,
voters({
someApiKey: process.env.SOME_PSK,
}),
),
);
or
const voterFactory = (options: Options): Voter[] => {
app.use(AccessDecisionManagerProvider( (req) => users[req.headers.authorization], voters({}) ));
Can you please elaborate more on the options
argument.
useIsGranted
hookThe current implementation uses something like
const SecretComponent: React.FC = (props) => {
const accessDecisionManager = useAccessDecisionManager();
const userCanSee = accessDecisionManager.useIsGranted('EDIT_POST', props.post);
But I notice in your comment above that useAccessDecisionManager()
is not being imported and useIsGranted
being implemented directly.
...About the next text
This hook would internally fetch the context to get the ADM, and call it's isGranted method.
How different is from the proposed solution?
Use the core functionality of
https://github.com/wizeline/access-decision-manager/tree/master/packages/access-decision-manager
to bind ADM to reactWhat is this ticket trying to accomplish?
Bind ADM functionality to react, so access to routes/components can be managed in a granular way
Objective / Action items:
Create a package that determines access to react components or routes using the core ADM functionality
Implementation:
As mentioned above react provides a context feature to create global props(these can be functions)
Since we need to check access in any level of the react tree component the usage of
Context
is adequate.To provision an AccessDesicionManagerProvider we need:
First, create an
AccessDesicionManagerProvider
that receives (user or getUser?:any, voters: Voter[], createContext?) . (In this way we are following the format of the rest of the packages and we enable the ADM core package to be implemented)Then inside the provider, we need to return a Context.Provider (this one is going to be consumed by the clients of this package
At the end we are going to have something like this:
and then the clients can use it with something like this
app.ts
Note: I placed a commend with an alternative way to accomplish the same result and that is not the end code it is just some PoC