-
Notifications
You must be signed in to change notification settings - Fork 7
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
Google, IDMan and static api key auth #316
Conversation
removed unwanted console log form function.js file
Bug fix n UI
Remove ttl from table
…ig completely optional
config/docker.yml
Outdated
enabled: ${AUTH_ENABLED} | ||
jwt: | ||
issuerId: foxtrot-server | ||
privateKey: "6/Qh1eLvF7cGJEgH1wIMYErIpi0d8Kqt5wHKw/o5iFbhfziKiux03HfFihO2+f5FKpsjk1D7UmgdAWQsY966WkyAsyo22m4g+C+rNCwMUD/UM2qop7JZBDEkgWpwK8jpxOTpYUnaNsiBaBsadoZCQ+Ns/d8RrS7QPAW+1PETbdFfo+fPW0/Sm8RUmMKkc9jJ6bhuSpFH3q/bWoyPZnj7geYQQIkgmQv9jXaszw==" |
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.
Is it okay to expose this private key here?
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.
This is some test key... not to be used in production. Moving it to compose file.
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.
wait .. no this is outdated... All these have already been moved to compose file.
|
||
String id; | ||
Set<FoxtrotRole> roles; | ||
Set<String> tables; |
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.
Does this mean a user would have same set of roles on a list of tables? Shouldn't it like user have higher access on tables which he owns and only read access on tables belonging to other teams
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.
Right now it is same across all tables to keep it simple. We can change this model later on if needed.
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.
Will change in different PR.
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.
foxtrot-server/pom.xml
Outdated
<dependency> | ||
<groupId>io.appform.idman</groupId> | ||
<artifactId>idman-model</artifactId> | ||
<version>1.0-SNAPSHOT</version> |
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.
Can be updated to release version
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.
Done
|
||
Optional<User> getUser(final String userId); | ||
|
||
boolean deleteUser(final String id); |
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.
Is this delete by userId? If yes, id can be changed to userId to make it consistent with getUser
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.
Done
|
||
boolean deleteUser(final String id); | ||
|
||
boolean updateUser(final String id, UnaryOperator<User> mutator); |
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.
Same as above
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.
Done
Preconditions.checkArgument(!Strings.isNullOrEmpty(googleAuthConfig.getProxyHost())); | ||
proxy = new Proxy(Proxy.Type.HTTP, | ||
new InetSocketAddress(googleAuthConfig.getProxyHost(), | ||
googleAuthConfig.getProxyPort())); |
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.
This implementation is same as HTTP implementation
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.
Fixing
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.
Done .. good catch
String param, | ||
Function<TokenInfo, Optional<T>> handler) throws URISyntaxException, IOException { | ||
val post = new HttpPost(new URIBuilder(config.getIdmanEndpoint()) | ||
.setPath("/apis/oauth2/token") |
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.
Where would IDMan be hosted? This would mean IDMan always have to be available if someone is using IDMan Auth
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.
It will be hosted separately for an org. Yes, it needs to be available for people to be able to login. Just like google needs to be up and working for people to login using google. Once authenticated, the auth info is cached for some time.
return EnumSet.allOf(FoxtrotRole.class); | ||
} | ||
case "FOXTROT_HUMAN_USER": { | ||
return EnumSet.of(FoxtrotRole.CONSOLE, FoxtrotRole.QUERY, FoxtrotRole.INGEST); |
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.
Human user needn't have Ingest role? Ideally ingestion should be happening only via systems
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.
Sometimes, needed for testing. But i generally agree. Removing.
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.
Changed
|
||
@PUT | ||
@Path("/users/{userId}/roles/grant/{role}") | ||
public Response grantRole(@NotNull @NotEmpty @PathParam("userId") final String userId, |
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.
Should we expose an api to grant multiple roles in one request?
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.
Roles can be granted in bunch during creation of user. This is for granting one at a time. But can be added in different pull request if needed.
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.
@PUT | ||
@Path("/users/{userId}/tables/access/grant/{table}") | ||
public Response grantTableAccess(@NotNull @NotEmpty @PathParam("userId") final String userId, | ||
@NotNull @NotEmpty @PathParam("table") final String table) { |
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.
Should we expose an api to grant multiple tables in one request? If an installation of foxtrot has multiple tables, it will be lot of work for the admin to give access to each user for multiple tables
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.
This is merged via different path |
No description provided.