Disable activitystream for access token last_used

[bhavenst]

Adds overridden bearer token validator to prevent multiple calls to is_valid, which in turn was causing multiple updates of last_used on the token and multiple activitystream entries reflecting that.

[bhavenst]

This does fix the issue I can see and fixes the double entry seen in the gateway dev environment. There is still the issue of the 4 entries seen in QE's dev system, but since it's a challenge to get access and/or replicate the environment, we could go ahead and merge this fix and follow up with another one if something is found there. So setting this one ready for review.

[AlanCoding]

It seems suspicious that calling is_valid should produce any activity stream entries. It would be good to paste a test case and show what activity stream entries that generates.

Also, anecdotally, I have found that unit tests are very useful for testing activity stream. Either API actions can be mocked, or ORM-only actions can be done and then just check what was added in the activity stream at the end, according to the expectation. I'm fairly sure this same type of token can be created in test_app, which would be good to see.

[bhavenst]

yeah sure, I can do that. It is the update of the last_used attribute on the access token that actually causes the activitystream entry, not is_valid in and of itself.

` activitystream = object if 'ansible_base.activitystream' in settings.INSTALLED_APPS: from ansible_base.activitystream.models import AuditableModel

activitystream = AuditableModel

class OAuth2AccessToken(CommonModel, oauth2_models.AbstractAccessToken, activitystream): `

It's just that is_valid is where the update is done..

[AlanCoding]

My 2 cents is that updating last_used and recording an entry, effectively creates a usage log - a list of timestamps when the token was used. If this is what you wanted (a list of times of use), then I don't feel like the activity stream is well-designed for that. If you were to create a token use history table, the design choices would be different. It also starts to look like plain logging is a better solution. For near-term purposes, ping @relrod, I would ask the question if these entries are needed at all. If someone ever did want this data, you'd be committed to a solution which is a bad fit for the problem, which isn't what you want anyway.

[bhavenst]

Thanks for the input! This is @relrod 's code doing the activitystream entries.. :) I just picked up the bug to fix the extra activitystream entries and didn't change the behavior otherwise. So we can let him comment on your concerns I think.

[relrod]

@AlanCoding Yeah I've gone back and forth on this. We could just add the field to the activity stream exclusions list and call it a day, and that might make more sense - we don't (as far as I know) add an activity stream entry every time any other auth method is used, so tokens would be the exceptional case there. I think I'd be okay with that.

I had the thought that maybe the UI would want to show the usage log for a token or something and the AS gives an easy way to provide that. But I'm not convinced it's very useful. As you mentioned, logs are probably the better place for this kind of thing.

So yeah, I would be fine with this PR becoming:

activity_stream_excluded_field_names = ['last_used']

...in the token model.

[bhavenst]

Done, just excluding last_used and no activitystream entries arrive as expected.

[relrod]

It might be nice to have a regression test (just a test that uses a token to do something -- there are a number of them already -- and checks that the number activity stream entries referencing the token object doesn't change)

[sonarcloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud