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

modelService.permissions(modelid).getPermissions() Promise returns undefined #185

Closed
toebes opened this issue Sep 6, 2020 · 2 comments
Closed
Assignees
Labels
breaking The resolution creates a breaking API change bug Something isn't working client Issues related to the client
Milestone

Comments

@toebes
Copy link

toebes commented Sep 6, 2020

Versions

  • Convergence Version: 1.0.0 RC7
  • OS: Windows 10
  • Browser: Chrome

Describe the Bug
When handling a fulfilled ModelPermissions promise, the permissions passed is undefined.

Step To Reproduce
When logged in using a JWT token, call modelService.permissions(modelid).getPermissions() on a model that exists and you have access to. The fulfilled promise will be undefined.

modelService.permissions(modelid).getPermissions()
                .then((myPermissions: ModelPermissions) => {
                    if (myPermissions === undefined) {
                        console.log("Permissions is null!");
                    })

The problem occurs in ModelPermissionManager.getPermissions() which has this code:

 return this._connection.request(request).then((response) => {
            const { getModelPermissionsResponse } = response;
            const permissionsData = getModelPermissionsResponse.userPermissions[this._connection.session().user().username];
            return toModelPermissions(permissionsData);

There are actually two bugs here.

  1. getModelPermissionsResponse is actually an array and not a map, so when the session username (a string) is used, it will always return undefined. The code should be iterating over the array to find the proper match.
  2. In the event that nothing is found, it should be returning an empty ModelPermissions that has all permissions set to false.

Expected Behavior
A non undefined ModelPermissions should be returned as part of the Promise.

@toebes toebes added the bug Something isn't working label Sep 6, 2020
@mmacfadden mmacfadden self-assigned this Sep 6, 2020
@mmacfadden mmacfadden added client Issues related to the client breaking The resolution creates a breaking API change labels Sep 6, 2020
@mmacfadden mmacfadden added this to the 1.0.0-rc.8 milestone Sep 6, 2020
@mmacfadden
Copy link
Contributor

There s a third issue here, which is that the users that are coming back are full fledged users with a type and user name, and we are only looking at the username here:

https://github.com/convergencelabs/convergence-client-javascript/blob/a7b42c894def1b92d1a9af34272ef7562bdf97b9/src/main/model/ModelMessageConverter.ts#L209

So the returned map has only the username as the key, this is wrong since it will ignore anonymous or convergence users. We will use the above method, but must update it to account for various types of users. This means that the keys in the map will change. While not an API change, it is a semantic change.

@mmacfadden
Copy link
Contributor

I think APIs that deal with permissions will similarly need to be updated to take the user guid, instead of just the username.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking The resolution creates a breaking API change bug Something isn't working client Issues related to the client
Projects
None yet
Development

No branches or pull requests

2 participants