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

User Groups Not being updated #3730

Closed
malcomm opened this issue Jul 25, 2019 · 26 comments
Closed

User Groups Not being updated #3730

malcomm opened this issue Jul 25, 2019 · 26 comments
Assignees
Labels
Auth Related to Auth components/category Cognito Related to cognito issues question General question

Comments

@malcomm
Copy link

malcomm commented Jul 25, 2019

Describe the bug

Amplify with Cognito. If I update a user's groups, I'm not seeing a great way to update the user's groups. My hope is that this will/should automatically update.

To Reproduce
Steps to reproduce the behavior:

  1. Check user's groups
  async currentUserGroups(): Promise<string[]> {
    const currentSession: CognitoUserSession = await Auth.currentSession();
    let groups: string[] = [];
    if (currentSession) {
      groups = currentSession.getIdToken().payload['cognito:groups'];
    }
    return groups;
  }

Expected behavior

When I update the user's, I would expect Auth.currentSession() to do the right thing.

Sample code
I was able to get the user session to be updated, but this work-around doesn't seem right:

  async updateUserGroups() {
    const currentUser: CognitoUser = await Auth.currentAuthenticatedUser();
    const userSession: CognitoUserSession = currentUser.getSignInUserSession();
    const refreshToken = userSession.getRefreshToken();
    currentUser.refreshSession(refreshToken, (err, session) => {
      currentUser.setSignInUserSession(session);
    });
  }

Or at least it's pretty painful. If this can't happen automatically, is there a way to have a method on CognitoUser ... something like updateUserGroups()?

If this is the "correct" or "best" way to handle this, then at the very least this should be heavily documented. I would be happy to help in any way that I can.

Thank you

@malcomm malcomm changed the title User Group Not being updated User Groups Not being updated Jul 25, 2019
@haverchuck
Copy link
Contributor

@malcomm - Try the bypassCache option in currentAuthenticatedUser:

 Auth.currentAuthenticatedUser({bypassCache: true}).then((res) => console.log('res', res))

@malcomm
Copy link
Author

malcomm commented Jul 25, 2019

@haverchuck - well my code above works. The issue is, I just need the session to be updated automatically if possible. If there was an option for that configuration it would be great. I think adding {bypassCache: true} makes sense, but it still doesn't get around the main issue.

That is ... at least from my perspective.

Just to be sure, the thing I'm trying to do is to limit a user's access to graphql resources. I just realized I did not put this in my original description of the problem ... that's my bad. So what I really meant in the description is that I'm doing @Auth with groupsField and I remove/add a group to a user in Cognito, but their session is not updated and they can still access (or not if I add a group) the graphql resources. For example, if I have group1 (in the groupsField field) on a resource and I have the following:

type Foo
  @model
  @auth(rules: [
    { allow: groups, groupsField: "groupsCanAccess", mutations: [] }
  ])
{
  id: ID!
  ...
}

If a user has group1 at login, they can access that resource. If a Cognito Admin removes group1 from that user and they still have a valid session, they can keep on accessing that resource. If the user does a logout and re-logins, all is good. Or if I do the following:

  async updateUserGroups() {
    const currentUser: CognitoUser = await Auth.currentAuthenticatedUser();
    const userSession: CognitoUserSession = currentUser.getSignInUserSession();
    const refreshToken = userSession.getRefreshToken();
    currentUser.refreshSession(refreshToken, (err, session) => {
      currentUser.setSignInUserSession(session);
    });
  }

Then it fixes this problem. But, that's a client side solution and that seems like a bug.

@haverchuck
Copy link
Contributor

@malcomm When are you calling updateUserGroups?

@malcomm
Copy link
Author

malcomm commented Jul 25, 2019

@haverchuck - in my case I have an Angular app and I am using an "AuthGuard" via a CanActivate and updateUserGroups() is being called in there.

@malcomm
Copy link
Author

malcomm commented Aug 12, 2019

@haverchuck - just wanted to check on this again ... any updates?

@Muriet96
Copy link

I implemented your code with AppSync, like this:

