User access management in the client code

marco-brandizi commented 1 year ago

Writing this after having seen this code in example-queries.js:

if(freegenelist_limit == 20 && accType.toLowerCase() == 'free'){
    queryRestriction = `<a class='query-restriction-text' onclick="loginModalInit()">(Login)</a>`; 
}
...

That's too bad, even for an interim codebase. The code is confusing the policy applied to the current user ('anonymous has 20 genes limit') with the task of establishing the kind of current user ('it's a free user, hence it has a 20 genes limit', not the opposite).

This is bad because of multiple reasons:

it isn't clear at all which roles we're dealing with (eg, why is the 20 limit is related to accType of type free?!)
cumbersome access policy-related code is duplicated potentially everywhere
which makes it very difficult to possibly change such policies (eg, if we want to change the 20 limit, we need to tweak all the points where this was used like in this example).

We need a userAccessManager singleton (possibly, define a class first), with the following (it's a draft, to be verified):

roles representation: Se the draft below. Something simpler like integer constants is possible, but the example below is more robust, especially for extensions
userAccessManager.getRole () Returns the current user's role (as an instance of UserRole)
userAccessManager.getGenesSearchLimit( role = <current> ) The max no of genes the user can search. If role is omitted, use current role
userAccessManager.can( role, testedRole = <current> ) True if the testedRole has the same or higher powers (ie, level) of the role expressed by the 'role'. This is a wrapper of the UserRole.can() method shown below
userAccessManager.require ( requiredRole, testedRole, currentRole = <current> ) A boolean wrapper of:
```
if !UserRole.get ( requiredRole ).can ( testedRole ) return false
return currentRole.can ( requiredRole )
```
Again, role names can be either strings or UserRole(s), currentRole can be omitted.
possibly, other methods, such as getIsGeneListLimitEnforced ( role = <current> ) (not sure this is necessary, rather than be represented with limit = -1 or max int).

This way, the sample query case above would work like:

if ( userAccessManager.requires ( 'free', accType ) 
  queryRestriction = ...

User roles

/** 
 * Enum's example, based on classes, see https://www.sohamkamani.com/javascript/enums/
 *
 */ 
class UserRole
{
  #level = 1000

  static ANONYMOUS = new UserRole ( 1000 )
  static FREE = new UserRole ( 500 )
  static PREMIUM = new UserRole ( 100 )
  static ADMIN = new UserRole ( 0 )

  constructor ( level ) {
    this.#level = level
  }

  /**
   * The smaller a role is, the higher power it has. Min is 0 for ADMIN, max is 1000
   * for ANONYMOUS.
   */
  getLevel () {
    return this.#level  
  }

  /** 
   * Compare by level, returns -1 | 0  | 1, ie, negative means this role is more
   * powerful than the other.
   */
  compare ( other )
  {
    if ( !other ) return -1
        if ( typeof role == "string" ) role = UserRole.get ( role )
    if ( ! ( other instanceof UserRole ) ) 
      throw new TypeError ( "compare() needs a UserRole parameter" )
    return this.getLevel() - other.getLevel()
  }

  /**
   * True if the role parameter has the same or higher power of this role.
     * Param can be a UserRole or a string.
   */
  can ( role )
  {
    return this.compare ( role ) <= 0
  }

  /**
   * Get role by string, case-insensitive
   */
  static get ( roleStr )
  {
    if ( typeof roleStr != 'string' ) 
      throw new TypeError ( "get() requires a non-null string" )
    roleStr = roleStr.toUpperCase ()
    let result = UserRole [ roleStr ]
    if ( !result ) throw new TypeError ( `Invalid user role ${roleStr}` )
    return result
  }
}

// Usage examples
console.assert ( UserRole.ADMIN.can ( UserRole.PREMIUM ), "can() on ADMIN/PREMIUM doesn't work!" )
console.assert ( !UserRole.ANONYMOUS.can ( UserRole.FREE ), "can() on ANON/FREE doesn't work!" )
console.assert ( UserRole.PREMIUM.can ( UserRole.PREMIUM ), "can() on PREMIUM/PREMIUM doesn't work!" )

console.log ( Object.keys ( UserRole ) ) // All the roles
console.assert ( UserRole.get ( "premium" ) === UserRole.PREMIUM, "string-based fetching doesn't work!" )

Arnedeklerk commented 1 year ago

If you're happy @marco-brandizi, let's close.

marco-brandizi commented 1 year ago

@Arnedeklerk, @lawal-olaotan this is good, but it requires a few more changes. see the code comments.

Arnedeklerk commented 1 year ago

@lawal-olaotan pls remember to add limits for allowing pro users to search with >10 genes selected out of gene view.

Arnedeklerk commented 1 year ago

Happy! Looks good, thanks @lawal-olaotan. Closing...

marco-brandizi commented 1 year ago

@lawal-olaotan, @Arnedeklerk sorry, I need to reopen this, cause it still contains things to be fixed:

In example-queries.js:

if (isGeneListRestricted && minimumUserRole == 'pro') {
  queryRestriction = `<a class='query-restriction-text' href="https://knetminer.com/pricing-plans" target="_blank" >(Upgrade)</a>`;
}

As explained in the comment, the proper way to do this is userManager.can() (or .requires(), as you named it). role == <required-role> does not work when the current role is something like admin, root. You must check this via user manager helpers, so that they consider the role hierarchy/priority.

UserAccessManager

setGeneSearchLimit() maybe works right, but it's dirty and unusual, since it works like:
```
if free role => freePerms
if pro role  => proPerms // overrides the previous setup
if ...
```
If you read the comment above this code again, I've proposed to scan from the most powerful to the least one, so that you can use if/elseif and stop at the first that matches.

UserRole:

This comment is still to be addressed: TODO: How is it that there are two roles with the same level?!
See new TODOs, the code is still messy and bugged.

lawal-olaotan commented 1 year ago

@marco-brandizi, here are the ways I tried to refine the code based on your last comment.

In example-queries.js:

var userLevel = UserRole.getUserRole();

if (isGeneListRestricted && userLevel <= 100) {
     ...         
   }

In UserAccessManager:

 if(this.requires('free')){
            this.#defaultGeneLimit = 100;  
        }else if(this.requires('pro')){
            this.#isGeneLimitEnforced = false;
            this.#defaultKnetViewLimit = 20
        }

In UserRole:

There are two roles with the same permission value because the Knetspace API returns a static string free for the logged user instead of <minimumUserRole>registered</minimumUserRole> in the sample-queries file. Since both roles have the same permission level, I assigned them the same value.

marco-brandizi commented 1 year ago

@lawal-olaotan

user-access

if (isGeneListRestricted && userLevel <= 100) {

This works, but still isn't clean enough: think of a UserRole as an abstract entity, where the only thing that is authoritative for telling the power of a role (and compare it to another role) is UserRole.can(), and no integer level is accessible from outside UserRole, that is, the outside should not know how UserRole establishes that one role is superior to another.

Then, you would do the above check this way:

// I understand this comes from sample-query.xml
var {minimumUserRole,description,index} = query
// Gets the object from the string
minimumUserRole = UserRole.get ( minimumUserRole )
...
if ( isGeneListRestricted && minimumUserRole.can ( UserRole.PRO ) ) {
  ...
}

There are commented examples in user-access.js, which show similar cases. This might seem nuanced, however:

is more readable (what is 100 in your version?) and implementation-independent (what if, for some reason, we need to associate 100 to a different role name? Or revert the level ordering?)
should it be necessary for some reason, it allows for changing methods like can(), eg, we might decide to fetch roles from an API, without knowing which levels will come back (OK, it's not a realistic example in this case, but to give an idea of why it's better to use this abstraction)
It allows for enumerating existing roles (with their self-explaining names) as UserRole.XXX (which are currently defined, but you don't use them here)
there isn't much more to fix in the existing code

Moreover, for the above to work, you need to change this too:

class UserRole {
...
  static can ( role,queryRole ) {
    ...
  }
}

can() belongs to a UserRole instance, it tells you if the current instance (ie, this) can do what the parameter can:

  can ( queryRole ) {
    return this.compare ( queryRole ) <= 0
  }

This is what is proposed in the hereby ticket's description above. Actually, I've written compare() in a way that allows queryRole to be a string, as an alternative to UserRole instances (it's based on UserRole.get(), which checks the string corresponds to one of the UserRole.XXX constants, so, it's not a free string). You can use this form if you want, though initially I proposed it only because I knew the strings would be coming from sample-queries.xml and the API. In the code, something like UserRole.PRO is more explicit.

Similarly, static compare(role,queryRole) needs to be compare ( queryRole ) (again, see the initial description above), and for both the methods the parameter needs to be an instance of UserRole (or a string, as said above), not a numeric level.

Also, note that the result of compare() is an integer, but it's not a role level, for it follows the common convention of returning negative/0/positive integer when 'this' (the left entity) is "abstract less"/"abstract equal"/"abstract greater" than the parameter (abstract in the sense that compare() can decide any ordering criterion, which is not necessarily about numbers). In other words, such result too abstracts the comparison away from the internals based on levels.

UsetAccessManager checks:

 if(this.requires('free')){
   this.#defaultGeneLimit = 100;  
 }else if(this.requires('pro')){
   this.#isGeneLimitEnforced = false;
   this.#defaultKnetViewLimit = 20
 }

No. if/elseif works only when you scan from the most powerful role to the least one, for if your user is a pro, then first if() sets the free limits and then nothing else is touched. Furthermore, for the reasons said above, it's better to work with UserRole names (constants or strings):


if ( this.#currentRole.can ( UserRole.PRO ) ) {
  // pro rights and settings, apply to ADMIN, ROOT too
  ....
}
else if (  this.#currentRole.can ( UserRole.FREE ) // adding this, just to show a complete example
{
  // FREE rights, apply to pro too
  ...
}
else {
  // fall back to min rights (ie, guest)
  // ===> set these defaults here, not elsewhere, for this is the method that deals with it
  ...
}

Here, I'm not using your UserAccessManager.requires ( queryRole ), if you find it more comfortable, define this method as a shortcut for this.#currentRole.can ( queryRole ), but 1) make can() an instance method (not a static one, as explained above) 2) probably UserAccessManager.can() is a better name than UserAccessManager.requires () (since it's a wrapper of the method with the same name).

Free/registered roles:

There are two roles with the same permission value because the Knetspace API returns a static string free for the logged user instead of registered in the sample-queries file. Since both roles have the same permission level, I assigned them the same value.

I see. One option is to just report this in a comment in UserRole, where these two roles are defined. I'd be fine with this, though if it was for me, I couldn't resist from tweaking the API handler like: if API-returned-role == 'free' => use 'registered' instead, so that the confusion stays in the API only and we use registered in KnetMiner (the same explaining comment needs to be added in this case too).

Rothamsted / knetminer