hubot-auth icon indicating copy to clipboard operation
hubot-auth copied to clipboard

Improve roles persistency

Open dberzano opened this issue 9 years ago • 7 comments

Improve roles handling and persistency

  • Do not cache roles at start: persistency store might not be available yet and an empty response can be returned
  • Save roles back to the brain on change, except admin

dberzano avatar Jul 27 '16 14:07 dberzano

@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...

dberzano avatar Jul 27 '16 15:07 dberzano

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

cecilia-sanare avatar Aug 03 '16 04:08 cecilia-sanare

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...

JSzaszvari avatar Jul 26 '17 15:07 JSzaszvari

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

faridnsh avatar Aug 16 '17 19:08 faridnsh

@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

JSzaszvari avatar Aug 16 '17 23:08 JSzaszvari

@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 avatar Aug 17 '17 14:08 cecilia-sanare

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)
  # ...

cecilia-sanare avatar Aug 17 '17 15:08 cecilia-sanare