Skip to content
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
969918f
start refactoring creds behaviour for address
paulo-ocean Feb 26, 2025
2d638fd
more refactor
paulo-ocean Feb 26, 2025
2c23a9b
change default behaviour
paulo-ocean Feb 27, 2025
e9f98d5
change default behaviour, add unti test
paulo-ocean Feb 27, 2025
97f56a0
fix unit test
paulo-ocean Feb 27, 2025
8631883
fix unit test
paulo-ocean Feb 27, 2025
3836ce3
fix unit test
paulo-ocean Feb 27, 2025
e9db2c4
fix unit test
paulo-ocean Feb 27, 2025
56e7953
update asset credentials
paulo-ocean Feb 27, 2025
ae79d68
try debug it
paulo-ocean Feb 28, 2025
edbdd12
fix service levels credentials on asset
paulo-ocean Feb 28, 2025
af5b509
clean debug console.log
paulo-ocean Feb 28, 2025
02bfa35
Merge branch 'main' into issue-810-creds-address-behaviour
paulo-ocean Mar 7, 2025
a4498fc
add new types and match all/any rules
paulo-ocean Mar 7, 2025
01e249a
add new types and match all/any rules
paulo-ocean Mar 7, 2025
ed62637
extend check to match rules any or all
paulo-ocean Mar 10, 2025
c808dde
Merge branch 'main' into issue-810-creds-address-behaviour
paulo-ocean Mar 11, 2025
f1b87b6
add support for accessList on download checkCredentials
paulo-ocean Mar 11, 2025
329fb95
fix tests, async call
paulo-ocean Mar 11, 2025
d8972e0
Merge branch 'main' into issue-810-creds-address-behaviour
paulo-ocean Mar 11, 2025
263bf4a
Merge branch 'main' into issue-810-creds-address-behaviour
paulo-ocean Mar 14, 2025
4136d96
Merge pull request #873 from oceanprotocol/issue-871-checkCredentials…
paulo-ocean Mar 17, 2025
aaeb4d4
Merge branch 'main' into issue-810-creds-address-behaviour
giurgiur99 Apr 22, 2025
b9b5d77
use latest ddo-js
giurgiur99 Apr 23, 2025
0de2b5c
refactor match credentials
giurgiur99 Apr 23, 2025
fcf6830
Merge branch 'main' into issue-810-creds-address-behaviour
giurgiur99 Apr 28, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/@types/DDO/Credentials.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,21 @@
// we will have more (user defined)
export const KNOWN_CREDENTIALS_TYPES = ['address', 'accessList']
export const KNOWN_CREDENTIALS_TYPES = ['address', 'accessList'] // the ones we handle

export const CREDENTIAL_TYPES = {
ADDRESS: KNOWN_CREDENTIALS_TYPES[0],
ACCESS_LIST: KNOWN_CREDENTIALS_TYPES[1],
POLICY_SERVER_SPECIFIC: 'PS-specific Type' // externally handled by Policy Server
}
export interface Credential {
type?: string
values?: string[]
}

export type MATCH_RULES = 'any' | 'all'

export interface Credentials {
match_allow?: MATCH_RULES // any => it's enough to have one rule matched, all => all allow rules should match, default: 'all'
match_deny?: MATCH_RULES // same pattern as above, default is 'any'
allow?: Credential[]
deny?: Credential[]
}
4 changes: 2 additions & 2 deletions src/components/core/handler/downloadHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ export class DownloadHandler extends Handler {
).success
} else {
accessGrantedDDOLevel = areKnownCredentialTypes(ddo.credentials)
? checkCredentials(ddo.credentials, task.consumerAddress)
? checkCredentials(ddo.credentials, task.consumerAddress, ddo.chainId)
: true
}
if (!accessGrantedDDOLevel) {
Expand Down Expand Up @@ -397,7 +397,7 @@ export class DownloadHandler extends Handler {
).success)
} else {
accessGrantedServiceLevel = areKnownCredentialTypes(service.credentials)
? checkCredentials(service.credentials, task.consumerAddress)
? checkCredentials(service.credentials, task.consumerAddress, chainId)
: true
}

Expand Down
14 changes: 13 additions & 1 deletion src/test/data/assets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,10 @@ const nftLevelCredentials: Credentials = {
},
{
type: 'address',
values: ['0xA78deb2Fa79463945C247991075E2a0e98Ba7A09']
values: [
'0xA78deb2Fa79463945C247991075E2a0e98Ba7A09',
'0xe2DD09d719Da89e5a3D0F2549c7E24566e947260'
]
}
],
deny: [
Expand All @@ -84,6 +87,15 @@ const serviceLevelCredentials: Credentials = {
type: 'address',
values: ['0xA78deb2Fa79463945C247991075E2a0e98Ba7A09']
}
],
allow: [
{
type: 'address',
values: [
'0xe2DD09d719Da89e5a3D0F2549c7E24566e947260',
'0xBE5449a6A97aD46c8558A3356267Ee5D2731ab5e'
]
}
]
}