const client = new AWSAppSyncClient({
  url: AppSyncConfig.aws_appsync_graphqlEndpoint,
  region: AppSyncConfig.aws_project_region,
  auth: {
    type: AppSyncConfig.aws_appsync_authenticationType,
    jwtToken: new Promise(async (res, rej) => {
      const currentUser = await Auth.currentAuthenticatedUser();
      const userSession = currentUser.getSignInUserSession();
      const refreshToken = userSession.getRefreshToken();
      currentUser.refreshSession(refreshToken, (err, session) => {
        if (err) {
          rej(err);
        } else {
          currentUser.setSignInUserSession(session);
          res(session.getIdToken().getJwtToken());
        }
      });
    })
  }
});

Thanks!!

@malcomm
Copy link
Author

malcomm commented Aug 13, 2019

@Muriet96 - thanks for that. But to be sure, isn't that just a different client side solution? My thinking is: this really needs to be a server side fix.

@malcomm
Copy link
Author

malcomm commented Aug 14, 2019

@haverchuck - this seems like a pretty big security risk to me ... shouldn't this be a marked a bug and escalated?

@oriharel
Copy link

I need this functionality as well. I use Cognito groups to manage user permissions. (unless someone has a better idea on how to manage them).

@stale
Copy link

stale bot commented Sep 18, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@malcomm
Copy link
Author

malcomm commented Sep 18, 2019

@haverchuck - any updates?

@oriharel
Copy link

If you're using the "responseType":"token" then calling currentAuthenticatedUser will return the un-updated user. you need to either refresh the token yourself or use "responseType":"code"

@malcomm
Copy link
Author

malcomm commented Sep 19, 2019

@oriharel - well the real issue here isn't the groups coming back ... that would be good to fix. The real issue is: when I remove a user from a group, they shouldn't be able to view the data if they don't have permission to do so. I'm using @auth and groupsField; if a user is in group1 they can access that data if group1 is on the field for that table. If I then remove the user from group1, they should no longer be able to view that data. This is not currently the case (it will work if the user logs out or if the token is refreshed, but again, those are client side solutions/workarounds). From what I'm seeing, appsync is using what the client is passing in to verify what groups a user has. This can be modified and is a security risk.

@stale
Copy link

stale bot commented Oct 19, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@malcomm
Copy link
Author

malcomm commented Oct 22, 2019

Looks like this got marked stale again ... any updates?

@jordanranz jordanranz added the to-be-reproduced Used in order for Amplify to reproduce said issue label Oct 31, 2019
@ericclemmons ericclemmons self-assigned this Nov 4, 2019
@ericclemmons
Copy link
Contributor

Hi @malcomm,

I'm also looking into this. Thanks for your clear synopsis from above:

If a user has group1 at login, they can access that resource. If a Cognito Admin removes group1 from that user and they still have a valid session, they can keep on accessing that resource. If the user does a logout and re-logins, all is good.
...
But, that's a client side solution and that seems like a bug.

There was a recent CLI update that improves group management (https://aws-amplify.github.io/docs/cli-toolchain/quickstart#group-management and this example: https://aws-amplify.github.io/docs/cli-toolchain/quickstart#administrative-actions), but you're right: the server should be restricting that resource before allowing access, not the client.

@ericclemmons
Copy link
Contributor

@malcomm I've learned that the downstream services are just doing signing validation of the access token, not checking permissions.

We're having conversations on how to best resolve this on both the client & server-side, but unfortunately don't have that answer today.

In the interim, knowing that polling/intervals can be expensive for batter life, I'd recommend the following:

  1. Determine a threshold (e.g. 10 minutes) that you deem reasonable for refreshing tokens.
  2. Leveraging the Page Visibility API, refresh the token when that threshold has lapsed.

Some examples of refreshing tokens have already been provided in this thread, among others:

Thanks for staying on top of this @malcomm! Hopefully the client-side token refresh is a reasonable stopgap while we can research a scalable solution for the server-side.

@malcomm
Copy link
Author

malcomm commented Nov 5, 2019

@ericclemmons - thank you! Glad this is going to be addressed. I was just curious: does Amplify have a bug bounty program? Seems like this might be a good candidate for something like that?

@undefobj
Copy link
Contributor

undefobj commented Nov 5, 2019

@malcomm just for clarity, there isn't a bug here. JWT tokens contain state from a server which clients pass to backend services. Sessions are not tracked on the backend in such a system and this is a standard industry practice with OIDC tokens where signing validation is completed. What @ericclemmons is suggesting is you could potentially have a trigger on your backend to notify clients when a change has taken place and have them refresh their tokens. This would be an additional feature that would need to be designed.

@malcomm
Copy link
Author

malcomm commented Nov 5, 2019

@ericclemmons & @undefobj - so there's no mechanism in place where the token becomes invalid? Refresh Tokens is what I'm thinking applies here, right?

Also, any solution that requires the client to do something doesn't seem right to me (basically what I'm thinking is that this has to be a server-side fix). If I'm off on this, then please let me know, because I want to learn why my thinking is incorrect here.

