-
Notifications
You must be signed in to change notification settings - Fork 26
Conversation
Any comments on this? |
(I didn't realize this was pending review) that said, I'm curious the use case for getting a users permissions outside of a request context.... |
The use is in this patch, in fact -- disable user tokens when the user no longer has the permissions embodied in the token. |
I'd very much like to deploy this soon, since we'll need user tokens for tooltool uploads. Any chance of an r? |
elif not disable and token.disabled: | ||
job_status.log_message("Re-enabling token %d for user %s" % (token.id, token.user)) | ||
token.disabled = False | ||
session.commit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this task is scheduled for an hour now, but I am slightly concerned that there will be a time where this takes longer than an hour. And then we have two copies doing duplicate work (the session as it stands means we'll have the same results each cycle).
My ideal would be something like, but it also may be overkill for this use case, I leave that up to you.
user_ids = [tok.user for tok in tables.Token.query.filter(tables.Token.typ == 'usr')]
for id in user_ids:
session = current_app.db.session('relengapi')
token = session.query(tables.Token).get(id)
if not token:
continue # if the token was deleted while this was executing, do nothing.
# ...
session.commit()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, committing a session with an iterator open will cause that iteration to fail.
I'm not too worried about overlaps -- we might get exceptions if both runs try to make changes, but the operations are idempotent so that won't hurt anything. The real fix is to use badpenny to run no more than one instance at a time (#132).
Only 1 line is missing from tokenauth/init imho might as well test it (at least partly); That said, if you address my nits 👍 feel free to ping me here if you want a second pass at any new csets. (or need me for any followup to my comments) |
e67c0e0
to
52eb4bb
Compare
@Callek, @jvehent
This, along with #175, should finish off the user-token work described in #163.