Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add owner scope to API keys #4935

Merged
merged 2 commits into from
Nov 2, 2017
Merged

Add owner scope to API keys #4935

merged 2 commits into from
Nov 2, 2017

Conversation

chenriksson
Copy link
Member

@chenriksson chenriksson commented Oct 31, 2017

Remaining work:

  • ApiController scope evaluation
  • Auditing
  • APIKeys.cshtml UI

AllowedAction = allowedAction;
}

// Deprecated: Should be removed once ApiKeys.cshtml is updated to support owner scope.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this just be as simple as changing a constructor call to pass the current username as owner?

You can add the ability to choose what user later. I would just prefer not having deprecated methods if it's very easy to fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but then I'd need to update all callers. I'm minimizing changes in this PR (schema and copying org scope for temp keys). I'll remove this ctor when I start requiring the owner, which happens when I enable the UI.

/// Package owner (user or organization) scoping.
/// </summary>
[JsonProperty("o")]
public string Owner { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this a user key?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "identity" of a user is the Key property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question... there was debate about whether keys can be scoped to multiple owners (yourself, and your organization(s)). We decided that we'd start with a single owner, but my impression was that we might change that.

I typed as string so that it could change with the same schema. Alternatively, I could change to FK and we move to many-many relation if we change our minds on supporting multiple owners.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joelverhagen Switched owner scope to FK. This does make more sense - if we support multiple owners per API key, we can just add more scopes.

@chenriksson chenriksson merged commit 02cb908 into dev Nov 2, 2017
@chenriksson chenriksson deleted the chenriks-org-scope branch November 2, 2017 18:26
@shishirx34 shishirx34 mentioned this pull request Nov 3, 2017
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants