pretenders / ployst

The ployst core repo
1 stars 0 forks source link

Abstract access token from being implicit primary key of teams #16

Closed alexcouper closed 10 years ago

alexcouper commented 10 years ago

Issue.

Currently access token used for github is the primary key guid of the team. This is bad due to coupling and lack of control (eg revoking access)

Solution

Abstract out to TeamProviderSettings - which has foreign keys to teams and providers and contains a single json dict, in which is the access token.

One concern @alexcouper has is that looking up the team by access token will be slow.

alexcouper commented 10 years ago

I started on this and then realised that I'm not sure I have the same idea on this as you do. I was beginning to implement a TeamProviderSettings model that had team, provider and settings columns.

I wrote a test to prove that I could get a team id from a provider and access token.

This led me to add an additional column of access_token to the TeamProviderSettings table, as filtering on a nested index wasn't going to be fun in django rest framework (against sqlite at least).

And now I'm thinking "@txels probably doesn't even want this field on this table in the first place".

I'm happy to continue and be blocked later, or wait to hear some thoughts from you. Either way the github provider work is blocked by this feature.

txels commented 10 years ago

I would implement tokens simply as:

class Token(models.Model):
    user = models.ForeignKey(User)  # or team = ForeignKey(Team)
    key = CharField()
    # scope = CharField()  # or JSONField of we want sthg more structured...

See my rationale and variations explained at length in issue #15.

alexcouper commented 10 years ago

I'm happy with that being the way we do tokens for now.

alexcouper commented 10 years ago

So we're missing Token and TeamProviderSettings models. And these are unrelated (logically).

txels commented 10 years ago

Yes. We will be needing TeamProviderSettings at some point later, not yet sure when.