-
Notifications
You must be signed in to change notification settings - Fork 22
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 support for linked SAML nameId to audit table #78
Conversation
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 a really great addition @sennap. 😍 I can already think of a few companies that would love to use this!
The logic of how you implemented it is great as well. I added a few minor comments that are more about best practices.
src/index.js
Outdated
const enterprise = core.getInput("enterprise") || process.env.ENTERPRISE; | ||
const samlIdentities = core.getInput("samlIdentities") || process.env.samlIdentities |
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.
Today I learned that the core.getInput
method parses all values as strings. (There is an open PR about this)
it would be my idea to convert samlIdentities
to a boolean to guarantee type safety within the CollectUserData
class. By doing this we also don't have to do an explicit equality comparison as we do now at line 294 and 300, but can just check if this.samlIdentities
is truthy.
So here we can do something along the lines of
const samlIdentities = core.getInput("samlIdentities") || process.env.samlIdentities | |
const samlIdentities = (core.getInput("samlIdentities") || process.env.samlIdentities) === 'true' |
And then we can replace this.samlIdentities == "true" && ......
in the method with this.samlIdentities && .....
or this.samlIdentities === true && ....
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.
AH that makes sense. Took me a while there to figure out that was happening. Yes that's a great idea - updated!
src/index.js
Outdated
@@ -21,7 +21,7 @@ const ERROR_MESSAGE_TOKEN_UNAUTHORIZED = | |||
!fs.existsSync(DATA_FOLDER) && fs.mkdirSync(DATA_FOLDER); | |||
|
|||
class CollectUserData { | |||
constructor(token, organization, enterprise, options) { | |||
constructor(token, organization, enterprise, samlIdentities, options ) { |
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.
Could we make samlIdentities
part of the options
object? I have two reasons for this:
- To have all optional arguments in the options object, like
postToIssue
andrepository
are at the moment. Omitting anything in this object shouldn't result in a failure. Omitting the token, organization/enterprise does make it fail at the moment. Just so there is a clear distinction. - To keep the API stable. I assume no one is using this class outside of a GitHub Action at the moment, but just in case someone is using it and initiates
CollectUserData
directly. The current change means the argument order changes (options
become the 4th argument). MakingsamlIdentities
part of the options object would fix this.
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.
Yes I like this idea too for consistency ⚡ thanks for the pointer on the API
src/index.js
Outdated
} | ||
|
||
let externalIdentities; | ||
if (useSamlIdentities == true) { |
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.
Could you make these types of comparisons strictly equal?
if (useSamlIdentities == true) { | |
if (useSamlIdentities === true) { |
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.
great catch 💯 fixed the three conditions where that's used
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!
thank you so much for your help and review! I'm not able to merge it myself though - looks like one those with write access can 🙂 |
What this PR does:
If
samlIdentities: true
is specified in the workflow and an organization has SAML enabled, asamlIdentity
column is added to the output for each member.Note: this currently only functions for
organization
audits and no extra column or SAML identities are added for anenterprise
audit (need to focus on how that will work in the meantime)Updated some of the documentation as well!
@SvanBoxel let me know what you think ... and if there are any adjustments I need to make to index.js with where things are done, etc. 🙏