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

AuthRegistry Implementation #139

Merged
merged 3 commits into from Feb 14, 2018
Merged

AuthRegistry Implementation #139

merged 3 commits into from Feb 14, 2018

Conversation

seehamrun
Copy link
Member

@seehamrun seehamrun commented Feb 14, 2018

* IMPORT specifies an authorization that allows you to import a type (implying read-write permissions)
* EXPORT specifies an authorization that allows you to export a type (implying read-only permissions)
*/
enum AuthMode {
Copy link
Member Author

Choose a reason for hiding this comment

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

@holachuy I know you added TransferMode in #134 - Does it make sense to either switch to that, or move that in here before merging?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mind going with AuthMode but the bigger question seems that it goes unused except in log statements. Can we remove it alltogether?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking it will get passed into the getAuthDataGenerator call to the AuthServiceProvider, which will then return a generator for the correct permissions mode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Per offline discussion, SGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

* IMPORT specifies an authorization that allows you to import a type (implying read-write permissions)
* EXPORT specifies an authorization that allows you to export a type (implying read-only permissions)
*/
enum AuthMode {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mind going with AuthMode but the bigger question seems that it goes unused except in log statements. Can we remove it alltogether?

Copy link
Collaborator

@holachuy holachuy left a comment

Choose a reason for hiding this comment

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

Very nice! One question on MS which can be in a follow up.

Also, I'll adjust my PR to use this once it's merged.


@Override
public List<String> getImportTypes() {
return ImmutableList.of();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to put values here and below?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it blank since nothing was implemented yet for it but I didnt want to return null at the same time. I guess I could throw an Unsupported exception too. What makes the most sense?

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.

None yet

2 participants