You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2022/10/17 21:33:47 UTC
[GitHub] [helix] micahstubbs opened a new pull request, #2238: 2235/identity token
micahstubbs opened a new pull request, #2238:
URL: https://github.com/apache/helix/pull/2238
### Description
<!-- Write a concise description: "what?, why?, how?" and then add some details about this PR, including screenshots of any UI changes -->
<!-- This PR fixes this Helix issue & includes the Helix issue in the PR description. Link your issue number here: You can write `Fix #123`. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue -->
This PR adds support for a configurable identity token source to the helix-front ExpressJS server.
Fix #2235
### Tests
New unit or integration tests: No new tests.
<details>
### Code Style
<!-- Ensure the PR diff has been formatted using [Prettier](https://prettier.io) -->
Formatted using [Prettier](https://prettier.io)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
### Changes that Break Backward Compatibility (Optional)
- [ ] My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include: -->
<!-- Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method or API behavior. -->
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page: -->
<!-- Link the GitHub wiki you added -->
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] NealSun96 merged pull request #2238: 2235/identity token
Posted by GitBox <gi...@apache.org>.
NealSun96 merged PR #2238:
URL: https://github.com/apache/helix/pull/2238
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] cliffordfajardo commented on a diff in pull request #2238: 2235/identity token
Posted by GitBox <gi...@apache.org>.
cliffordfajardo commented on code in PR #2238:
URL: https://github.com/apache/helix/pull/2238#discussion_r999698338
##########
helix-front/server/config.example.ts:
##########
@@ -5,20 +5,21 @@ export const HELIX_ENDPOINTS = {
},
],
};
-
export const SESSION_STORE = undefined;
-
export const SSL = {
port: 0,
keyfile: '',
certfile: '',
passfile: '',
cafiles: [],
};
-
export const LDAP = {
uri: 'ldap://example.com',
base: 'DC=example,DC=com',
principalSuffix: '@example.com',
adminGroup: 'admin',
};
+
+export const IDENTITY_TOKEN_SOURCE = undefined; // 'www.example.com';
+export const CUSTOM_IDENTITY_TOKEN_REQUEST_BODY = {};
Review Comment:
Are there specific attributes that are required to be present for `CUSTOM_IDENTITY_TOKEN_REQUEST_BODY`
Does it make sense to add a `type` or `interface for `CUSTOM_IDENTITY_TOKEN_REQUEST_BODY`
Example
```
export const CUSTOM_IDENTITY_TOKEN_REQUEST_BODY: {some_inline_type} = {};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] micahstubbs commented on a diff in pull request #2238: 2235/identity token
Posted by GitBox <gi...@apache.org>.
micahstubbs commented on code in PR #2238:
URL: https://github.com/apache/helix/pull/2238#discussion_r999764032
##########
helix-front/server/config.example.ts:
##########
@@ -5,20 +5,21 @@ export const HELIX_ENDPOINTS = {
},
],
};
-
export const SESSION_STORE = undefined;
-
export const SSL = {
port: 0,
keyfile: '',
certfile: '',
passfile: '',
cafiles: [],
};
-
export const LDAP = {
uri: 'ldap://example.com',
base: 'DC=example,DC=com',
principalSuffix: '@example.com',
adminGroup: 'admin',
};
+
+export const IDENTITY_TOKEN_SOURCE = undefined; // 'www.example.com';
+export const CUSTOM_IDENTITY_TOKEN_REQUEST_BODY = {};
Review Comment:
Added comments to clarify this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] micahstubbs commented on pull request #2238: 2235/identity token
Posted by GitBox <gi...@apache.org>.
micahstubbs commented on PR #2238:
URL: https://github.com/apache/helix/pull/2238#issuecomment-1284397902
This PR is ready to be merged, approved by @somecodemonkey @cliffordfajardo
Final commit message:
## Add support for optional identity token source in helix-front (#2235)
During helix-front login, optionally request an identity token
from a configurable identity token API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] micahstubbs commented on a diff in pull request #2238: 2235/identity token
Posted by GitBox <gi...@apache.org>.
micahstubbs commented on code in PR #2238:
URL: https://github.com/apache/helix/pull/2238#discussion_r999746102
##########
helix-front/server/config.example.ts:
##########
@@ -5,20 +5,21 @@ export const HELIX_ENDPOINTS = {
},
],
};
-
export const SESSION_STORE = undefined;
-
export const SSL = {
port: 0,
keyfile: '',
certfile: '',
passfile: '',
cafiles: [],
};
-
export const LDAP = {
uri: 'ldap://example.com',
base: 'DC=example,DC=com',
principalSuffix: '@example.com',
adminGroup: 'admin',
};
+
+export const IDENTITY_TOKEN_SOURCE = undefined; // 'www.example.com';
+export const CUSTOM_IDENTITY_TOKEN_REQUEST_BODY = {};
Review Comment:
Good question. I don't think so, since by definition it is custom, so it totally depends on the Identity Source / Auth Provider that you decide to use helix-front with. It would probably make sense to explicitly type this as `any` to communicate that better. Thanks for the suggestion!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] cliffordfajardo commented on a diff in pull request #2238: 2235/identity token
Posted by GitBox <gi...@apache.org>.
cliffordfajardo commented on code in PR #2238:
URL: https://github.com/apache/helix/pull/2238#discussion_r999698813
##########
helix-front/server/controllers/d.ts:
##########
@@ -5,6 +5,7 @@ export interface HelixUserRequest extends Request {
}
interface HelixSession {
+ identityToken: any;
Review Comment:
I think we should add a type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] micahstubbs commented on a diff in pull request #2238: 2235/identity token
Posted by GitBox <gi...@apache.org>.
micahstubbs commented on code in PR #2238:
URL: https://github.com/apache/helix/pull/2238#discussion_r999747263
##########
helix-front/server/controllers/d.ts:
##########
@@ -5,6 +5,7 @@ export interface HelixUserRequest extends Request {
}
interface HelixSession {
+ identityToken: any;
Review Comment:
I would generally agree, however since this token format is user-customizable, this really is `any` from helix-front's point of view. helix-front doesn't know what Identity Source it will be used with, or what type that Identity Source's identityToken will be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org