@undefobj
Copy link
Contributor

undefobj commented Nov 5, 2019

@ericclemmons & @undefobj - so there's no mechanism in places where the token becomes invalid? Refresh Tokens is what I'm thinking applies here, right?

No, tokens are valid until they expire. Refresh Tokens are a mechanism for obtaining new IdTokens or AccessTokens without prompting the user to reauthenticate. So you could put something in your backend workflow to trigger a Refresh Token to be used to get new Id/Access Tokens which the client can then use with updated state.

Also, any solution that requires the client to do something doesn't seem right to me (basically what I'm thinking is that this has to be a server-side fix). If I'm off on this, then please let me know, because I want to learn why my thinking is incorrect here.

There are always tradeoffs in systems if you want them to scale. This is an area where there isn't a bulletproof solution. You could store sessions in a central database and have every request go through that, but then you're causing AuthN to be repeated and that server must deserialize and lookup permissions each time which is prone to scale issues, throttling, and even DDoS attacks. This is why claims based AuthZ exists with tokens which expire after a time period.

Essentially what you're looking for is runtime authorization metadata to be dynamically updated. This won't happen without some sort of push based mechanism to clients like I mentioned above. One other alternative however would be to look at seeing if Dynamic Group Authorization with the GraphQL Transformer works for you. Aside from that you'll be hand-rolling runtime logic in a centralized authorization system that needs to scale appropriately.

@malcomm
Copy link
Author

malcomm commented Nov 6, 2019

@undefobj - Thank you for your response. Just to be sure, I am using Dynamic Group Authorization , see my comment above on July 25th.

@malcomm
Copy link
Author

malcomm commented Nov 11, 2019

@ericclemmons & @undefobj - just wanted to loop back on this and see if there are any updates.

Also, I understand there are complexities here that do no make this simple; however, I'm assuming that with Cognito, you must keep track of the tokens on the server side right? Going off of that base assumption I was hoping there would be a "stale" flag where the server would know to initiate an update/refresh. In my mind this is the same if you disable or delete a user (same basic workflow) ... the system surely would have to send back an unauthorized if the user has been disabled via the Cognito UI, right?

And just to be sure: for me, the biggest issue is that server is returning records that the user should not be able to see. I'm not even all that interested in the client token being updated (that would be nice, but not needed at all). What I'm most concerned about is that the GraphQL API is return records that belong to a group via the Dynamic Group Authorization that the user no longer has access to. This might be clear to everyone based on the everything else above, but I just wanted to be sure.

@undefobj
Copy link
Contributor

@malcomm I'm not sure there are any updates we could give, it's more of a solution you will need to own. You could try to implement your own solution to dynamically push a notification down to clients and have them refresh their token information, but that's just a suggestion as this integration isn't supported OOTB. As outlined earlier tokens are good until they expire. This is the nature of JWTs and isn't Cognito specific. When using Dynamic Group AuthZ what happens is the "groups that are allowed access" are stored on the data itself on the record in DynamoDB, and the permissions granted for a user are in the JWT which are compared at runtime. So if the JWT has information that hasn't been refreshed yet the client could still have access until the token expires. This is why you would need some sort of push mechanism based on a trigger event in the backend to force the client to refresh its token. This is totally up to what you think your security posture needs to be as most of the time for systems it's fine to let the token expire and refresh in an acceptable time period.

@sammartinez sammartinez added Cognito Related to cognito issues question General question and removed to-be-reproduced Used in order for Amplify to reproduce said issue labels Nov 22, 2019
@sammartinez
Copy link
Contributor

I am going to resolve this issue based on the above comment and information provided early from @undefobj

@github-actions
Copy link

This issue has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Auth Related to Auth components/category Cognito Related to cognito issues question General question
Projects
None yet
Development

No branches or pull requests

8 participants