Expand Down
50 changes: 38 additions & 12 deletions src/test/unit/credentials.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
} from '../utils/utils.js'
import { ENVIRONMENT_VARIABLES } from '../../utils/constants.js'
import { homedir } from 'os'
import { DEVELOPMENT_CHAIN_ID } from '../../utils/address.js'

let envOverrides: OverrideEnvConfig[]

Expand All @@ -28,25 +29,26 @@ describe('credentials', () => {
envOverrides = await setupEnvironment(null, envOverrides)
})

it('should allow access with undefined or empty credentials', () => {
it('should deny access with undefined or empty credentials', () => {
const credentialsUndefined: Credentials = undefined
const consumerAddress = '0x123'
const accessGranted1 = checkCredentials(credentialsUndefined, consumerAddress)
expect(accessGranted1).to.equal(true)
expect(accessGranted1).to.equal(false)
const credentialsEmapty = {} as Credentials
const accessGranted2 = checkCredentials(credentialsEmapty, consumerAddress)
expect(accessGranted2).to.equal(true)
expect(accessGranted2).to.equal(false)
})
it('should allow access with empty allow and deny lists', () => {
it('should deny access with empty allow and deny lists', () => {
// if list does not exist or is empty access is denied
const credentials: Credentials = {
allow: [],
deny: []
}
const consumerAddress = '0x123'
const accessGranted = checkCredentials(credentials, consumerAddress)
expect(accessGranted).to.equal(true)
expect(accessGranted).to.equal(false)
})
it('should allow access with empty values in deny lists', () => {
it('should deny access with empty values in deny lists', () => {
const credentials: Credentials = {
deny: [
{
Expand All @@ -57,22 +59,22 @@ describe('credentials', () => {
}
const consumerAddress = '0x123'
const accessGranted = checkCredentials(credentials, consumerAddress)
expect(accessGranted).to.equal(true)
expect(accessGranted).to.equal(false)
})

it('should allow access with "accessList" credentials type', () => {
it('should deny access with "accessList" credentials (default behaviour if cannot check)', () => {
const consumerAddress = '0x123'
const credentials: Credentials = {
deny: [
{
type: 'accessList',
values: [consumerAddress]
values: [consumerAddress] // not a valid SC address anyway
}
]
}

const accessGranted = checkCredentials(credentials, consumerAddress)
expect(accessGranted).to.equal(true)
expect(accessGranted).to.equal(false)
})

it('should deny access with empty values in allow lists', () => {
Expand Down Expand Up @@ -101,7 +103,7 @@ describe('credentials', () => {
const accessGranted = checkCredentials(credentials, consumerAddress)
expect(accessGranted).to.equal(true)
})
it('should allow access with address not in deny list', () => {
it('should deny access with address not explicitly in deny list but also without any allow list', () => {
const credentials: Credentials = {
deny: [
{
Expand All @@ -112,7 +114,7 @@ describe('credentials', () => {
}
const consumerAddress = '0x123'
const accessGranted = checkCredentials(credentials, consumerAddress)
expect(accessGranted).to.equal(true)
expect(accessGranted).to.equal(false) // its not denied explicitly but not allowed either
})
it('should deny access with address in deny list', () => {
const credentials: Credentials = {
Expand Down Expand Up @@ -227,6 +229,30 @@ describe('credentials', () => {
expect(hasAddressMatchAllRule(creds2.credentials.allow)).to.be.equal(false)
})

it('should deny access by default if no specific allow rule is a match', () => {
const credentials: Credentials = {
allow: [
{
type: 'address',
values: []
}
],
deny: [
{
type: 'address',
values: []
}
]
}
const consumerAddress = '0x123'
const accessGranted = checkCredentials(
credentials,
consumerAddress,
DEVELOPMENT_CHAIN_ID
)
expect(accessGranted).to.equal(false)
})

after(async () => {
await tearDownEnvironment(envOverrides)
})
Expand Down
101 changes: 81 additions & 20 deletions src/utils/credentials.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { ethers, Signer } from 'ethers'
import { CORE_LOGGER } from './logging/common.js'
import {
Credential,
CREDENTIAL_TYPES,
Credentials,
KNOWN_CREDENTIALS_TYPES
} from '../@types/DDO/Credentials.js'
Expand All @@ -13,7 +14,7 @@ import { isDefined } from './util.js'
export function findCredential(
credentials: Credential[],
consumerCredentials: Credential
) {
): Credential | undefined {
return credentials.find((credential) => {
if (Array.isArray(credential?.values)) {
if (credential.values.length > 0) {
Expand All @@ -29,18 +30,38 @@ export function findCredential(
})
}

export function isAddressCredentialMatch(
credential: Credential,
consumerCredentials: Credential
): boolean {
if (credential?.type?.toLowerCase() !== CREDENTIAL_TYPES.ADDRESS) {
return false
}
if (credential.values.length > 0) {
const credentialValues = credential.values.map((v) => String(v)?.toLowerCase())
return credentialValues.includes(consumerCredentials.values[0])
}

return false
}

function isAddressMatchAll(credential: Credential): boolean {
if (credential?.type?.toLowerCase() !== CREDENTIAL_TYPES.ADDRESS) {
return false
}
if (credential.values.length > 0) {
const filteredValues: string[] = credential.values.filter((value: string) => {
return value?.toLowerCase() === '*' // address
})
return filteredValues.length > 0
}
return false
}

export function hasAddressMatchAllRule(credentials: Credential[]): boolean {
const creds = credentials.find((credential: Credential) => {
if (Array.isArray(credential?.values)) {
if (credential.values.length > 0 && credential.type) {
const filteredValues: string[] = credential.values.filter((value: string) => {
return value?.toLowerCase() === '*' // address
})
return (
filteredValues.length > 0 &&
credential.type.toLowerCase() === KNOWN_CREDENTIALS_TYPES[0]
)
}
return isAddressMatchAll(credential)
}
return false
})
Expand All @@ -52,29 +73,69 @@ export function hasAddressMatchAllRule(credentials: Credential[]): boolean {
* @param credentials credentials
* @param consumerAddress consumer address
*/
export function checkCredentials(credentials: Credentials, consumerAddress: string) {
export function checkCredentials(
credentials: Credentials,
consumerAddress: string,
chainId?: number
) {
const consumerCredentials: Credential = {
type: 'address',
type: CREDENTIAL_TYPES.ADDRESS, // 'address',
values: [String(consumerAddress)?.toLowerCase()]
}

const accessGranted = true
// if no address-based credentials are defined (both allow and deny lists are empty), access to the asset is restricted to everybody;
// to allow access to everybody, the symbol * will be used in the allow list;
// if a web3 address is present on both deny and allow lists, the deny list takes precedence
// and access to the asset is denied for the respective address.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm kind of surprised that we agreed to implement this change. So the default behaviour is that if an asset published, no one can access it? Seems contrary to our mission statement 🤔

Copy link
Contributor Author

@paulo-ocean paulo-ocean Mar 10, 2025

Choose a reason for hiding this comment

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

that's copied from the specs yes #804 #810 OceanProtocolEnterprise#25
makes perfectly sense to me for security reasons.

Copy link
Contributor

@jamiehewitt15 jamiehewitt15 Mar 10, 2025

Choose a reason for hiding this comment

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

It makes sense to enterprise users... In the past Ocean has always taken the approach that everything is open by default and Enterprises must take extra steps if they don't like the default. So this is a fundamental shift in that approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not gonna discuss that... please check with @alexcos20 . I just followed the specs and they make sense to me

Copy link
Contributor

@jamiehewitt15 jamiehewitt15 Mar 11, 2025

Choose a reason for hiding this comment

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

I don't see anything changing if we have a discussion, you may as well just go ahead. Nothing wrong with the implementation

const accessGranted = false
// check deny access
// https://github.com/oceanprotocol/ocean-node/issues/810
// for deny rules: if value does not exist or it's empty -> there is no deny list. if value list has at least one element, check it

if (Array.isArray(credentials?.deny) && credentials.deny.length > 0) {
const accessDeny = findCredential(credentials.deny, consumerCredentials)
// credential is on deny list, so it should be blocked access
if (accessDeny) {
let denyCount = 0
for (const cred of credentials.deny) {
const { type } = cred
if (type === CREDENTIAL_TYPES.ADDRESS) {
const accessDeny = isAddressCredentialMatch(cred, consumerCredentials)
// credential is on deny list, so it should be blocked access
if (accessDeny) {
if (!isDefined(credentials.match_deny) || credentials.match_deny === 'any') {
return false
}
}
denyCount++
// credential not found, so it really depends if we have a match on the allow list instead
}
}
if (credentials.match_deny === 'all' && denyCount === credentials.deny.length) {
return false
}
// credential not found, so it really depends if we have a match
}
// check allow access
// for allow rules: if value does not exist or it's empty -> no one has access. if value list has at least one element, check it
if (Array.isArray(credentials?.allow) && credentials.allow.length > 0) {
const accessAllow = findCredential(credentials.allow, consumerCredentials)
if (accessAllow || hasAddressMatchAllRule(credentials.allow)) {
let matchCount = 0
for (const cred of credentials.allow) {
const { type } = cred
if (type === CREDENTIAL_TYPES.ADDRESS) {
const accessAllow = isAddressCredentialMatch(cred, consumerCredentials)
if (accessAllow || isAddressMatchAll(cred)) {
// if no match_allow or 'any', its fine
if (!isDefined(credentials.match_allow) || credentials.match_allow === 'any') {
return true
}
// otherwise, match 'all', in this case the amount of matches should be the same of the amount of rules
matchCount++
}
}
// extend function to ACCESS_LIST (https://github.com/oceanprotocol/ocean-node/issues/804)
// else if (type === CREDENTIAL_TYPES.ACCESS_LIST && chainId) {
// }
}
if (credentials.match_allow === 'all' && matchCount === credentials.allow.length) {
return true
}
return false
}
return accessGranted
}
Expand Down
Loading