hubot-archive / hubot-auth

Assign roles to users and restrict command access in other scripts
149 stars 54 forks source link

Improve roles persistency #40

Open dberzano opened 7 years ago

dberzano commented 7 years ago

Improve roles handling and persistency

dberzano commented 7 years ago

@ceci-woodward this should in principle provide a better solution to #36 and #37 than #38 and should also provide a fix to the issue described in my comment, if I am not overlooking anything...

cecilia-sanare commented 7 years ago

My changes in this PR shouldn't be merged due to it breaking backwards compatability. I.E. storing roles separate from the users

JSzaszvari commented 6 years ago

What exactly does this break?

Applying this patch actually makes hubot-auth work. The fact that if you restart hubot all roles are lost/forgotten is a pretty big issue...

alFReD-NSH commented 6 years ago

Can we merge this and release a new semver major version?

JSzaszvari commented 6 years ago

@alFReD-NSH I forked this a whole ago with the changes the changes above so it actually works. I'm not sure how this plugin is meant to be useful in the slightest without the ability to retain the roles information after a reboot, but here it is

https://www.npmjs.com/package/hubot-auth-persistent

cecilia-sanare commented 6 years ago

@jszaszvari @dberzano @alFReD-NSH

I recall the breaking change was due to us moving the location redis would store the users roles. Previously we would store them directly on the user object, now we store them in a roles obect. This means that users who don't run into this issue will have their roles wiped out.

I'm guessing its a race condition where there are times that the user isn't initialized and the roles can get wiped out.

Adding the role and verifying it exists

image

After a restart verifying the role still exists

image

After pulling in @dberzano change

image

Overall I like the change, but we should probably try and pull the roles from the user object if they exist.

cecilia-sanare commented 6 years ago

Something like the following should work for migrating roles from the old format.

module.exports = (robot) ->
  robot.brain.on 'connected', () ->
    roles = robot.brain.get('roles')
    for id, user of robot.brain.users()
      if user.roles and user.roles.length
        console.info "converting #{id} to the new roles format..."
        roles[id] or= []
        userRoles = roles[id]
        userRoles.push role for role in user.roles when role not in userRoles
        delete user.roles
    roles = robot.brain.set('roles', roles)
  # ...